Optimize native property iteration for structured cloning

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: evilpie, Assigned: evilpie)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch WIP (obsolete) — Splinter Review
Reusing the ideas from Object.assign and various Object.values methods we can hand write an optimized iteration function that reduces intermediate allocations.

With all the patches including this one applied a simple array heavy serialization benchmark that I have been using improves from 350ms to 240ms. Looking at a profile I don't immediately see anything stand out anymore except for probably unavoidable memory allocation.
Attachment #9002121 - Attachment is obsolete: true
Comment on attachment 9002253 [details] [diff] [review]
Optimize native property iteration for structured cloning. r?

I have the feeling test coverage for structured cloning is not all that great on the SpiderMonkey side. However considering how much Firefox uses it, this try push seems to be good enough: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51e5b94a1a654a8147d12128d401532c4d8cda0 (Only the translation stuff is a bit worrying, because it appears a few times).

In theory we don't even need to check for TypedArrayObjects etc. because we should only end up with PlainObject and ArrayObject here. Maybe I should just assert that.
Attachment #9002253 - Flags: review?(jdemooij)
Comment on attachment 9002253 [details] [diff] [review]
Optimize native property iteration for structured cloning. r?

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

Nice speedup.

::: js/src/jit-test/tests/structured-clone/roundtrip.js
@@ +15,5 @@
> +    {0: 0, 1: 0, 1000000: 0, 1000001: 1},
> +
> +    [],
> +    [0, 1, 2],
> +    [0, 15, 16],

Add some with holes:

[0, , , 1, 2],
[, 1],
[0,,],
[,,],

::: js/src/vm/StructuredClone.cpp
@@ +1387,5 @@
> +    HandleNativeObject nobj = obj.as<NativeObject>();
> +    if (nobj->isIndexed() ||
> +        nobj->is<TypedArrayObject>() ||
> +        nobj->getClass()->getNewEnumerate() ||
> +        nobj->getClass()->getEnumerate())

At some point we should think about abstracting this. Maybe some kind of FastNativePropertyIterator template, but this is fine for now.
Attachment #9002253 - Flags: review?(jdemooij) → review+
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/075bfbeee249
Optimize native property iteration for structured cloning. r=jandem
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/075bfbeee249
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.