Closed Bug 1448838 Opened Last year Closed Last year

Add native version of CopyDataProperties

Categories

(Core :: JavaScript Engine, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: anba, Assigned: anba)

Details

Attachments

(1 file, 1 obsolete file)

Add native code to optimise object spread/rest similar to the optimisations for Object.assign() in bug 1364854.
Attached patch bug1448838.patch (obsolete) — Splinter Review
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.
Attachment #8962375 - Flags: review?(jdemooij)
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();
---
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);
}
Priority: -- → P2
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.
Flags: needinfo?(andrebargull)
(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!
Flags: needinfo?(andrebargull)
Attached patch bug1448838.patchSplinter Review
Updated patch to include excludedItems check for the unboxed version per comment #4.
Attachment #8962375 - Attachment is obsolete: true
Attachment #8962375 - Flags: review?(jdemooij)
Attachment #8962880 - Flags: review?(jdemooij)
Comment on attachment 8962880 [details] [diff] [review]
bug1448838.patch

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

Great speedup, thanks!
Attachment #8962880 - Flags: review?(jdemooij) → review+
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/23c2abe70e6c
Add native version for CopyDataProperties. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/23c2abe70e6c
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.