1804253 - Improve C++ class hierarchy for Shapes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

It would be nice if the C++ Shape type had a class hierarchy like this:

Shape
   NativeShape => for NativeObject
      SharedShape
      DictionaryShape
   ProxyShape => for ProxyObject
   WasmGCShape => for Wasm GC objects

After the ReShape work this shouldn't be too hard to do I think.

We could then move the proxy handler from the proxy object to the proxy shape, and similarly store Wasm-GC-specific data in WasmGCShape (this is why I'm filing this now).

Comment 1

[Jan de Mooij [:jandem]]

Also, some of the JSClass hooks and flags could potentially be replaced by switching on the "shape kind" enum. This is especially nice for the very hot object->is<NativeObject>() check.

Comment 2

[Ryan Hunt [:rhunt]]

Attachment: wasm-gc-shape.patch

In case it's helpful, here's the WIP patch I have for wasm GC shapes. You can ignore the part about keeping a HashSet of them, I don't need that anymore.

Really the only point for a WasmGcShape at this point is to keep a strong reference to the atomically refcounted wasm::RecGroup so that all WasmGcObject's don't need finalizers that perform the atomic dec-ref.

Any actual access to the type information is going to be done through the second header word of WasmGcObject, which will be a raw pointer to the wasm::TypeDef (stored in the recursion group kept alive by the shape).

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Ryan Hunt [:rhunt] from comment #2)

In case it's helpful, here's the WIP patch I have for wasm GC shapes.

That looks good. I want to clean up the Shape subclasses a bit before adding more of them, but the result will be similar.

Adding separate classes for proxy shapes and wasm gc objects will be nice. We can then add NativeShape as base class of SharedShape and DictionaryShape, so shapes will be less biased towards native objects.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1804253 part 1 - Factor out ShapeBaseHasher helper class. r?jonco!

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1804253 part 2 - Add ProxyShape and WasmGCShape. r?jonco!

Add separate Shape subclasses. SharedShape is now only used for native objects.
This will make it possible to store other data in Proxy/WasmGC shapes in the future.

Depends on D164211

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1804253 part 3 - Add NativeShape base class. r?jonco!

The Shape methods related to property information and number of fixed slots are
moved into NativeShape, to improve type safety.

Depends on D164212

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1804253 part 4 - Optimize checks for native objects and proxies. r?jonco!

This saves two dereferences by looking at the shape instead of the class.

Depends on D164213

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1804253 part 5 - Fix code comments. r?jonco!

Depends on D164214

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8492a84977c9
part 1 - Factor out ShapeBaseHasher helper class. r=jonco
https://hg.mozilla.org/integration/autoland/rev/e9ec0e089b0d
part 2 - Add ProxyShape and WasmGCShape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/6e25cfb0c2c9
part 3 - Add NativeShape base class. r=jonco
https://hg.mozilla.org/integration/autoland/rev/fec2ed2fbde3
part 4 - Optimize checks for native objects and proxies. r=jonco
https://hg.mozilla.org/integration/autoland/rev/79c3e5d3415f
part 5 - Fix code comments. r=jonco

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/8492a84977c9
https://hg.mozilla.org/mozilla-central/rev/e9ec0e089b0d
https://hg.mozilla.org/mozilla-central/rev/6e25cfb0c2c9
https://hg.mozilla.org/mozilla-central/rev/fec2ed2fbde3
https://hg.mozilla.org/mozilla-central/rev/79c3e5d3415f