807443 - IonMonkey: Add support for DeclEnv scope object.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

2 PdfJS (octane benchmark) functions are not compiled because IonMonkey does not support JSOP_DECLENV yet.

[Scripts] Analyzing script pdfjs.js:17061 (0x7fdbd39acd20) (usecount=10340) (maxloopcount=171)
[Abort] DeclEnv scope objects are not yet supported
[Abort] aborted @ pdfjs.js:17063
[Abort] Builder failed to build.
[Abort] Disabling Ion compilation of script pdfjs.js:17061

[Scripts] Analyzing script pdfjs.js:14474 (0x7fdbd39a0f48) (usecount=10440) (maxloopcount=85)
[Abort] DeclEnv scope objects are not yet supported
[Abort] aborted @ pdfjs.js:14475
[Abort] Builder failed to build.
[Abort] Disabling Ion compilation of script pdfjs.js:14474

Comment 1

[Luke Wagner [:luke]]

(To wit: there is no JSOP_DECLENV, rather heavyweight named lambdas (fun->isHeavyweight() && fun->isNamedLambda()).)

Creating a DeclEnvObject inline (in the prologue, next to where we create the CallObject) seems not-too-hard.  The harder part will be if the name of a named lambda is used at all ;)  For simple recursion with named lambda:
  (function f() { f() })
the frontend will emit JSOP_CALLEE which I see is not handled in IonBuilder (I guess this will abort compilation?).  This should be simple to handle as long as you have the callee on hand.  It's worse when the name of a named lambda is used from a nested function; for:
  (function f() { function g() { f() } })
the frontend will emit JSOP_CALLNAME!  (There is no fundamental reason why we couldn't use JSOP_CALLALIASEDVAR; the only reason that we don't is because the hacky handling of named lambdas in the frontend makes this a pain.)  Could you instrument JSOP_NAME/JSOP_CALLNAME to see if we are hitting this in pdfjs?

Comment 2

[Nicolas B. Pierron [:nbp]]

(In reply to Luke Wagner [:luke] from comment #1)
> It's worse when the name of a named
> lambda is used from a nested function; for:
>   (function f() { function g() { f() } })
> the frontend will emit JSOP_CALLNAME!  (There is no fundamental reason why
> we couldn't use JSOP_CALLALIASEDVAR; the only reason that we don't is
> because the hacky handling of named lambdas in the frontend makes this a
> pain.)  Could you instrument JSOP_NAME/JSOP_CALLNAME to see if we are
> hitting this in pdfjs?

From what I saw from PdfJS, this case should not appear, the reason they named their lambda is to provide symbols in the debugger.  Most parts of B2G are using the same trick.  I haven't instrumented the code yet to see if this case was happening or not.

Comment 3

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey, Compile named lambdas.

This patch duplicate the logic made for CallObject to DeclEnvObject, namely:
- Add a createTemplateObject function to DevlEnvObject
- Clone this template object in the generated code.

This patch also:
- Use a fixed slot for the name of the lambda, instead of allocating a dynamic slot.
- Remove JSCLASS_HAS_PRIVATE from the DeclEnvObject. It does not seems to be used and it does not seems to fail yet.
- Fix encoding of LArgument offsets in snapshots, such as MCallee is correctly recovered after a bailout.
- Fix a minor warning in the init of IonBuilder.
- Include Bug 807464 modification (sorry about that)


Currently this patch cause a regression of performances in PdfJS because of other issues such as Bug 813425, still the goal is to compile hot scripts in IonMonkey.  It still improve the no-jm PdfJS score from 2430 to 4050 (+66%).

Comment 4

[Luke Wagner [:luke]]

Comment on attachment 684334 [details] [diff] [review]
IonMonkey, Compile named lambdas.

Sorry for the delay.  I'm not a qualified Ion reviewer, so I'm afraid I'll have defer to someone else.  dvander did the original CallObject-creation-inlining.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 684334 [details] [diff] [review]
IonMonkey, Compile named lambdas.

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

::: js/src/ion/IonBuilder.cpp
@@ +585,5 @@
>          if (fun->isHeavyweight()) {
> +            if (fun->isNamedLambda()) {
> +                scope = createDeclEnvObject(scope);
> +                if (!scope)
> +                    return false;

When we bailout, we might not have gotten a chance to create the scope objects yet, and we check for that in ThunkToInterpret and elsewhere. I'd check EnsureHasCallObject to make sure this will work with DeclEnv (maybe rename to EnsureHasScopeObjects if not).

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to David Anderson [:dvander] from comment #5)
> When we bailout, we might not have gotten a chance to create the scope
> objects yet, and we check for that in ThunkToInterpret and elsewhere. I'd
> check EnsureHasCallObject to make sure this will work with DeclEnv (maybe
> rename to EnsureHasScopeObjects if not).

Ok, I checked the code and EnsureHasCallObject calls into StackFrame::initCallObject which use CallObject::createForFunction.  createForFunction allocates a DeclEnvObject if the function is a named lambda.

Comment 7

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/747b04228fa6

Comment 8

[(no longer active)]

Backed out: http://hg.mozilla.org/integration/mozilla-inbound/rev/6d32ee966b68

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: IonMonkey, Compile named lambdas.

Delta:
- Move lamdba initialization out of the DeclEnv template creation (use fp->callee instead of fp->fun).
- Add test case corresponding to Bug 819035.

Comment 10

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:pierron] [:nbp] from comment #9)
> Created attachment 689613 [details] [diff] [review]
> IonMonkey, Compile named lambdas.
> 
> Delta:
> - Move lamdba initialization out of the DeclEnv template creation (use
> fp->callee instead of fp->fun).
> - Add test case corresponding to Bug 819035.

https://hg.mozilla.org/integration/mozilla-inbound/rev/93cac86bdd95

Comment 11

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/93cac86bdd95