1364854 - Optimize Object.assign

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

I was looking at a React-Redux benchmark and we were spending quite a lot of time in Object.assign. There are some problems with our self-hosted Object.assign:

* Ion is unable to inline functions that use arguments[x].

* The SetElem to assign the property is highly polymorphic.

* The OwnPropertyKeys and std_Object_propertyIsEnumerable calls also add overhead.

We should gather some data on how Object.assign is typically used in the wild. In many cases we can probably get away with a C++ intrinsic to optimize the common cases (no getters/setters/proxies involved) by copying the shapes or something.

Comment 1

[Naveed Ihsanullah [:naveed]]

There is a good chance this will improve Speedometer v2 perf among other things.

Comment 2

[Kannan Vijayan [:djvj]]

Kannan: MEASURE THIS (Object.assign).

Comment 3

[André Bargull [:anba]]

V8: https://github.com/v8/v8/blob/05b9778d12f5fcdbdbc89a347fc9ab7916da9fb0/src/objects.cc#L1991-L2082
Chakra: https://github.com/Microsoft/ChakraCore/blob/d71f43e56f80d0188f37850cf2657d1f3a841c36/lib/Runtime/Library/JavascriptObject.cpp#L1513-L1547

Maybe we can use a similar solution like V8 now that bug 1368626 has landed?

Comment 4

[Jan de Mooij [:jandem]]

Even a naive implementation in C++ seems to be a win on react-redux. It does regress six-speed's assign test (1.8x or so), but that's just because ICs work well for that particular micro-benchmark. A fast path will hopefully fix most of that.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Port Object.assign to C++

This reimplements Object.assign in C++. Locally it's a small win on Speedometer's React-redux and (maybe) Inferno tests. We also allocate less garbage now (the self-hosted code had to allocate an array each time) which should reduce GC pressure a bit.

I added a fast path for when we assign a native object to another native object. We get a list of |from| shapes, and as long as |from|'s shape does not change, we can fast path the enumerability check and lookup.

With the fast path, the slowdown on six-speed's Object.assign test should be less than 1.5x. There's more we can squeeze out here, in particular the property set is still slower than it needs to be, but we can do that later.

I also added a lot of tests for various edge cases (all tests passed when I intentionally broke some of this stuff).

Comment 6

[Tom S [:evilpie]]

Comment on attachment 8879497 [details] [diff] [review]
Port Object.assign to C++

Review of attachment 8879497 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work, sadly a lot more complicated than it would seem at first. Thanks for the tests they really made it easier to understand. The perf seems unavoidable, but JSC added a bunch of code to match us on the AWFY six-speed benchmark ;)

Anyway I had this idea about using GetPropertyKeys on "to" (only when unobservable of course) and then every time we want to use Set, we can instead use DefineProperty (AddProperty?) when it hasn't been seen yet.

::: js/src/builtin/Object.cpp
@@ +671,5 @@
> +                continue;
> +            propValue = from->getSlot(shape->slot());
> +        } else {
> +            // |from| changed shape or the property is not a data property, so
> +            // we have to do the slower enumerability check and GetProp.

So, what can happen is the object's shape totally changes, which implies the old "enumerability" bit and such is completely stale, right?

Just the "property names" are still the old ones we should use. A lot of complexity here.

::: js/src/jit-test/tests/basic/object-assign.js
@@ +28,5 @@
> +    // Property is deleted. Should NOT get Object.prototype.toString.
> +    from = {x: 1, toString: 2};
> +    to = {set x(v) { delete from.toString; }};
> +    Object.assign(to, from);
> +    assertEq(to.hasOwnProperty("toString"), false);

Just copy this test with x/toString reversed in order, so we don't only test deleting the last property.

Comment 7

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #6)
> Anyway I had this idea about using GetPropertyKeys on "to" (only when
> unobservable of course) and then every time we want to use Set, we can
> instead use DefineProperty (AddProperty?) when it hasn't been seen yet.

We just have to be careful to not add more overhead than the naive version (Object.prototype has many properties). What we could do is look for getter/setter properties specifically... And maybe give up the search when we find more than a few properties (some DOM objects have a lot of stuff on the prototype).

Initially I wanted to add a "has getter/setter or other weirdness" BaseShape flag (it's pretty simple to do now), but that doesn't work well because __proto__ is an accessor, of course :( We could hack around it but it's annoying.

> So, what can happen is the object's shape totally changes, which implies the
> old "enumerability" bit and such is completely stale, right?

Yep.

> Just copy this test with x/toString reversed in order, so we don't only test
> deleting the last property.

Sure.

Comment 8

[Jan de Mooij [:jandem]]

The tests I added caught an issue with unboxed objects - it's possible to call a getter/setter that turns the native objects into unboxed objects :( Should be easy to fix at least.

Comment 9

[Tim Nguyen :ntim]

Webkit & blink seem to do ~5 million operations/sec, while Gecko only do 2 million ops/sec on Object.assign().

https://jsperf.com/object-assign-vs-object-spread

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch v2

This patch ensures we don't assume the objects are native inside the loop. We can optimize this more later but this is the simplest fix for now.

Comment 11

[André Bargull [:anba]]

Nit: For spec-compliance, symbol-valued properties need to be assigned after all string-valued properties.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Patch v3

Thanks André, good catch. This version falls back to the slow path if we see a symbol property. I also added a test for this that passes now.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f165f830468d
Port Object.assign to C++. r=evilpie

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f165f830468d