Bug 1700052 Comment 0 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

Getters/setters are currently stored in the shape tree, in `AccessorShape`s. 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. 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).
```js
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);
```
Getters/setters are currently stored in the shape tree, in `AccessorShape`s. 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).
```js
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);
```

Back to Bug 1700052 Comment 0