Switch QuickJS to QuickJS-NG #369
Conversation
|
Hey folks! Very excited to see this! Let me know if there is anything I can do to help on the QuickJS-ng side of things :-) I see there are still a couple of patches applied, feel free to send them over and we can discuss them!
This is a leak. You can enable leak dumping and it should tell you what it is that got leaked. |
Fantastic, thanks for the support @saghul 🎉
Let's do it, we can probably expose return unlikely(js_get_stack_pointer() - alloca_size < rt->stack_limit);vs. return unlikely(js_get_stack_pointer() < rt->stack_limit + alloca_size);I don't know why this was added. The first expression makes the comparison after subtracting the allocation size from the current stack pointer. The second expression makes the comparison by adjusting the stack limit instead, then comparing the current stack pointer directly.
Yep. However it's seems to be affected by races. Also might be a bug in the refactoring of allocator. When for example running each test in class.rs individually, they work. When running the whole test suite, it fails with either memory leak or Here are some leak information: |
What kind of races? Are you running multiple threads using the same runtime? |
No this is the thing that's strange. They all run in separate runtimes. The only tests that are failing now are Line 381 in bb4a97b Running each test individually works, running them together, fails with either memory reference error or leaking objects. |
|
Running with all dump-flags i'll get this output JS reference/memory leak: And i have attached dump for |
|
@DelSkayn memory reference error occurs when running finalizer for /// FFI finalizer, destroying the object once it is delete by the Gc.
pub(crate) unsafe extern "C" fn finalizer<'js, C: JsClass<'js>>(
rt: *mut qjs::JSRuntime,
val: qjs::JSValue,
) {
let ptr = qjs::JS_GetOpaque(val, C::class_id().get(rt)).cast::<JsCell<C>>();
debug_assert!(!ptr.is_null());
let inst = Box::from_raw(ptr);
mem::drop(inst); // <---- causes memory reference error when class is is RustFunction
} |
|
I am fine with switching to NG! I have actually been looking into these issues but haven't been able to locate the exact problem. |
That's awesome! Thank you!
I can consistency replicate the memory reference issue by moving all class tests to a single function. It's now consistency failing when finalizer runs for 
mod tests#[cfg(test)]
mod test {
use std::sync::{
atomic::{AtomicBool, Ordering}, Arc
};
use crate::{
class::{ClassId, JsClass, Readable, Trace, Tracer, Writable},
function::This,
test_with,
value::Constructor,
Class, Context, FromJs, Function, IntoJs, Object, Runtime,
};
/// Test circular references.
#[test]
fn class_tests() {
pub struct Container<'js> {
inner: Vec<Class<'js, Container<'js>>>,
test: Arc<AtomicBool>,
}
impl<'js> Drop for Container<'js> {
fn drop(&mut self) {
self.test.store(true, Ordering::SeqCst);
}
}
impl<'js> Trace<'js> for Container<'js> {
fn trace<'a>(&self, tracer: Tracer<'a, 'js>) {
self.inner.iter().for_each(|x| x.trace(tracer))
}
}
impl<'js> JsClass<'js> for Container<'js> {
const NAME: &'static str = "Container";
type Mutable = Writable;
fn class_id() -> &'static crate::class::ClassId {
static ID: ClassId = ClassId::new();
&ID
}
fn prototype(ctx: &crate::Ctx<'js>) -> crate::Result<Option<crate::Object<'js>>> {
Ok(Some(Object::new(ctx.clone())?))
}
fn constructor(
_ctx: &crate::Ctx<'js>,
) -> crate::Result<Option<crate::value::Constructor<'js>>> {
Ok(None)
}
}
let rt = Runtime::new().unwrap();
let ctx = Context::full(&rt).unwrap();
let drop_test = Arc::new(AtomicBool::new(false));
ctx.with(|ctx| {
let cls = Class::instance(
ctx.clone(),
Container {
inner: Vec::new(),
test: drop_test.clone(),
},
)
.unwrap();
let cls_clone = cls.clone();
cls.borrow_mut().inner.push(cls_clone);
});
rt.run_gc();
assert!(drop_test.load(Ordering::SeqCst));
ctx.with(|ctx| {
let cls = Class::instance(
ctx.clone(),
Container {
inner: Vec::new(),
test: drop_test.clone(),
},
)
.unwrap();
let cls_clone = cls.clone();
cls.borrow_mut().inner.push(cls_clone);
ctx.globals().set("t", cls).unwrap();
});
#[derive(Clone, Copy)]
pub struct Vec3 {
x: f32,
y: f32,
z: f32,
}
impl Vec3 {
pub fn new(x: f32, y: f32, z: f32) -> Self {
Vec3 { x, y, z }
}
pub fn add(self, v: Vec3) -> Self {
Vec3 {
x: self.x + v.x,
y: self.y + v.y,
z: self.z + v.z,
}
}
}
impl<'js> Trace<'js> for Vec3 {
fn trace<'a>(&self, _tracer: Tracer<'a, 'js>) {}
}
impl<'js> FromJs<'js> for Vec3 {
fn from_js(ctx: &crate::Ctx<'js>, value: crate::Value<'js>) -> crate::Result<Self> {
Ok(*Class::<Vec3>::from_js(ctx, value)?.try_borrow()?)
}
}
impl<'js> IntoJs<'js> for Vec3 {
fn into_js(self, ctx: &crate::Ctx<'js>) -> crate::Result<crate::Value<'js>> {
Class::instance(ctx.clone(), self).into_js(ctx)
}
}
impl<'js> JsClass<'js> for Vec3 {
const NAME: &'static str = "Vec3";
type Mutable = Writable;
fn class_id() -> &'static crate::class::ClassId {
static ID: ClassId = ClassId::new();
&ID
}
fn prototype(ctx: &crate::Ctx<'js>) -> crate::Result<Option<crate::Object<'js>>> {
let proto = Object::new(ctx.clone())?;
let func =
Function::new(ctx.clone(), |this: This<Vec3>, other: Vec3| this.add(other))?
.with_name("add")?;
proto.set("add", func)?;
Ok(Some(proto))
}
fn constructor(
ctx: &crate::Ctx<'js>,
) -> crate::Result<Option<crate::value::Constructor<'js>>> {
let constr =
Constructor::new_class::<Vec3, _, _>(ctx.clone(), |x: f32, y: f32, z: f32| {
Vec3::new(x, y, z)
})?;
Ok(Some(constr))
}
}
test_with(|ctx| {
Class::<Vec3>::define(&ctx.globals()).unwrap();
let v = ctx
.eval::<Vec3, _>(
r"
let a = new Vec3(1,2,3);
let b = new Vec3(4,2,8);
a.add(b)
",
)
.unwrap();
approx::assert_abs_diff_eq!(v.x, 5.0);
approx::assert_abs_diff_eq!(v.y, 4.0);
approx::assert_abs_diff_eq!(v.z, 11.0);
let name: String = ctx.eval("new Vec3(1,2,3).constructor.name").unwrap();
assert_eq!(name, Vec3::NAME);
});
pub struct X;
impl<'js> Trace<'js> for X {
fn trace<'a>(&self, _tracer: Tracer<'a, 'js>) {}
}
impl<'js> JsClass<'js> for X {
const NAME: &'static str = "X";
type Mutable = Readable;
fn class_id() -> &'static ClassId {
static ID: ClassId = ClassId::new();
&ID
}
fn prototype(ctx: &crate::Ctx<'js>) -> crate::Result<Option<Object<'js>>> {
Object::new(ctx.clone()).map(Some)
}
fn constructor(_ctx: &crate::Ctx<'js>) -> crate::Result<Option<Constructor<'js>>> {
Ok(None)
}
}
test_with(|ctx| {
Class::<X>::register(&ctx).unwrap();
Class::<X>::register(&ctx).unwrap();
})
}
} |
|
Some more investigation shows something wrong with the atoms. For example for working However when running all tests together they are instead log as: Where the |
|
I found the issue. The problem is that class id's are no longer a global value in quickjs-ng. The current class functionality are designed around this being a single global value. When a class is registered for the first time, during execution, in a runtime we store the new class id returned in a static global associated to the type. When we convert to a class from an object we then use this stored id to check if the object is of the right type and we use it to check if a type is already registered. With the class id's no longer being global but instead local to the runtime we now store conflicting id's. In your reproduction RustFunction and Vec3 have the same class id because RustFunction was registered by the first run test and Vec3 is only registered in the second. In the second test the RustFunction class then doesn't request a new class id since that is already initialized and then registers itself under a class id which is not yet returned. Vec3 then gets the same class id as RustFunction and it thinks it is already registered so we don't set the right finalizer. This causes Vec3 object to be stored with the finalizer from RustFunction which causes the segfault when the runtime tries to drop the Vec3 object. If we want to fix this we will have to redesign how types are registered. |
|
Nice find! Global class IDs were broken since the beginning, specially if you had multiple runtimes registering classes with globals as used by quickjs-libc.c for example. IMHO, ng or not, it's probably a good idea to make sure the registered class IDs are per runtime. |
|
Awesome finding. I was suspecting this as well but could not figure out why. Your explanation was the missing piece of the puzzle. I’ll refactor the class id implementation and test later today! |
|
Ok should be in a pretty good state to review now. What I see now is some Windows failings, which may be related to non-updated bindings. |
|
@DelSkayn something super strange is occurring in CI which i can't explain. When building with |
|
I have implemented a rework of the class system in #373 which doesn't require a hashmap lookup for every time we try to convert a quickjs object to a rust class, just two checks. We only need a hashmap to lookup class prototypes. It also removes the rust Let me know what you think. |
Cool! I will take a look. I just pushed an update where i use tinyvec to store an array of classIds (since it's low integer incremented by 1) and we can use that as backing storage. There is no heap allocation unless we register more than 1024 classes per runtime and lookups are very fast since they're based on indexes instead of hashes. This solves a lot of issues as well. |
|
@DelSkayn |
|
@DelSkayn F.Y.I I have now rebased on the new class system |
|
Great work @richarddd ! 🙌 |
Changes QuickJS to QuickJS NG.
Significant changes:
Status:
Project currently builds with a lot of successful tests but some tests are failing due to memory leaks and invalid allocations:
@DelSkayn feel free to check out this branch or provide feedback here. LMKWYT!
I might need some support ironing out the last memory reference/leak bugs