869733 - CloneInterpretedFunctions into the tenured generation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Terrence Cole [:terrence]]

Attachment: v0

These flow into jit-code registers and thus are required to be tenured at the moment.

Comment 1

[Terrence Cole [:terrence]]

Sorry, s/registers/CompilerRoots/.

Comment 2

[Brian Hackett [Laid off!]]

This path is used every time a Lambda needs to clone its input function, so we shouldn't be always using the tenured generation.  What are the conditions when these cloned functions can be used as CompilerRoots?

Comment 3

[Shu-yu Guo [:shu]]

(In reply to Brian Hackett (:bhackett) from comment #2)
> This path is used every time a Lambda needs to clone its input function, so
> we shouldn't be always using the tenured generation.  What are the
> conditions when these cloned functions can be used as CompilerRoots?

I think the situation that came up was that a function whose script that contains lambdas is freshly cloned (such as self-hosted functions) gets compiled. The functions inside this script's |objects()| array are still in the nursery, and we try to CompilerRoot them when compiling.

I'm not positive that's what's happening, though. Terrence, can you see if always cloning self-hosted functions into the tenured heap helps?

Comment 4

[Terrence Cole [:terrence]]

(In reply to Shu-yu Guo [:shu] from comment #3)
> (In reply to Brian Hackett (:bhackett) from comment #2)
> > This path is used every time a Lambda needs to clone its input function, so
> > we shouldn't be always using the tenured generation.  What are the
> > conditions when these cloned functions can be used as CompilerRoots?
> 
> I think the situation that came up was that a function whose script that
> contains lambdas is freshly cloned (such as self-hosted functions) gets
> compiled. The functions inside this script's |objects()| array are still in
> the nursery, and we try to CompilerRoot them when compiling.
> 
> I'm not positive that's what's happening, though. Terrence, can you see if
> always cloning self-hosted functions into the tenured heap helps?

This indeed fixes all of the self-hosted cases that come up. Unfortunately all of the arrow cases and the parallelarray scatter tests still fail, as well as (I think) several others under baseline. After discussing this on #ionmonkey this morning, it turns out the Lambda call tree is a thicket of code that nobody really understands.

Given that it's going to need to be rewritten in order to support lazy bytecode, and given that Lambda is comparatively rare, I think the best approach for now is to just aggressively tenure all cloning activity under Lambda. We can revisit this all after lazy bytecode. I up a new patch once I've had some time to test that this approach actually solves the remaining problems.

Comment 5

[Terrence Cole [:terrence]]

Attachment: v0

This implements the change described above. Feel free to pass review to Brian if you aren't comfortable reviewing this: it seems he's also quite familiar with this cloning code.

Comment 6

[Shu-yu Guo [:shu]]

Comment on attachment 748298 [details] [diff] [review]
v0

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

I'm not *too* familiar with all the clone functions, but I've probably looked at them more than others due to my dealings with the self-hosting infrastructure. This patch just threads NewObjectKind through; looks fine to me.

::: js/src/jsfun.cpp
@@ +1581,5 @@
>       * functions. Cross-compartment cloning only happens via JSAPI
>       * (JS_CloneFunctionObject) which dynamically ensures that 'script' has
>       * no enclosing lexical scope (only the global scope).
>       */
> +    if (cloneRoot->isInterpreted() && !CloneFunctionScript(cx, fun, cloneRoot, newKindArg))

A note for posterity: newKindArg is passed through here as singleton types are not structurally recursive: just because a closure needs a singleton type doesn't mean any objects it contains (including other closures) are necessarily singleton-typed.

::: js/src/jsfun.h
@@ +390,5 @@
>  
>  extern JSFunction *
>  CloneFunctionObject(JSContext *cx, HandleFunction fun, HandleObject parent,
> +                    gc::AllocKind kind = JSFunction::FinalizeKind,
> +                    NewObjectKind newKind = GenericObject);

Nit: newKind -> newKindArg to be consistent with definition.

Comment 7

[Terrence Cole [:terrence]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=4bed17d9a9d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/119618950052

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/119618950052
https://hg.mozilla.org/mozilla-central/rev/3948c9769e03

Comment 9

[Terrence Cole [:terrence]]

Note for posterity: bug 871036 also mistakenly landed under this bug number.