Optimize native property iteration for structured cloning

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
9 months ago
9 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)

Assignee

Description

9 months ago
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.
Assignee

Updated

9 months ago
Attachment #9002121 - Attachment is obsolete: true
Assignee

Comment 2

9 months ago
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+

Comment 4

9 months ago
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

Comment 5

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/075bfbeee249
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.