Closed
Bug 1364854
Opened 8 years ago
Closed 8 years ago
Optimize Object.assign
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 3 open bugs)
Details
Attachments
(1 file, 2 obsolete files)
12.52 KB,
patch
|
evilpies
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•8 years ago
|
Whiteboard: [qf]
Comment 1•8 years ago
|
||
There is a good chance this will improve Speedometer v2 perf among other things.
Whiteboard: [qf] → [qf:p1]
Updated•8 years ago
|
Blocks: Speedometer_V2
Updated•8 years ago
|
Blocks: TimeToFirstPaint_FB
Updated•8 years ago
|
No longer blocks: TimeToFirstPaint_FB
Comment 3•8 years ago
|
||
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?
Assignee | ||
Comment 4•8 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•8 years ago
|
||
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•8 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•8 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•8 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•8 years ago
|
||
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)
Comment 11•8 years ago
|
||
Nit: For spec-compliance, symbol-valued properties need to be assigned after all string-valued properties.
Assignee | ||
Comment 12•8 years ago
|
||
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 13•8 years ago
|
||
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f165f830468d
Port Object.assign to C++. r=evilpie
Comment 14•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•6 years ago
|
Flags: needinfo?(kvijayan)
Updated•3 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•