Refactor `JsObject` to always be size 8

[raskad]

This is currently a draft, since I think there are some important design decisions that need to be discussed.If you look at this, best is to review the changes commit by commit.

This PR contains two changes that can be best viewed separately.

1. Refactor JsObject to always be a size 8 thin pointer (Gc<T: Sized>).
2. Building on 1., remove the Box from the nan-boxed JsObject value.

Motivation

I think the ideal representation for JsObject and JsValue should be a no Drop + Copy size 8 value. Currently we have a lot of drop and clone code in our runtime, caused by our ref-counting GC, as well as a lot of Box allocations from our current nan-box creation / clones. Hopefully we can get rid of the Drop and Clone requirements for Gc in the future. Then we could also move all other pointer types (JsString, JsSymbol and JsBigInt) to be Gc values, to enable JsValue to be Copy.

Refactor JsObject to always be a size 8 thin pointer (Gc<T: Sized>)

Currently our JsObject type can be either typed (e.g. JsObject<u64>) or erased (e.g. JsObject == JsObject<dyn NativeObject>). While there is only a Gc field on JsObject, the erased variant is a size 16 fat pointer, to store the Rust DST vtable for dyn NativeObject. The goal of this change is to make JsObject always a transparent size 8 thin pointer (Gc) to enable storing it directly as a nan-boxed value.

Here are the requirements for JsObject that I tried to keep:

• JsObject needs to be a size 8 thin pointer.
• Typed and erased variants should be one struct (erased being the default).
  • Possible alternative: JsObject could be a non generic struct and JsObject::downcast could return a typed struct. The positive of this approach would be, that access to the data T of the object can be controlled better, which would probably make everything safer. One drawback, that I currently see, is, that functions that should work on JsObject<T> would have to be either specific to one variant, or need to be implemented twice. I have not tried this approach, but I think in theory it should be possible.
• Any type that implements NativeObject should be able to be used as T for JsObject without any other user interactions (e.g. registering the type, manual unsafe vtable implementation, ...)

This lead me to the current design in this PR. Instead of erasing the object into a dyn NativeObject, the TypeId is stored in the object itself and used to check during casts. The default T is erased into a size 0 struct. I don't really like this approach, since one could access the erased struct. It might also be that some of this is inherently unsafe. Any ideas are welcome.

Using the JsObject Gc pointer as the value for nan boxing.

At first this seems straight forward. But since there is no additional Box around the real value, it seems to me, that returning a reference to the JsObject from a nan boxed JsValue is impossible. This alters the JsValue::as_object API to return a Option<JsObject> instead of Option<&JsObject>, which cascades out into some breaking changes in our JsValue APIs and some arguably less ergonomic usage patterns. I think we should break the APIs, since when we move all pointer types and JsValue to being Copy, we basically don't want to work with references to those types anyway and always take them as values. This would of course mean a lot of breaking changes in our APIs, but I think it would be the logical thing to do.

Benchmarks

The current changes obviously don't change anything related to the GC, but removing the additional Box for nan-boxed JsObjects seems to already make a difference. Local benchmark results:

Before

RESULT Richards 221
RESULT DeltaBlue 218
RESULT Crypto 208
RESULT RayTrace 485
RESULT EarleyBoyer 591
RESULT RegExp 104
RESULT Splay 829
RESULT NavierStokes 485
SCORE 324

After

RESULT Richards 276
RESULT DeltaBlue 281
RESULT Crypto 226
RESULT RayTrace 587
RESULT EarleyBoyer 721
RESULT RegExp 110
RESULT Splay 1039
RESULT NavierStokes 513
SCORE 380

[github-actions[bot]]

Test262 conformance changes

Test result	main count	PR count	difference
Total	50,595	50,595	0
Passed	47,238	47,238	0
Ignored	1,964	1,964	0
Failed	1,393	1,393	0
Panics	0	0	0
Conformance	93.36%	93.36%	0.00%

[hansl]

This alters the JsValue::as_object API to return a Option<JsObject> instead of Option<&JsObject>, which cascades out into some breaking changes in our JsValue APIs and some arguably less ergonomic usage patterns.

I think this is a very good change, and will inevitably be done for the other "pointer" types (BigInt, String, etc). If we can make JsObject Copy, there shouldn't be any references to it anywhere anymore. Should improve benchmarks even further. I like this a lot.

[jedel1043]

