868684 - OdinMonkey: sequential compilation allocates LIR in the tempLifoAlloc

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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

Comment 1

[Sean Stangl [: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.

Comment 2

[Luke Wagner [:luke]]

(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.

Comment 3

[Sean Stangl [:sstangl]]

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

Comment 4

[Luke Wagner [:luke]]

oic, good point

Comment 5

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/983dd922cef2

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/983dd922cef2