1448838 - Add native version of CopyDataProperties

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[André Bargull [:anba]]

Add native code to optimise object spread/rest similar to the optimisations for Object.assign() in bug 1364854.

Comment 1

[André Bargull [:anba]]

Attachment: bug1448838.patch

Adds a native fast path for CopyDataProperties similar to the fast paths for |Object.assign(...)|. 

CopyDataProperties can only be called from object spread `({...source})` and object rest `var {...foo} = source` syntax, which means the target object wasn't yet exposed to user code and CopyDataProperties always creates own properties. These two characteristics allow CopyDataProperties to be more easily optimisable when compared to Object.assign(), for example setter accessors are never called and we can create another optimisation when the target object is initially empty.

I've also updated the variable names and step comments in CopyDataProperties to match the current draft version.

The test cases were derived from basic/object-assign.js and basic/object-assign-unboxed.js.

Comment 2

[André Bargull [:anba]]

Performance improvements for the following µ-benchmark:
---
// 0 properties:  165ms -> 60ms
// 1 property:    315ms -> 210ms
// 2 properties:  385ms -> 265ms
// 3 properties:  465ms -> 320ms
// 4 properties:  550ms -> 395ms
// 5 properties:  635ms -> 465ms
// 6 properties:  760ms -> 535ms
// 7 properties: 2400ms -> 600ms
function f() {
    var o = {a:0, b:0, c:0, d:0, e:0, f:0};
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        ({...o});
    }
    print(dateNow() - t);
}
f();
---

Comment 3

[André Bargull [:anba]]

I think it is even possible to further optimise the case when copying from PlainObject to PlainObject when the source PlainObject has no own properties by only setting lastProperty, similar to InitializePropertiesFromCompatibleNativeObject() and js::CopyInitializerObject(). But I haven't yet investigated which extra conditions need to be fulfilled to be able to simply share the lastProperty shape.


Other optimisations ideas for object rest/spread properties. (Not for this bug, just noting them here for documentation purposes :-)
- BytecodeEmitter::emitDestructuringObjRestExclusionSet() could use JSOP_OBJECT instead of JSOP_NEWINIT/JSOP_NEWOBJECT when the exclusion properties are known at compile-time.
- We could optimise the use case of trailing object spread properties like `({a:0, b:0, ...maybeMoreProps})` to emit JSOP_NEWOBJECT instead of JSOP_NEWINIT if we want to use unboxed objects when spread properties are present. That should help to reduce the performance difference between these two µ-benchmarks:

// Completes in 380ms.
function spread() {
    var o = {a:0, b:0};
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        ({x:0, ...o});
    }
    print(dateNow() - t);
}

// Completes in 285ms.
function assign() {
    var o = {a:0, b:0};
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) {
        Object.assign({x:0}, o);
    }
    print(dateNow() - t);
}

Comment 4

[Jan de Mooij [:jandem]]

Don't we need to check excludedItems in the unboxed-object version? If yes, can we add a test for it?

Asking before I finish the review to make sure I'm not missing something important.

Comment 5

[André Bargull [:anba]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Don't we need to check excludedItems in the unboxed-object version? If yes,
> can we add a test for it?
> 
> Asking before I finish the review to make sure I'm not missing something
> important.

We need to check for it, I just forgot to include it. I'll fix this and add a test to cover this path. Thanks for spotting that!

Comment 6

[André Bargull [:anba]]

Attachment: bug1448838.patch

Updated patch to include excludedItems check for the unboxed version per comment #4.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8962880 [details] [diff] [review]
bug1448838.patch

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

Great speedup, thanks!

Comment 8

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a512f2c1cfc6daddadcc34ea0207bcebd0a95f2

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c2abe70e6c
Add native version for CopyDataProperties. r=jandem

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/23c2abe70e6c