OdinMonkey: sequential compilation allocates LIR in the tempLifoAlloc

RESOLVED FIXED in mozilla23

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 745459 [details] [diff] [review]
patch

Alon noticed that memory climbs much higher in sequential builds in the shell than parallel builds.  The reason is that, while the type checking and MIRGraph generation is wrapped in an IonContext, this is out of scope for the OptimizeMIR/GenerateLIR calls in CheckFunctionBodiesSequential.  This failure was masked by the IonContext in ModuleCompiler leftover from before the parallelization patch.  Since this IonContext isn't necessary (and it masks bugs), I removed it too.  The assertion fix in ~Label is because ~Label runs in ~ModuleCompiler.
Attachment #745459 - Flags: review?(sstangl)
Comment on attachment 745459 [details] [diff] [review]
patch

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

Think this is partly to blame for Bug 868334?

::: js/src/ion/AsmJS.cpp
@@ +4518,5 @@
>              return false;
>  
>          IonSpewNewFunction(&mirGen->graph(), NullPtr());
>  
> +        IonContext icx(m.cx()->compartment, &mirGen->temp());

Note that although CheckFunctionBody() constructs yet another IonContext internally, the patch is fine as-is: I can clean up the redundancy when working on parallel codegen.
Attachment #745459 - Flags: review?(sstangl) → review+
(Assignee)

Comment 2

5 years ago
(In reply to Sean Stangl [:sstangl] from comment #1)
> Think this is partly to blame for Bug 868334?

Nah, it's only a problem in sequential mode.

> Note that although CheckFunctionBody() constructs yet another IonContext
> internally, the patch is fine as-is: I can clean up the redundancy when
> working on parallel codegen.

The scope of the two IonContexts is disjoint; the IonContext added by this patch is constructed after CheckFunctionBody returns.
(In reply to Luke Wagner [:luke] from comment #2)
> The scope of the two IonContexts is disjoint; the IonContext added by this
> patch is constructed after CheckFunctionBody returns.

I know -- I meant that a single IonContext can be used in CheckFunctionBodiesSequential() before the call to CheckFunctionBody(), eliminating a then-redundant IonContext from CheckFunctionBody().
(Assignee)

Comment 4

5 years ago
oic, good point
https://hg.mozilla.org/mozilla-central/rev/983dd922cef2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.