Optimize ArgumentsObject allocation

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug, {perf})

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

2 years ago
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

2 years ago
Created attachment 8752461 [details] [diff] [review]
Part 1 - Move deletedBits into RareArgumentsData
Attachment #8752461 - Flags: review?(luke)
(Assignee)

Comment 2

2 years ago
Created attachment 8752462 [details] [diff] [review]
Part 2 - Move ArgumentsData::callee to a reserved slot

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

2 years ago
Created attachment 8752463 [details] [diff] [review]
Part 3 - Remove JS_OVERWRITTEN_CALLEE

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

2 years ago
Created attachment 8752464 [details] [diff] [review]
Part 4 - rm ArgumentsData::{script,dataBytes}

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

2 years ago
Created attachment 8752468 [details] [diff] [review]
Part 5 - Allocate from JIT code

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

2 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

2 years ago
Attachment #8752462 - Flags: review?(luke) → review+

Updated

2 years ago
Attachment #8752463 - Flags: review?(luke) → review+

Updated

2 years ago
Attachment #8752464 - Flags: review?(luke) → review+

Comment 7

2 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

2 years ago
Should this get the "perf" keyword?
(Assignee)

Updated

2 years ago
Keywords: perf

Comment 9

2 years ago
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
Depends on: 1280252
Depends on: 1297142
Depends on: 1308346
(Assignee)

Updated

2 years ago
No longer depends on: 1308346
You need to log in before you can comment on or make changes to this bug.