Instead of erasing the object into a dyn NativeObject, the TypeId is stored in the object itself and used to check during casts. The default T is erased into a size 0 struct. I don't really like this approach, since one could access the erased struct.

I don't think this matters too much, because writes to zero-sized structs are replaced by no-ops IIRC. Maybe we would have to add a #[repr(C)] to Object though, so that the compiler doesn't reorder the data field.

I'm wondering why do we need to wrap the data with a Box though. Is it because of the ?Sized requirement? Because if we're using a zero-sized type as our placeholder, I think we can remove that from Object.

[raskad]

I don't think this matters too much, because writes to zero-sized structs are replaced by no-ops IIRC. Maybe we would have to add a #[repr(C)] to Object though, so that the compiler doesn't reorder the data field.

Alright, I will add some tests to check if that all works as expected.

I'm wondering why do we need to wrap the data with a Box though. Is it because of the ?Sized requirement?

It's because of the alignment of Object. If we don't box T, the alignment would depend on the alignment of T. That would mean, that we cannot safely access fields on Object while it is erased (at least as far as I understand).

Because if we're using a zero-sized type as our placeholder, I think we can remove that from Object.

You mean completely removing the ?Sized requirement from Object and JsObject? That was also something I though about and I think it could be removed now, because it was only there to allow for the erased dyn NativeObject, correct?

[jedel1043]

That was also something I though about and I think it could be removed now, because it was only there to allow for the erased dyn NativeObject, correct?

Yes. We could technically enable something like Object<[u8]> if we left the ?Sized requirement, but I think NativeObject requires Sized, so it wouldn't be useful at all to have that if we're removing dyn NativeObject.

It's because of the alignment of Object. If we don't box T, the alignment would depend on the alignment of T. That would mean, that we cannot safely access fields on Object while it is erased (at least as far as I understand).

Maybe the field is already aligned to 8 bytes thanks to the rest of the fields? We could also force #[repr(C, align(64)] and add padding so that the data field is always 8 byte aligned.

[raskad]

Yes. We could technically enable something like Object<[u8]> if we left the ?Sized requirement, but I think NativeObject requires Sized, so it wouldn't be useful at all to have that if we're removing dyn NativeObject.

Sounds good!

Maybe the field is already aligned to 8 bytes thanks to the rest of the fields? We could also force #[repr(C, align(64)] and add padding so that the data field is always 8 byte aligned.

Can we force the struct into a specific alignment? The reference for repr C documents, that the alignment is always the alignment of the most aligned field. When I test the following code, Object is still 2^29 aligned:

#[repr(align(536870912))]
struct Inner {
    field: u8,
}

#[repr(C, align(64))]
struct Object<T> {
    field: T,
}

[jedel1043]

Can we force the struct into a specific alignment? The reference for repr C documents, that the alignment is always the alignment of the most aligned field. When I test the following code, Object is still 2^29 aligned:

Oh true, I think I got confused by how #[repr(C)] works. Thinking it through a bit more, to avoid the Box we just need to add #[repr(C)], because in that case the automatic padding to align data would be just before data itself, right? Thus, the previous fields won't change their offset, making it them safe to access.

[jedel1043]

Possible alternative: JsObject could be a non generic struct and JsObject::downcast could return a typed struct. The positive of this approach would be, that access to the data T of the object can be controlled better, which would probably make everything safer. One drawback, that I currently see, is, that functions that should work on JsObject<T> would have to be either specific to one variant, or need to be implemented twice. I have not tried this approach, but I think in theory it should be possible.

Note that most of our methods are only implemented for JsObject<dyn NativeObject>, because most of the JS functions only take the erased version of JsObject. There are only a handful of methods implemented for JsObject<T>, so going with this approach shouldn't be that much of a problem I think.

[raskad]

Oh true, I think I got confused by how #[repr(C)] works. Thinking it through a bit more, to avoid the Box we just need to add #[repr(C)], because in that case the automatic padding to align data would be just before data itself, right? Thus, the previous fields won't change their offset, making it them safe to access.

I tried that and it produced UB. I don't have the code anymore, but I think I added repr(C) to both the Object and VTableObject. The offsets of the previous fields would be the same [1], but the fields inside of Object before T would not be fine, since Object is at a different offset.

use std::{any::TypeId, fmt::Debug};

