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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: 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.

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4370f503d69f

Comment 3

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4370f503d69f

Comment 4

[Hannes Verschore [:h4writer]]

Caused serious regressions for 
awfy-assorted: misc-basic-array (70%) and misc-basic-fpops (32%)

Comment 5

[Hannes Verschore [:h4writer]]

Backout due to the regressions:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb75f6d6877

Comment 6

[Hannes Verschore [:h4writer]]

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

Comment 7

[Brian Hackett [Laid off!]]

Attachment: 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.

Comment 8

[Hannes Verschore [:h4writer]]

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

Comment 9

[Brian Hackett [Laid off!]]

(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

[Boris Zbarsky [:bzbarsky]]

I assume the kraken-fft regression from comment 8 is why this is showing up as a 3% regression on Kraken on Talos?

Comment 11

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae7acc7c485

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ae7acc7c485