Closed
Bug 875276
Opened 11 years ago
Closed 11 years ago
Don't create types for scripts until they are compiled by baseline
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
People
(Reporter: bhackett1024, Assigned: bhackett1024)
References
Details
(Whiteboard: [MemShrink])
Attachments
(2 files)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
I tried to do this as part of bug 865059 but got hung up on dromaeo crashes I couldn't reproduce. Trying again, these seem to have been fixed by bug 869529 (https://tbpl.mozilla.org/?tree=Try&rev=f4ae3391da17).
Right now we always allocate script->types when a script first runs. This is an array of type sets for a script's arguments and property accessing bytecodes, and can consume a lot of memory --- 2% of js-main-runtime in my current session --- and can also be expensive to trace during GC. These type sets won't be used unless the code is Ion compiled, so it would be good if we allocated them less frequently. Allocating them when the script is baseline compiled is a good spot. The baseline compiler will allocate inline caches to fill in the type sets so they should exist then (though it may be worth followup to encode the type sets in the baseline stubs, at least until the code is Ion compiled), and per bug 678037 comment 33 we only expect about 20% of code that executes to get warm enough for baseline, so this spot should wipe out most of the memory used for script->types.
Attachment #753236 -
Flags: review?(jdemooij)
Comment 1•11 years ago
|
||
Comment on attachment 753236 [details] [diff] [review]
patch
Review of attachment 753236 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsobj.cpp
@@ +1522,5 @@
>
> if (res && cx->typeInferenceEnabled()) {
> JSScript *script = callee->toFunction()->nonLazyScript();
> + if (script->types)
> + TypeScript::SetThis(cx, script, types::Type::ObjectType(res));
Can we move the script->types check into TypeScript::SetThis, like you did for TypeMonitorResult and friends? r=me with that if possible.
Attachment #753236 -
Flags: review?(jdemooij) → review+
Updated•11 years ago
|
Whiteboard: [MemShrink]
Assignee | ||
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment 4•11 years ago
|
||
Caused serious regressions for
awfy-assorted: misc-basic-array (70%) and misc-basic-fpops (32%)
Comment 5•11 years ago
|
||
Backout due to the regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb75f6d6877
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 6•11 years ago
|
||
Sorry about this mess. After discussing it was decided I wrongly had backed out this changeset. So fixing up my mess. Sorry
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5a354146ebf
Assignee | ||
Comment 7•11 years ago
|
||
The arrayops regression was caused by not yet knowing the representation for an array until after the loop finished processing, which inhibited the array length fast path. Should probably just ban use of MDefinition::type() during IonBuilder as representations are not fixed until after the type analysis.
The misc-basic-fpops and misc-basic-intops regressions seem to be due to an Ion optimization for lowering commutative ops (ReorderCommutative) going haywire. In this code:
for (var i = 0; i < x; i++)
z += y;
The number of uses of 'y' has decreased and lowering reorders this as 'z = y + z' which causes the regalloc to generate more spill code. ReorderCommutative could look at whether it is in a loop, rather than the number of uses (it doesn't consider uses carried around backedges), but this seems a hack and incomplete as we still won't ever be able to reorder 'z = y + z' into 'z = z + y' as would be best for regalloc. This optimization needs a more in depth rewrite.
Attachment #754557 -
Flags: review?(jdemooij)
Comment 8•11 years ago
|
||
(In reply to Brian Hackett (:bhackett) from comment #7)
> The number of uses of 'y' has decreased and lowering reorders this as 'z = y
> + z' which causes the regalloc to generate more spill code.
> ReorderCommutative could look at whether it is in a loop, rather than the
> number of uses (it doesn't consider uses carried around backedges), but this
> seems a hack and incomplete as we still won't ever be able to reorder 'z = y
> + z' into 'z = z + y' as would be best for regalloc. This optimization
> needs a more in depth rewrite.
This optimization is rather new. But good to know we hit already at the boundaries of it. Maybe indeed we need a more elaborate pass. Would bug 875135 fix the issue here?
Just for bookkeeping, this bug caused the following regressions:
ss-base64: 15%
ss-cube: 9%
ss-nsieve: 20%
kraken-fft: 4%
misc-basic-array: 41%
misc-basic-fpops: 24.8%
misc-basic-intops: 9%
misc-bugs-508716-fluid-dynamics: 3.6%
misc-bugs-636096-model2d: 9.2%
misc-bugs-847389-jpeg2000: 6.7%
misc-corrections: 2.2%
The reason this wasn't a ss loss, is because of the ss-bitwise-and win
ss-bitwise-and: +200% (nice win!)
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Hannes Verschore [:h4writer] from comment #8)
> This optimization is rather new. But good to know we hit already at the
> boundaries of it. Maybe indeed we need a more elaborate pass. Would bug
> 875135 fix the issue here?
I don't think so, as neither operand here is a constant. It seems to me that doing this properly requires information about the live ranges of the inputs, which won't be available until we are in regalloc. So the regalloc could do this reordering as part of its computation, or maybe add a separate generic pass to insert in the middle of regalloc.
Comment 10•11 years ago
|
||
I assume the kraken-fft regression from comment 8 is why this is showing up as a 3% regression on Kraken on Talos?
Updated•11 years ago
|
Attachment #754557 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 11•11 years ago
|
||
Updated•11 years ago
|
Assignee: general → bhackett1024
Comment 12•11 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•