Closed Bug 807443 Opened 7 years ago Closed 7 years ago

IonMonkey: Add support for DeclEnv scope object.

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: nbp, Assigned: nbp)

References

(Blocks 1 open bug)

Details

(Whiteboard: [ion:t])

Attachments

(1 file, 1 obsolete file)

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
(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?
Assignee: general → nicolas.b.pierron
Status: NEW → ASSIGNED
Blocks: 813425
(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.
Summary: IonMonkey: Add support for JSOP_DECLENV bytecode. → IonMonkey: Add support for DeclEnv scope object.
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%).
Attachment #684334 - Flags: review?(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.
Attachment #684334 - Flags: review?(luke)
Attachment #684334 - Flags: review?(dvander)
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).
Attachment #684334 - Flags: review?(dvander) → review+
(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.
Delta:
- Move lamdba initialization out of the DeclEnv template creation (use fp->callee instead of fp->fun).
- Add test case corresponding to Bug 819035.
Attachment #684334 - Attachment is obsolete: true
Attachment #689613 - Flags: review?(dvander)
Attachment #689613 - Flags: review?(dvander) → review+
(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
https://hg.mozilla.org/mozilla-central/rev/93cac86bdd95
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 819865
Depends on: 822170
You need to log in before you can comment on or make changes to this bug.