Optimize Object.assign

RESOLVED FIXED in Firefox 56
(NeedInfo from)

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem, NeedInfo)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
Blocks: 1300848

Updated

2 years ago
Whiteboard: [qf]
There is a good chance this will improve Speedometer v2 perf among other things.
Whiteboard: [qf] → [qf:p1]
Blocks: 1307395
Depends on: 1364908
No longer blocks: 1366777
Kannan: MEASURE THIS (Object.assign).
Flags: needinfo?(kvijayan)
(Assignee)

Comment 4

2 years ago
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 5

2 years ago
Created attachment 8879497 [details] [diff] [review]
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).
Attachment #8879497 - Flags: review?(evilpies)
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.
Attachment #8879497 - Flags: review?(evilpies) → review+
(Assignee)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

2 years ago
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

2 years ago
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
(Assignee)

Comment 10

2 years ago
Created attachment 8881293 [details] [diff] [review]
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.
Attachment #8879497 - Attachment is obsolete: true
Attachment #8881293 - Flags: review?(evilpies)
Nit: For spec-compliance, symbol-valued properties need to be assigned after all string-valued properties.
(Assignee)

Comment 12

2 years ago
Created attachment 8881335 [details] [diff] [review]
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.
Attachment #8881293 - Attachment is obsolete: true
Attachment #8881293 - Flags: review?(evilpies)
Attachment #8881335 - Flags: review?(evilpies)
Attachment #8881335 - Flags: review?(evilpies) → review+

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f165f830468d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.