865059 - Don't analyze scripts until they are compiled by baseline

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Right now, we analyze scripts when they first execute, to keep track of the types of values that have been observed and update the associated type sets on the script.  With bug 804676 fixed this behavior is no longer required by Ion, but is still required by JM+TI (which infallibly reuses the inferred types at loop headers during OSR).  If JM is removed then this analysis could be postponed until the script is baseline compiled, which would allow plenty of time to accumulate type information before the script is Ion compiled.

Postponing the generation of analysis information (and additionally, folding it into the baseline script itself) would be beneficial as a lot of code only executes once or a few times.  Some sites which execute lots of loop free scripts waste a lot of memory due to this analysis information.

Comment 1

[Luke Wagner [:luke]]

This sounds great and will definitely help some of the memory spikes I've seen for the non-asm.js portions of Emscripten code (boilerplate, glue, etc).

Folding it into baseline compilation sounds right.  Could all the analyze::Bytecodes be allocated in a LifoAllocScope released at the end of baseline compilation (thereby removing the need for the released-on-gc analysisLifoAlloc)?  While this would require re-analysis on every baseline compile and Ion compile of a method:(1) IIUC baseline doesn't need to recompile much (2) for Ion, I assume the analysis time would be small compared to the cost of Ion compilation.

Comment 2

[Brian Hackett [Laid off!]]

I'm hoping we'll be able to kill ScriptAnalysis entirely and fold the minimal amount of analysis needed into baseline compilation itself.  Baseline uses the analysis info for a few things such as knowing jump targets, but I think Ion uses are even more minimal (Ion uses source notes to traverse the bytecode so figures out jump targets etc. separately) and could be easily folded into the BaselineScript.

Comment 3

[Luke Wagner [:luke]]

\o/

Comment 4

[Brian Hackett [Laid off!]]

Thinking a bit more about this, in cases where JM is disabled at compile time we could do this bug now.  The script isn't analyzed until it is baseline compiled, and when monitoring bytecode results the interpreter first checks to see if there is analysis info, doing nothing if there isn't.  If JM is enabled then the script still needs to be analyzed immediately for the reason outlined in comment 0.

Accelerating this could be beneficial both for the browser and for b2g (if configure options are changed to disable JM completely).  The latter could be especially important if, as I vaguely recall from the GC meeting today, b2g will be forking a version off trunk soon and not merge again for several months.  Is anyone around with actual knowledge about this?

Comment 5

[Brian Hackett [Laid off!]]

Attachment: patch

This patch adds a cx->jaegerCompilationAllowed() bit which works similarly to cx->typeInferenceEnabled() --- set at zone creation and not changed afterwards.  If the bit is set then we do the same analysis work as usual to accumulate types for JM execution, otherwise we don't analyze until required by baseline compilation or by the 'arguments' or 'new' script properties analyses.  Still a couple TODOs:

- There is an exception to the above, we still analyze scripts that contain initializers to see whether they are in a loop and shouldn't have singleton type.  Will fix this in a separate patch.

- I still need to make sure this bit is being set properly in the browser.

Comment 6

[Brian Hackett [Laid off!]]

Attachment: part 2

Adds a try note for non-iterator loops, so that we can quickly test for whether a bytecode is inside a loop.  Use this instead of analysis information for deciding to use a singleton type during object initialization.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 743063 [details] [diff] [review]
patch

Review of attachment 743063 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, we should change the default value for the JM browser prefs though. Also please make sure you get reasonable SS numbers for --no-ion --no-baseline (to confirm JM+TI still works).

Comment 8

[Brian Hackett [Laid off!]]

Attachment: part 3

Fix browser defaults to disable JM, and remove some test detritus referencing these config options.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 743092 [details] [diff] [review]
part 2

Review of attachment 743092 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4367,5 @@
>      if (EmitJump(cx, bce, op, top - bce->offset()) < 0)
>          return false;
>  
> +    if (!bce->tryNoteList.append(JSTRY_LOOP, bce->stackDepth, top, bce->offset()))
> +        return false;

