1689413 - Remove ObjectGroup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

edit: this comment refers to the original plan before the bug was morphed into removing ObjectGroup instead:

BaseShape is just 4 words, it might make sense to merge into Shape:

• flags_ could likely be merged into Shape flags word.
• The unowned_ pointer would go away.
• clasp could then be removed from ObjectGroup (or Shape, but that's harder)
• Shape obviously no longer needs a pointer to the BaseShape
• It would remove some pointer chasing.

I prototyped this a bit last week and it seems doable.

Comment 1

[Jan de Mooij [:jandem]]

I should have time for this in a few weeks.

Comment 2

[Jan de Mooij [:jandem]]

We'll have to see how this affects memory usage. It might be fine or even slightly better than now, but we just have to measure.

Comment 3

[Jan de Mooij [:jandem]]

I prototyped removing BaseShape yesterday. It adds a single word to Shape so it's a memory regression, but it did allow removing the JSClass* from ObjectGroup. It removes a lot of code though, in particular owned vs unowned BaseShapes are complicated.

---

After thinking/talking this through more, we now think it's worth keeping BaseShape around for a bit and instead:

1. Simplify BaseShape by moving the "cache pointer" and object flags to Shape. This adds a word to Shape but it lets us remove a bunch of complexity and overhead around BaseShapes + the indirection for the shape cache pointer.
2. Move the realm and proto from ObjectGroup to BaseShape.
3. Remove ObjectGroup.

For TypedObjects: move TypeDescr from ObjectGroup to the object. This could be stored as second word (instead of the current ObjectGroup) so that we can still do a single branch for TypedObject layout guards, relying on other objects never storing a bit-identical value there. The second (currently third) word will have to move into JSObject to represent that invariant nicely.

Advantages here are:

• Removes a word from every JS object.
• Removes ObjectGroup overhead and complexity.
• Still simplifies BaseShape quite a bit.

The downsides:

• Adds an extra indirection for {class, proto, realm} pointers.
• __proto__ mutation would always require a BaseShape + Shape change, no more UNCACHEABLE_PROTO flag. This does simplify the system.

I don't expect it but if the UNCACHEABLE_PROTO flag removal turns out to be an issue, we could bring it back by allowing the object to store the prototype in a fixed/dynamic slot: in this case the BaseShape would store a slot index instead of the proto itself. This adds complexity and branches so isn't perfect, but we do have options there.

Comment 4

[Jan de Mooij [:jandem]]

I have a WIP patch stack for this. Looking at Linux64 numbers on Try:

• Base Content JS is about 35K (~1.4%) less.
• For awsy-tp6 the improvement is a bit bigger, ~4% for JS memory.
• Speedometer and JetStream 2 aren't affected much, maybe a tiny bit faster but hard to say with only a few data points.

Memory reports are mostly as expected:

• Objects are smaller.
• ObjectGroups are gone.
• Shapes have an extra word so use more memory.
• BaseShapes are one word smaller; a ShapeTable/ShapeIC no longer requires a new BaseShape. Memory usage is similar to current ObjectGroup memory (expected because this basically turns BaseShape into ObjectGroup).

The number of GC-things drops a bit (no more groups, fewer BaseShapes) so I hope that will have a positive impact.

This also removes a lot of complexity (object groups, owned BaseShapes) so I'm pretty happy with it.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 1 - Move ShapeCachePtr from BaseShape to Shape. r?jonco!

This adds a pointer to Shape, but it lets us remove owned BaseShapes in the next patch.

Accessing the ShapeTable/ShapeIC is now a bit more efficient because it removes a
level of indirection. Creating the ShapeTable/ShapeIC is also faster because we no
longer need to create a new (owned) BaseShape.

Because dictionary objects no longer have owned BaseShapes at this point, the patch
has to change BaseShape::adoptUnowned calls to Shape::setBase to prevent assertion
failures.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 2 - Remove owned BaseShapes. r?jonco!

At this point all BaseShapes are unowned (= immutable, shared) so we can remove the
UnownedBaseShape type and the BaseShape::unowned_ field.

Simplify NextEnvironmentShape by reusing the previous shape's BaseShape instead
of doing a slow lookup resulting in the same BaseShape.

Depends on D106971

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 3 - Simplify NativeObject::removeProperty a bit. r?jonco!

The BaseShape::get call is redundant. Reusing the BaseShape from the previous
last-property is equivalent and infallible, and lets us simplify the code a bit.

Depends on D106972

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 4 - Move ObjectFlags from BaseShape to Shape. r?jonco!

Depends on D106973

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 5 - Remove JSClass* from ObjectGroup. r?jonco!

BaseShape stores the JSClass* too so this patch changes the code to get the JSClass*
from there instead of from the ObjectGroup.

Depends on D106974

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 6 - Move Realm* from ObjectGroup to BaseShape. r?jonco!

Depends on D106975

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 7 - Avoid GCMarker compartment failure for shapes in IC stubs. r?jonco!

Now that shapes have an associated realm/compartment, we could fail the tracingCompartment
assertion for same-zone but different-compartment shapes stored in IC stubs for
cross-compartment wrapper stubs.

This adds TraceSameZoneCrossCompartmentEdge to avoid that.

Also add an assertion to CacheIRWriter to ensure we don't embed cross-zone shapes.

Depends on D106976

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 8 - Remove shadow::ObjectGroup. r?jonco!

Depends on D106977

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 9 - Move proto from ObjectGroup to BaseShape. r?jonco!

The ObjectGroup still stores the proto as well but we don't really use it anymore.
This is simpler than removing the group's proto now because that would result in
"empty" groups and the group table then doesn't make sense anymore.

Depends on D106978

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 10 - Stop passing proto in a few places now that we can get it from the shape. r?jonco!

Depends on D106979

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 11 - Update comments and JIT guards now that shape always implies proto. r?tcampbell!

Shape implying proto lets us simplify CacheIR guards. The UncacheableProto flag
is now only used to invalidate and disable the shape teleporting optimization.

Depends on D106980

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 12 - Remove GuardObjectGroup CacheIR and MIR instructions. r?tcampbell!

Depends on D106981

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 13 - Remove JIT pre-barrier code for object groups. r?tcampbell!

Depends on D106982

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 14 - Set the Delegate flag in EmptyShape::getInitialShape if needed. r?tcampbell!

We were relying on this flag being set in ObjectGroup::defaultNewGroup, but that's
going away soon.

Depends on D106983

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 15 - Initialize JSObject group to nullptr everywhere. r?tcampbell!

This effectively removes groups from objects, but the actual object shrinking happens
in the next patch.

Depends on D106984

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 16 - Remove JSObject group field. r?tcampbell!,jonco!

Note that on 32-bit platforms, JSObject is still 8 bytes. This means we don't have
to worry about misaligned DoubleValue stores to fixed slots on ARM32/MIPS32.

Depends on D106985

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 17 - Remove ObjectGroup. r?jonco!

Depends on D106986

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Bug 1689413 part 18 - Fix GDB pretty printer for JSObject. r?tcampbell!

Depends on D106987

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76db3b04dedd
part 1 - Move ShapeCachePtr from BaseShape to Shape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/6f7514b0a657
part 2 - Remove owned BaseShapes. r=jonco
https://hg.mozilla.org/integration/autoland/rev/211c4f80a286
part 3 - Simplify NativeObject::removeProperty a bit. r=jonco
https://hg.mozilla.org/integration/autoland/rev/f1bb16079c0e
part 4 - Move ObjectFlags from BaseShape to Shape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/4531d5c25694
part 5 - Remove JSClass* from ObjectGroup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/22854b710fbf
part 6 - Move Realm* from ObjectGroup to BaseShape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/80d594d926ad
part 7 - Avoid GCMarker compartment failure for shapes in IC stubs. r=jonco
https://hg.mozilla.org/integration/autoland/rev/95fa0ec36f7a
part 8 - Remove shadow::ObjectGroup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/e15f1a6f5a38
part 9 - Move proto from ObjectGroup to BaseShape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/5595064ffb60
part 10 - Stop passing proto in a few places now that we can get it from the shape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/558e0b0af7b7
part 11 - Update comments and JIT guards now that shape always implies proto. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/63eb8f93c6d8
part 12 - Remove GuardObjectGroup CacheIR and MIR instructions. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/ed3ef7b5c8c8
part 13 - Remove JIT pre-barrier code for object groups. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/8262ed245e27
part 14 - Set the Delegate flag in EmptyShape::getInitialShape if needed. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/d1411da6c254
part 15 - Initialize JSObject group to nullptr everywhere. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/a11a96dce11b
part 16 - Remove JSObject group field. r=tcampbell,jonco
https://hg.mozilla.org/integration/autoland/rev/4d4d052bf8d3
part 17 - Remove ObjectGroup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/2fe249306030
part 18 - Fix GDB pretty printer for JSObject. r=tcampbell

Comment 24

[Narcis Beleuzu [:NarcisB]]

Backed out for wpt failures on Event-subclasses-constructors.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/be83e8dbc365e0c03dd57e2dca49dfde1be07843
Log link: https://treeherder.mozilla.org/logviewer?job_id=332237591&repo=autoland&lineNumber=6501

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Narcis Beleuzu [:NarcisB] from comment #24)

Backed out for wpt failures on Event-subclasses-constructors.html

It's annoying that mach try auto didn't run that test, but it's a good find. This is an issue with InitializePropertiesFromCompatibleNativeObject, an API that's only used in the browser.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ae4c30bb931
part 1 - Move ShapeCachePtr from BaseShape to Shape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/e9c2f256240a
part 2 - Remove owned BaseShapes. r=jonco
https://hg.mozilla.org/integration/autoland/rev/d5d3cb487914
part 3 - Simplify NativeObject::removeProperty a bit. r=jonco
https://hg.mozilla.org/integration/autoland/rev/2186bd51bebd
part 4 - Move ObjectFlags from BaseShape to Shape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/9bfa6023a553
part 5 - Remove JSClass* from ObjectGroup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/11fc8f557a2a
part 6 - Move Realm* from ObjectGroup to BaseShape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/1eb43e6ab1f8
part 7 - Avoid GCMarker compartment failure for shapes in IC stubs. r=jonco
https://hg.mozilla.org/integration/autoland/rev/cad616508f73
part 8 - Remove shadow::ObjectGroup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/ec72f104edbf
part 9 - Move proto from ObjectGroup to BaseShape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/40d2e0691861
part 10 - Stop passing proto in a few places now that we can get it from the shape. r=jonco
https://hg.mozilla.org/integration/autoland/rev/8199e30ca52f
part 11 - Update comments and JIT guards now that shape always implies proto. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/b91b9804031d
part 12 - Remove GuardObjectGroup CacheIR and MIR instructions. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/5986697cbefa
part 13 - Remove JIT pre-barrier code for object groups. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/503f3d7f7162
part 14 - Set the Delegate flag in EmptyShape::getInitialShape if needed. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/483a1682c2fa
part 15 - Initialize JSObject group to nullptr everywhere. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/95cc69c76f0e
part 16 - Remove JSObject group field. r=tcampbell,jonco
https://hg.mozilla.org/integration/autoland/rev/88d76a5c5958
part 17 - Remove ObjectGroup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/4197952997ba
part 18 - Fix GDB pretty printer for JSObject. r=tcampbell

Comment 27

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1ae4c30bb931
https://hg.mozilla.org/mozilla-central/rev/e9c2f256240a
https://hg.mozilla.org/mozilla-central/rev/d5d3cb487914
https://hg.mozilla.org/mozilla-central/rev/2186bd51bebd
https://hg.mozilla.org/mozilla-central/rev/9bfa6023a553
https://hg.mozilla.org/mozilla-central/rev/11fc8f557a2a
https://hg.mozilla.org/mozilla-central/rev/1eb43e6ab1f8
https://hg.mozilla.org/mozilla-central/rev/cad616508f73
https://hg.mozilla.org/mozilla-central/rev/ec72f104edbf
https://hg.mozilla.org/mozilla-central/rev/40d2e0691861
https://hg.mozilla.org/mozilla-central/rev/8199e30ca52f
https://hg.mozilla.org/mozilla-central/rev/b91b9804031d
https://hg.mozilla.org/mozilla-central/rev/5986697cbefa
https://hg.mozilla.org/mozilla-central/rev/503f3d7f7162
https://hg.mozilla.org/mozilla-central/rev/483a1682c2fa
https://hg.mozilla.org/mozilla-central/rev/95cc69c76f0e
https://hg.mozilla.org/mozilla-central/rev/88d76a5c5958
https://hg.mozilla.org/mozilla-central/rev/4197952997ba

Comment 28

[Andra Esanu (needinfo me)]

== Change summary for alert #29147 (as of Tue, 09 Mar 2021 13:35:56 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
3%	JS		linux1804-64-shippable-qr	tp6	171,553,938.45 -> 165,988,948.55
3%	JS		macosx1015-64-shippable-qr	tp6	173,287,247.03 -> 168,843,936.09
2%	JS		linux1804-64-shippable	tp6	169,816,956.09 -> 165,937,167.86
2%	JS		windows10-64-shippable-qr	tp6	172,547,939.34 -> 168,766,500.09
2%	JS		windows10-64-shippable		89,135,713.16 -> 87,319,923.96
2%	JS		windows10-64-shippable		89,073,843.86 -> 87,372,224.82
1%	Base Content JS		macosx1015-64-shippable-qr		2,517,759.33 -> 2,480,250.67
1%	Base Content JS		linux1804-64-shippable-qr		2,510,974.67 -> 2,474,522.67
1%	Base Content JS		linux1804-64-shippable		2,509,538.00 -> 2,474,546.67
1%	Base Content JS		windows10-64-shippable		2,513,248.00 -> 2,478,816.00
1%	Base Content JS		windows10-64-shippable-qr		2,513,248.00 -> 2,478,816.00

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=29147

Comment 29

[Ionuț Goldan [:igoldan]]

== Change summary for alert #29132 (as of Mon, 08 Mar 2021 21:16:36 GMT) ==

Improvements:

Ratio	Suite	Test	Platform	Options	Absolute values (old vs new)
2%	netflix	loadtime	linux64-shippable-qr	cold nocondprof webrender	765.21 -> 749.58

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=29132