1700052 - Move getters/setters from AccessorShape to object slots

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

Getters/setters are currently stored in the shape tree, in AccessorShapes. The reasons for doing it this way were mostly historical, as far as I know: we already supported JSGetterOp and JSSetterOp getters/setters and that code was reused when implementing ES5 getter/setter objects.

There are a number of problems with this:

• Branchy shape trees when lambdas are used as getters/setters can result in performance cliffs and wasting memory. See the micro-benchmark below for an example of this.
• It complicates the shape code because data properties and accessor properties are so different.
• Certain changes to the object layout we're considering are not possible.

The plan here is as follows:

1. Remove JSGetterOp/JSSetterOp (bug 1404885). Today these are only used for array.length and ArgumentsObject, and I think we can handle these special cases with a special property attribute instead.

2. Add a GetterSetterPair GC thing, a tuple holding just the getter/setter objects. Use this in AccessorShape.

3. Store GetterSetterPair in an object slot as PrivateGCThingValue. Remove AccessorShape.

This will require an extra CacheIR guard for getter/setter calls. This is likely acceptable compared to the call overhead, and this could be optimized away in the future if needed (since most getters/setters are on prototype objects, we could use fuses to invalidate code when redefining an accessor property).

---

Here's a micro-benchmark where a getter property makes accessing a data property much slower (28 ms => 204 ms).

function init() {
	var objs = [];
	for (var i = 0; i < 128; i++) {
		objs.push({get foo() {}, x: 1});
	}
	return objs;
}
function test(objs) {
	var res = 0;
	for (var i = 0; i < 10_000_000; i++) {
		res += objs[i % 128].x;
	}
	return res;
}
var objs = init();
var t = dateNow();
var res = test(objs);
print(dateNow() - t);
print(res);

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 1 - Add methods to NativeObject for accessing getter/setter objects. r?jonco!

For now these just forward to the equivalent methods on the shape, but that
will change in a later patch.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 2 - Check getter object identity in ArraySpeciesLookup and PromiseLookup. r?jonco!

This way the code will do the right thing when the shape no longer implies a specific
getter object.

A later patch will change this so we store just the slot number instead of the
species property shape.

Depends on D110248

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 3 - Use isDataProperty instead of !isAccessorShape in a few places. r?jonco!

Depends on D110249

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 4 - Simplify and optimize AddOrChangeProperty a bit. r?jonco!

This is preparing for a later patch.

Depends on D110250

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 5 - Add movePropertyKey to the MacroAssembler. r?iain!

Also clean up the similar Push method a bit to use PropertyKey instead of jsid.

Depends on D110251

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 6 - Pass jsid to ObjectHasGetterSetterPure. r?iain!

We currently get the jsid from the property's shape, but in a later patch that
Shape* argument will be replaced with a GetterSetter* argument.

I considered using PropertyKey instead of jsid, but this matches similar code
using jsid.

Depends on D110252

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 7 - Add branchTestValue overload to the MacroAssembler. r?iain!

This turned out to be a bit more work than expected because we were missing
branch32/branchPtr implementations taking a BaseIndex and a Register.

Depends on D110253

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 8 - Add GuardFixedSlotValue and GuardDynamicSlotValue CacheIR instructions. r?iain!

These will be used in a later patch to guard obj[slot] == PrivateGCThingValue(GetterSetter*).

Depends on D110254

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 9 - Add js::GetterSetter GC thing. r?jonco!

Depends on D110255

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 10 - Add support for GetterSetter stub fields. r?iain!

Depends on D110256

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 11 - Store getter/setter objects in slots instead of in the shape tree. r?jonco!

There will be additional cleanup of the Shape code in follow-up patches.

Depends on D110257

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 12 - Remove more AccessorShape code. r?jonco!

Depends on D110258

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 13 - Simplify and rename some functions that are now only used for custom data properties. r?jonco!

Depends on D110259

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 14 - Rename DataProperty to Property in a few places. r?jonco!

Because these methods are now also used for accessor properties.

Depends on D110260

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 15 - Use slot number instead of property shape for ArraySpeciesLookup and PromiseLookup. r?jonco!

