Don't create types for scripts until they are compiled by baseline

RESOLVED FIXED in mozilla24

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Depends on: 1 bug)

Other Branch
mozilla24
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink])

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
Created attachment 753236 [details] [diff] [review]
patch

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]
(Assignee)

Comment 2

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4370f503d69f
https://hg.mozilla.org/mozilla-central/rev/4370f503d69f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24

Updated

4 years ago
Depends on: 875720
Caused serious regressions for 
awfy-assorted: misc-basic-array (70%) and misc-basic-fpops (32%)
Backout due to the regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb75f6d6877
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
(Assignee)

Comment 7

4 years ago
Created attachment 754557 [details] [diff] [review]
misc-basic-arrayops fix

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!)
(Assignee)

Comment 9

4 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.
I assume the kraken-fft regression from comment 8 is why this is showing up as a 3% regression on Kraken on Talos?

Updated

4 years ago
Attachment #754557 - Flags: review?(jdemooij) → review+
(Assignee)

Comment 11

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae7acc7c485
Depends on: 876247
Assignee: general → bhackett1024
Depends on: 875804
https://hg.mozilla.org/mozilla-central/rev/2ae7acc7c485
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Depends on: 880091
Depends on: 920446
You need to log in before you can comment on or make changes to this bug.