1616032 - Object.defineProperty getters force unique shapes

Status: RESOLVED DUPLICATE of bug 1700052

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[mread]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.100 Safari/537.36

Steps to reproduce:

When retrieving property values using Object.defineProperty with a getter function performance is poor.

The test case https://jsbench.me/igk6ma7zkl/1 shows various ways of getting a property value. Using a getter with Object.defineProperty is dramatically slower than all other methods.

Other browsers are also slower when using Object.defineProperty getters, but not as relatively slow. Here are some results on my machine:

Firefox 73: 100x slower than regular property access
Chrome 80: 20x slower than regular property access
Safari 13.0.5: 10x slower than regular property access

If the getter is defined on the object's prototype rather than the instance, then performance is comparable with regular property access. However if the property is defined and accessed on multiple different prototypes (eg. 10), then the same poor performance occurs, as shown in this test case: https://jsbench.me/6rk6qe48y7/1

Comment 1

[Nicolas B. Pierron [:nbp]]

I can reproduce this issue on Nightly ARM64. And apparently a regular property access is slower than accessing the same property through a getter.
My blind guess would be that we are adding ICs for each object instead of doing a shape guard.

Testing with Ion disabled show slow speed for all cases, but the regular property is slower than the regular getter, as expected.
This suggest that we are no longer able to optimize property accesses in IonMonkey, as we used to do before.

This definitely sounds like an important bug to fix.

Comment 2

[Nourdine]

Hi, I'd like to take care of this bug.

Comment 3

[Steven DeTar [:sdetar]]

Iain, any thoughts on this bug? Nicolas indicated this seems like a real bug/regression we should fix in comment 1. I saw Nicolas NI'd you so that was why I was asking you about it.

Comment 4

[Iain Ireland [:iain]]

Working through old needinfos. My first observation is that several of the examples here are broken and do not do what the author presumably intends.

This code is presumably intended to create an object with a getter:

  var regularGetter = function () {
    Object.assign(this, {
      get prop() { return this.prop_; }
    });
    this.prop_ = 1;
  }

However, Object.assign invokes getters on the source object, so the resulting object is {prop_: 1, prop: undefined}. Accessing prop on this object is obviously very fast, although it does not give the intended result.

Also, the following code is presumably intended to use an actual function call to get the value of prop:

var regularFunctionCall = function () {
    this.get = function(name) {
	return this[name];
    };
    this.prop = 1;
}

However, adding a property named get to an object does not do anything special. This code, too, ends up doing a normal property access. In fact, every test case except the Object.defineProperty case boils down to monomorphic slot access.

A more idiomatic way of creating an object with a getter would be something like:

class IdiomaticGetter {
  constructor() { this.prop_ = 1; }
  get prop() { return this.prop_; }
}

Testing this, it is roughly 2x slower than a normal property access, which seems completely acceptable. The difference between that case and the Object.defineProperty case appears to be that we create a distinct getter object each time we call Object.defineProperty and, importantly, the difference in getter object means that each of these objects has a unique Shape. (The rawGetter field of the descriptor is initialized to point to the object here, called from intrinsic_DefineProperty. It is eventually used as a hash key here.)

My instinct is that we mostly don't care about this case. I doubt that defining getters this way is common in the wild. If that turns out to be wrong, we could maybe look into modifying shape lookup (perhaps by hashing on the script, not the function object).

Comment 5

[mread]

Apologies for the broken examples. There is a new test case with those 2 broken cases fixed here: https://jsbench.me/fgkfljr09n/1.

To clarify, the other getter methods are only included as a comparison to the performance of Object.defineProperty.

Using Object.defineProperty appears to be the slowest method in all tested JavaScript engines, but in SpiderMonkey the difference is the most pronounced, being ~100x slower than regular property access. From a JavaScript developer's perspective that degree of relative performance impact would be unexpected.

This bug was raised after identifying this as the cause of animation 'janks' in a client's UI application. The client was making heavy use of 'Object.defineProperty' to implement getters/setters in their low level framework code, and it was recommended not to use them based on the observed performance cost. I'm not sure how common this problem would be in general, but it since it is a possible way to implement a getter it is likely that people may choose to use it as their default design pattern.

Searching the internet for "Object.defineProperty performance" shows a few references to the problem, though not specific to SpiderMonkey, such as:
https://github.com/mrdoob/three.js/issues/9536

Comment 6

[Iain Ireland [:iain]]

Code that cares about performance should avoid this pattern. Even if we made it so these objects all got the same shape, it would still be better to put the getter on the prototype: you only need to store one copy of the getter that is shared by all objects, instead of one copy per object.

Fixing this is also not trivial: we have to audit all the places we do shape guards to make sure that none of them depend on the current behaviour.

We are tentatively planning to do some work on our object model (shapes, groups, etc) next year. It's possible that this could be fixed as part of that work. In the meantime, though, my advice is to rewrite code to avoid this problem. (Being 10-20x slower is not good either.)

Comment 7

[André Bargull [:anba]]

Fixed through bug 1700052.