Closed Bug 1804253 Opened 2 months ago Closed 2 months ago

Improve C++ class hierarchy for Shapes

Categories

(Core :: JavaScript Engine, task, P3)

task

Tracking

()

RESOLVED FIXED
110 Branch
Tracking Status
firefox110 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(6 files)

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).

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.

Assignee: nobody → jdemooij
Status: NEW → ASSIGNED

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).

Depends on: 1804394

(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.

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

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

Depends on D164212

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

Depends on D164213

Depends on D164214

Pushed by jdemooij@mozilla.com:
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
Blocks: 1805430
Regressions: 1806006
Blocks: 1806389
You need to log in before you can comment on or make changes to this bug.