852331 - immediately GC the Zone after JS::Evaluate on a very large script

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: patch

The top-level scripts for these Emscripten apps are enormous.  This one demo has script->length over 4,000,000 and allocates 1.5GB in cx->analysisLifoAlloc.  This causes OOMs later on when the app does actual allocation.

Brian, iiuc, bug 804676 will remove ScriptAnalysis and all this analyze::Bytecode allocation, right?

In the short term, I think we can just do a zone GC which will release all this data before requestAnimationFrame gets set which prevents us from freeing it.

Comment 1

[Brian Hackett [Laid off!]]

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

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

This looks fine, though I worry that we could go nuts doing tons of GCs if the LARGE_SCRIPT_LENGTH limit is hit in other situations?  Particularly, things like XmlHttpRequest which apparently do lots of one-time script evaluation (don't know if that's through Evaluate() though) and which also hurt a lot (bug 789892).

Bug 804676 will be paving the way for removal of Script::analysis but will not go that final step.  The baseline compiler also depends on the script analysis so once 804676 is done the next step is moving needed logic from analyzeBytecode() to the baseline compiler itself.  After that there are a couple lingering uses of the analysis info ('new' script definite properties, lazy arguments) to move over to use MIR instead, after that the analysis info can be killed entirely.

Comment 2

[Luke Wagner [:luke]]

(In reply to Brian Hackett (:bhackett) from comment #1)
> This looks fine, though I worry that we could go nuts doing tons of GCs if
> the LARGE_SCRIPT_LENGTH limit is hit in other situations?  Particularly,
> things like XmlHttpRequest which apparently do lots of one-time script
> evaluation (don't know if that's through Evaluate() though) and which also
> hurt a lot (bug 789892).

If the problem is too much type-inference memory (glancing at the title of bug 789892); I'd think this patch would be a net win since it'll keep flushing.

> Bug 804676 will be paving the way for removal of Script::analysis but will
> not go that final step.  The baseline compiler also depends on the script
> analysis so once 804676 is done the next step is moving needed logic from
> analyzeBytecode() to the baseline compiler itself.  After that there are a
> couple lingering uses of the analysis info ('new' script definite
> properties, lazy arguments) to move over to use MIR instead, after that the
> analysis info can be killed entirely.

Ah, ok; I'm glad we're headed that direction though.

Comment 3

[Luke Wagner [:luke]]

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

Comment 4

[Ed Morley [:emorley]]

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