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

RESOLVED FIXED in mozilla22

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 726396 [details] [diff] [review]
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.
Attachment #726396 - Flags: review?(bhackett1024)
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.
Attachment #726396 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 2

5 years ago
(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 4

5 years ago
https://hg.mozilla.org/mozilla-central/rev/b0b858d36b3d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.