Closed Bug 1411774 Opened 3 years ago Closed 3 years ago

Optimize Object.assign with unboxed objects

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch WIP (obsolete) — Splinter Review
It's relatively like and it does happen on the web that code like Object.assign(obj, {a: 1, b: 3}) will turn the object literal into an unboxed object. This current always takes a slow path.

In my patch for this I am not sure if we need to copy the layout.properties(), because those might change during execution?

This makes unboxed objects as a source about as fast as native objects, maybe a bit slower.
> In my patch for this I am not sure if we need to copy the layout.properties(), because those might change during execution?
Brian can you answer this for me? Thanks
Flags: needinfo?(bhackett1024)
(In reply to Tom Schuster [:evilpie] from comment #0)
> In my patch for this I am not sure if we need to copy the
> layout.properties(), because those might change during execution?

The properties in a group's unboxed layout do not change after construction.
Flags: needinfo?(bhackett1024)
Thanks Brian. I was actually wondering what keeps the UnboxedLayout alive, but I didn't phrase that well. During the iteration we might change from unboxed to native.
Attachment #8922101 - Attachment is obsolete: true
Attachment #8923553 - Flags: review?(jdemooij)
(In reply to Tom Schuster [:evilpie] from comment #3)
> Created attachment 8923553 [details] [diff] [review]
> Optimize Object.assign from an unboxed object
> 
> Thanks Brian. I was actually wondering what keeps the UnboxedLayout alive,
> but I didn't phrase that well. During the iteration we might change from
> unboxed to native.

Well, the unboxed layout is owned by / stored in the object group which refers to it, and since that group is rooted in the patch things should be fine.
Priority: -- → P3
Comment on attachment 8923553 [details] [diff] [review]
Optimize Object.assign from an unboxed object

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

\o/ very nice to have this fixed!
Attachment #8923553 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cbf1345efd1
Optimize Object.assign with unboxed objects. r=jandem
Backed out for failing spidermonkey non-unified at js/src/vm/UnboxedObject.h:263:33: inline function 'const js::UnboxedLayout& js::UnboxedPlainObject::layout() const' used but never defined:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4154937d7b4e4d217f37cb6d06fd51e5012cbe84

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8cbf1345efd1e9fb1f6a729af5a115431fcd4c15&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=retry&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=141112011&repo=mozilla-inbound
> js/src/vm/UnboxedObject.h:263:33: error: inline function 'const js::UnboxedLayout& js::UnboxedPlainObject::layout() const' used but never defined [-Werror]
Flags: needinfo?(evilpies)
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aaa2d20e7aa
Optimize Object.assign with unboxed objects. r=jandem
Flags: needinfo?(evilpies)
https://hg.mozilla.org/mozilla-central/rev/2aaa2d20e7aa
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.