fn main() {
    dbg!(std::mem::offset_of!(VTableObject<Erased>, t));        // 0
    dbg!(std::mem::offset_of!(VTableObject<Erased>, vtable));   // 16
    dbg!(std::mem::offset_of!(VTableObject<Erased>, data));     // 24

    dbg!(std::mem::offset_of!(VTableObject<u128>, t));          // 0
    dbg!(std::mem::offset_of!(VTableObject<u128>, vtable));     // 16
    dbg!(std::mem::offset_of!(VTableObject<u128>, data));       // 32
}

#[derive(Debug)]
struct Erased {}

#[repr(C)]
struct VTableObject<T = Erased> {
    t: TypeId,
    vtable: & 'static InternalMethods,
    data: Object<T>,
}

struct InternalMethods {
    debug: fn() -> & 'static str
}

impl Debug for InternalMethods {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.write_str((self.debug)())
    }
}

#[repr(C)]
struct Object<T = Erased> {
    properties: Vec<usize>,
    data: T,
}

[1]: I tried to wrap that example code with a dummy JsObject (just a NonNull) and cased it. Running it with already throws an error when accessing the vtable field.

[jedel1043]

@raskad Ahh yeah, I didn't consider the alignment of VTableObject, and any other struct that wraps Object.

[jedel1043]

The only other way I see to remove Box is to store the offset to the data as a pointer, then use that for access:

#[repr(C)]
struct VTableObject<T = Erased> {
    t: TypeId,
    vtable: & 'static InternalMethods,
    data_ptr: *mut Object<T>,
    data: Object<T>,
}

But maybe this would complicate the refactor a lot more, so it would be arguably better to use Box for now and leave this as a future improvement.

[HalidOdat]

Hmmm... I think we can leave the removal of the box in separate PR, just so it doen't get to big :sweat_smile:....

Just wanted to give a potential solution for it: What we could do is restrict the min and max alignment that a JsData can be.

For the min we can create a wrapper struct with the #[align(N)] property.

// Force alignment to be the same for all `JsData` types.
#[repr(C, align(16))]
struct ObjectData<T> {
    data: T,
}

We can wrap object type:

#[repr(C)]
struct Object<T = Erased> {
    properties: Vec<usize>,
    data: ObjectData<T>,
}

As for the max we can use some const assert!s

impl<T> ObjectData<T> {
    const IS_VALID: bool = std::mem::align_of::<T>() <= 16;
    const ALIGNMENT_ASSERT: () = assert!(Self::IS_VALID, "Alignment of JsData must be <= 16, NOTE: you can wrap the data in a Box<T>.");

    pub fn new(value: T) -> Self {
        // force const assertion to triger when we instantiate.
        let _ = Self::ALIGNMENT_ASSERT;
        
        Self {
            data: value,
        }
    }
}

I couldn't get the assert to trigger when we instantiate the type, it triggers when we instantiate the new() function and will cause a compile error with the assert! message

See code (changes denoted with comments :) ): https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=4abfd21414195b7d66863ce1208fc4d5

[raskad]

Got an update on this. I rebased this on the changes from #4291 and used GcErased in JsObject. @HalidOdat I needed to switch the *cast functions on GcErased to use take a value instead of a reference, to avoid clones of Gc<T>. For most other scenarios, I added a downcast_ref function. What do you think abou this?

Building on you comment @jedel1043

Note that most of our methods are only implemented for JsObject<dyn NativeObject>, because most of the JS functions only take the erased version of JsObject. There are only a handful of methods implemented for JsObject<T>, so going with this approach shouldn't be that much of a problem I think.

I added a JsObjectTyped<T> struct that can be used for the downcasted JsObjects. You where totally correct, there where only a couple of methods, that I just copied for now. The biggest breaking change would be, that users would need to change their usages. What do you all think (you can see our internal usage changes to guage the effort).

[HalidOdat]

[..] I needed to switch the *cast functions on GcErased to use take a value instead of a reference, to avoid clones of Gc<T>

Do we actually need this change? Could we instead call clone inside JsObject's .downcast methods?

The previous implementation had the advantage of avoiding clones (reference count increments) when accessing the type. I'm totally fine with the change either way, just curious! 😄

[raskad]

Do we actually need this change? Could we instead call clone inside JsObject's .downcast methods?

The previous implementation had the advantage of avoiding clones (reference count increments) when accessing the type. I'm totally fine with the change either way, just curious! 😄

With the latest change, I only use Gc::cast_unchecked. As far I can see, there should be no clone with Gc::cast_unchecked taking Self.