Bump XDR_BYTECODE_VERSION in vm/Xdr.h

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/80163a75cca5

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f37eeebce2eb

So far, looks like:

* talos tp on every platform with talos' opaque "browser non-zero return code" a la https://tbpl.mozilla.org/php/getParsedLog.php?id=22377765&tree=Mozilla-Inbound

* android jsreftest-2 crashing in regress-119909.js [@ JSC::Yarr::ByteCompiler::emitDisjunction(JSC::Yarr::PatternDisjunction*, unsigned int, unsigned int)] a la https://tbpl.mozilla.org/php/getParsedLog.php?id=22377910&tree=Mozilla-Inbound

* probably Win talos xperf crashing [@ js::types::TypeSet::hasType(js::types::Type)] 

* perhaps talos dromaeo_css crashing [@ js::ion::BuildDominatorTree(js::ion::MIRGraph&)]

Comment 12

[Phil Ringnalda (:philor)]

* perhaps b2g mochitest startup hangs (or crashes, or total failure to start, it can be hard to tell what it's up to)

Comment 13

[Brian Hackett [Laid off!]]

Hmm, the hasType crash is easy to fix but it's not at all obvious what's happening with the other failures.

Decoder and Gary, can you try fuzzing rev 80163a75cca5 with a variety of options that include --no-jm?  I don't know if this will turn up anything besides the hasType crash but seems worth trying.

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Decoder and Gary, can you try fuzzing rev 80163a75cca5 with a variety of
> options that include --no-jm?  I don't know if this will turn up anything
> besides the hasType crash but seems worth trying.

The jsfunfuzz harness already fuzzes with various options including --no-jm.

Comment 15

[Brian Hackett [Laid off!]]

Attachment: turn off methodjit prefs

All the ARM/b2g failures seem to stem from just turning off the methodjit pref.  This is entangled with code that controls the Yarr JIT, which also gets disabled and the fallback interpreter crashes somewhere.  This patch disentangles these two and on try doesn't crash in any new spots.

https://tbpl.mozilla.org/?tree=Try&rev=5a16355efbde

Comment 16

[Brian Hackett [Laid off!]]

Turn off methodjit prefs:

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0a237017a9

Comment 17

[Brian Hackett [Laid off!]]

The rest:

https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac1564bff87

Try run:

https://tbpl.mozilla.org/?tree=Try&rev=9df6f327d8f2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf0a237017a9

Comment 19

[Ryan VanderMeulen [:RyanVM]]

(In reply to Brian Hackett (:bhackett) from comment #17)
> The rest:
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac1564bff87
> 
> Try run:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=9df6f327d8f2

Backed out for causing intermittent linux32 dromaeo crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c426bd08d28

https://tbpl.mozilla.org/php/getParsedLog.php?id=22511532&tree=Mozilla-Inbound

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Different stack:
https://tbpl.mozilla.org/php/getParsedLog.php?id=22511203&tree=Mozilla-Inbound

Comment 21

[Ryan VanderMeulen [:RyanVM]]

(In reply to Brian Hackett (:bhackett) from comment #17)
> Try run:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=9df6f327d8f2

Retriggers on this push show the crash too.

Comment 22

[Matt Brubeck (:mbrubeck)]

This patch also caused regressions in V8, Kraken, and Dromaeo on multiple platforms, both times it landed.  Were these expected?

http://graphs.mozilla.org/graph.html#tests=[[230,131,33],[230,131,12]]&sel=1366918928204,1367523728204

https://groups.google.com/d/topic/mozilla.dev.tree-management/tgJnrydtoeA/discussion

Comment 23

[Brian Hackett [Laid off!]]

Attachment: allow monitoring types without analyzing scripts

The dromaeo failure is some existing bug in the compiler that is getting tickled by the changes to the type information here.  I can't reproduce it myself, it only repros once in every 20 or so times on recent try runs, and the signature gives no obvious clues for diagnosis.  I'm going to try some random things to see if they avoid the dromaeo crash, but who knows if that will work.  Without some STR these patches are hosed.

The attached patch gives much of the benefit, generates the same type information and is stable on try.  Instead of analyzing a script to allow the interpreter to monitor bytecode results, it just computes a much smaller index -> pc map for the monitored opcodes, just enough information to efficiently allow the type sets to be updated.  The amount of memory used by this map is a tiny fraction of the analysis information.  The main way this is deficient vs. the earlier patch is that we still need to allocate script->types for scripts that haven't yet been compiled by baseline.  script->types consumes a significant amount of memory, though nothing anywhere near as bursty as analysis memory currently is.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 745584 [details] [diff] [review]
allow monitoring types without analyzing scripts

Review of attachment 745584 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.

Comment 25

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ca9a6bd8f64

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for jsreftest crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5e04d0c4a37

https://tbpl.mozilla.org/php/getParsedLog.php?id=22661073&tree=Mozilla-Inbound

Comment 27

[Ryan VanderMeulen [:RyanVM]]

And jit-test failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=22661878&tree=Mozilla-Inbound

But hey, at least this landed late in the day, so we won't be starring as many failures for the next few hours...

Comment 28

[Phil Ringnalda (:philor)]

And Windows dromaeo. And apparently left Android completely unable to read reftest/crashtest manifests, which is a particularly curious failure.

Comment 29

[Phil Ringnalda (:philor)]

Particularly curious, and perhaps a needs-clobber that you inherited, since later runs before you got backed out didn't have the same complete Android bustage.

Comment 30

[Brian Hackett [Laid off!]]

This bug is cursed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/357af4877abd

Comment 31

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #22)
> This patch also caused regressions in V8, Kraken, and Dromaeo on multiple
> platforms, both times it landed.  Were these expected?

It looks like the Kraken regression was fixed in the second landing, but V8 and Dromaeo still regressed significantly.  Again, was this expected?  Should we back out the third landing if it causes 10-20% regressions in V8 and Dromaeo DOM?

Comment 32

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #31)
> It looks like the Kraken regression was fixed in the second landing, but V8
> and Dromaeo still regressed significantly.  Again, was this expected?

On the third landing, both Kraken and V8 are fine but Dromaeo DOM and CSS both regressed around 10-15%:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=081ebf4e0886,3b562fd6d653,7d3a05e286a6&newRev=357af4877abd&submit=true

Comment 33

[Brian Hackett [Laid off!]]

(In reply to Matt Brubeck (:mbrubeck) from comment #32)
> (In reply to Matt Brubeck (:mbrubeck) from comment #31)
> > It looks like the Kraken regression was fixed in the second landing, but V8
> > and Dromaeo still regressed significantly.  Again, was this expected?
> 
> On the third landing, both Kraken and V8 are fine but Dromaeo DOM and CSS
> both regressed around 10-15%:
> http://perf.snarkfest.net/compare-talos/index.html?oldRevs=081ebf4e0886,
> 3b562fd6d653,7d3a05e286a6&newRev=357af4877abd&submit=true

I'll fix the dromaeo regression in place.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/357af4877abd

Comment 35

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Anything left here?

Comment 36

[Kartikaya Gupta (email:[e-mail])]

This patch reduced startup resident memory usage by ~2mb on Fennec. \o/

Comment 37

[Brian Hackett [Laid off!]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #35)
> Anything left here?

No, the remaining work was split off into separate bugs.

Comment 38

[Andrew McCreight [:mccr8]]

Marking fixed per comment 37.

Comment 39

[Andrew McCreight [:mccr8]]

Looking at AreWeSlimYet, on desktop, this reduced startup resident memory by 14mb, which is about 10%.  That 14mb reduction entirely eliminates the difference in memory usage we were seeing between startup and 30 seconds later.  Pretty great!