Closed
Bug 1272598
Opened 8 years ago
Closed 8 years ago
Optimize ArgumentsObject allocation
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(5 files)
18.78 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.72 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.92 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
9.33 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
13.59 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
I've a set of patches to optimize the ArgumentsObject layout and then another patch to do the object allocation in JIT code (with a VM call for the ArgumentsData allocation and initialization). ArgumentsData currently has a deletedBits vector with a bit for each argument. That is necessary to handle |delete arguments[i]| correctly, but outside of test suites and fuzzers, deleting arguments is very uncommon. So one of these patches adds a separate RareArgumentsData class that holds the deleted bits, and we only allocate that the first time we delete an element. That + some other minor optimizations + JIT inlining improves the Foo1 benchmark in bug 1271473 from 95 ms to 34 ms, that's good enough for now.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8752461 -
Flags: review?(luke)
Assignee | ||
Comment 2•8 years ago
|
||
ArgumentsData::callee is a HeapValue. This patch moves the callee to a reserved slot on the ArgumentsObject. We had 1 unused slot so we might as well use it and shrink ArgumentsData a bit.
Attachment #8752462 -
Flags: review?(luke)
Assignee | ||
Comment 3•8 years ago
|
||
This patch removes the JS_OVERWRITTEN_CALLEE Magic value and instead uses a "callee overridden" bit to indicate .callee was overridden. Benefits are: (1) We no longer overwrite the callee anywhere, so the next patch can get rid of ArgumentsData::script and simply get the script from the callee. (2) It's more consistent with the similar bits we use for "overridden length", "overridden element", etc.
Attachment #8752463 -
Flags: review?(luke)
Assignee | ||
Comment 4•8 years ago
|
||
Removes the 'script' and 'dataBytes' fields from ArgumentsData. They're now easy to derive from other information.
Attachment #8752464 -
Flags: review?(luke)
Assignee | ||
Comment 5•8 years ago
|
||
Allocates ArgumentsObject in Ion code, with a VM call to allocate and initialize the ArgumentsData. Luke, if you're too busy or don't feel confident reviewing these patches, feel free to forward to someone else.
Attachment #8752468 -
Flags: review?(luke)
Comment 6•8 years ago
|
||
Comment on attachment 8752461 [details] [diff] [review] Part 1 - Move deletedBits into RareArgumentsData Review of attachment 8752461 [details] [diff] [review]: ----------------------------------------------------------------- Nice!
Attachment #8752461 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8752462 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8752463 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8752464 -
Flags: review?(luke) → review+
Comment 7•8 years ago
|
||
Comment on attachment 8752468 [details] [diff] [review] Part 5 - Allocate from JIT code Review of attachment 8752468 [details] [diff] [review]: ----------------------------------------------------------------- Great work on these patches! ::: js/src/jit/MacroAssembler.cpp @@ +1014,5 @@ > if (nslots == 0) > return; > > + if (templateObj->is<ArgumentsObject>()) { > + // The caller is responsible for initializing the reserved slots. This is a bit implicit; how about another bool initReservedSlots (default true) so that way the caller can say this explicitly (and perhaps have a comment that the ABI call implies no GC is possible between alloc and init?
Attachment #8752468 -
Flags: review?(luke) → review+
Comment 8•8 years ago
|
||
Should this get the "perf" keyword?
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d4ed5fb7a3 part 1 - Move ArgumentsObject deleted bits into a new RareArgumentsData class. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/61ca55d24373 part 2 - Move ArgumentsData::callee to a reserved slot on the object. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/8efd0d392116 part 3 - Remove JS_OVERWRITTEN_CALLEE. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/c97757afd190 part 4 - Remove script and dataBytes from ArgumentsData. r=luke https://hg.mozilla.org/integration/mozilla-inbound/rev/772713cbce54 part 5 - Allocate arguments objects from Ion JIT code. r=luke
Comment 10•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d6d4ed5fb7a3 https://hg.mozilla.org/mozilla-central/rev/61ca55d24373 https://hg.mozilla.org/mozilla-central/rev/8efd0d392116 https://hg.mozilla.org/mozilla-central/rev/c97757afd190 https://hg.mozilla.org/mozilla-central/rev/772713cbce54
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•