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)

Other Branch
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bhackett1024, Assigned: bhackett1024)

References

Details

(Whiteboard: [MemShrink])

Attachments

(2 files)

Attached patch patch (deleted) — 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 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+
Whiteboard: [MemShrink]
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 875720
Caused serious regressions for awfy-assorted: misc-basic-array (70%) and misc-basic-fpops (32%)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Attached patch misc-basic-arrayops fix (deleted) — Splinter Review
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)
(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!)
(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.
I assume the kraken-fft regression from comment 8 is why this is showing up as a 3% regression on Kraken on Talos?
Attachment #754557 - Flags: review?(jdemooij) → review+
Depends on: 876247
Assignee: general → bhackett1024
Depends on: 875804
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: