Closed Bug 1484370 Opened 2 years ago Closed 2 years ago

Optimize native property iteration for structured cloning

Categories

(Core :: JavaScript Engine, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached 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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.