This is a bit faster than getting the slot number from the shape every time.

Depends on D110261

Comment 16

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 16 - Add tests for accessor properties. r?jonco!

Depends on D110262

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3af530ddacc9
part 1 - Add methods to NativeObject for accessing getter/setter objects. r=jonco
https://hg.mozilla.org/integration/autoland/rev/d5eb51b405c4
part 2 - Check getter object identity in ArraySpeciesLookup and PromiseLookup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/56f4a105ca52
part 3 - Use isDataProperty instead of !isAccessorShape in a few places. r=jonco
https://hg.mozilla.org/integration/autoland/rev/a4d18ab82fef
part 4 - Simplify and optimize AddOrChangeProperty a bit. r=jonco
https://hg.mozilla.org/integration/autoland/rev/b3219fb7aa9b
part 5 - Add movePropertyKey to the MacroAssembler. r=iain

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Bug 1700052 part 17 - Add Shape::hasSlot. r?jonco!

Use this instead of checking !isCustomDataProperty (and before this stack, !isDataProperty).

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/3af530ddacc9
https://hg.mozilla.org/mozilla-central/rev/d5eb51b405c4
https://hg.mozilla.org/mozilla-central/rev/56f4a105ca52
https://hg.mozilla.org/mozilla-central/rev/a4d18ab82fef
https://hg.mozilla.org/mozilla-central/rev/b3219fb7aa9b

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8540f6f82ab6
part 6 - Pass jsid to ObjectHasGetterSetterPure. r=iain
https://hg.mozilla.org/integration/autoland/rev/bf62a6aa5aa4
part 7 - Add branchTestValue overload to the MacroAssembler. r=iain
https://hg.mozilla.org/integration/autoland/rev/a91c9a837ba6
part 8 - Add GuardFixedSlotValue and GuardDynamicSlotValue CacheIR instructions. r=iain
https://hg.mozilla.org/integration/autoland/rev/fc6b5fb92749
part 9 - Add js::GetterSetter GC thing. r=jonco
https://hg.mozilla.org/integration/autoland/rev/7e3a27b38e56
part 10 - Add support for GetterSetter stub fields. r=iain

Comment 21

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/8540f6f82ab6
https://hg.mozilla.org/mozilla-central/rev/bf62a6aa5aa4
https://hg.mozilla.org/mozilla-central/rev/a91c9a837ba6
https://hg.mozilla.org/mozilla-central/rev/fc6b5fb92749
https://hg.mozilla.org/mozilla-central/rev/7e3a27b38e56

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4825995d926
part 11 - Store getter/setter objects in slots instead of in the shape tree. r=jonco,evilpie
https://hg.mozilla.org/integration/autoland/rev/40b1b6f502ca
part 12 - Remove more AccessorShape code. r=jonco
https://hg.mozilla.org/integration/autoland/rev/1bee1c7dabac
part 13 - Simplify and rename some functions that are now only used for custom data properties. r=jonco
https://hg.mozilla.org/integration/autoland/rev/89d1c5483930
part 14 - Rename DataProperty to Property in a few places. r=jonco
https://hg.mozilla.org/integration/autoland/rev/3ad3acac6cf6
part 15 - Use slot number instead of property shape for ArraySpeciesLookup and PromiseLookup. r=jonco
https://hg.mozilla.org/integration/autoland/rev/55012db38df0
part 16 - Add tests for accessor properties. r=jonco
https://hg.mozilla.org/integration/autoland/rev/b00dca6a4995
part 17 - Add Shape::hasSlot. r=jonco

Comment 23

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/c4825995d926
https://hg.mozilla.org/mozilla-central/rev/40b1b6f502ca
https://hg.mozilla.org/mozilla-central/rev/1bee1c7dabac
https://hg.mozilla.org/mozilla-central/rev/89d1c5483930
https://hg.mozilla.org/mozilla-central/rev/3ad3acac6cf6
https://hg.mozilla.org/mozilla-central/rev/55012db38df0
https://hg.mozilla.org/mozilla-central/rev/b00dca6a4995