initialize JSStackFrame::scopeChain with immediate in scripted IC fast path... or not at all

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: luke, Assigned: bhackett)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
In the fastest scripted IC fast path, funobj->getParent() is a constant so we can bake the constant into the fast path.
(Reporter)

Comment 1

7 years ago
Actually, I think we could avoid setting scopeChain at all by using the same lazy-initialization trick we've been applying to the other members: if JSFRAME_HAS_SCOPE_CHAIN is not set in JSStackFrame::flags, you derive it from callee->parent, store it to scopeChain, and set the flag.  The important thing is to avoid needing to check this flag on fast paths.  Now, in a heavyweight function, we'll create a call object and thus the scopeChain will definitely be set and we needn't check.  But there are still all the PIC ops for dynamic name lookup that use the scopeChain in lightweight functions.  For these cases, I think we could have the function prologue eagerly set scopeChain (using the jaeger-compiler to tell us when the body contains any such PIC ops).  Hopefully, this would remove any lazy-scopeChain checks from hot paths.
Summary: initialize JSStackFrame::scopeChain with immediate in scripted IC fast path → initialize JSStackFrame::scopeChain with immediate in scripted IC fast path... or not at all
(Reporter)

Updated

7 years ago
Blocks: 557378
(Assignee)

Comment 2

7 years ago
Created attachment 479835 [details] [diff] [review]
WIP patch

This lazies the scopeChain and rval (also blockChain, but that's been superseded by bug 535912).  Working for JM.  This is I think 3ms or so on SS, 80ms or so on V8 (pure JM, less for JM+TM).  Times for this benchmark:

function bar(x) { return x; }
function foo() {
  for (var i = 0; i < 1200000; i++)
    bar;
}
foo();

JM (old): 10.9ms
JM (new): 8.3ms
JSC: 7.7ms
V8: 7ms

Given other overhead the cost of calls in the engines should be roughly on par.
Assignee: general → bhackett1024
(Assignee)

Comment 3

7 years ago
Err, that microbenchmark should be bar(0);, not bar;
(Reporter)

Updated

7 years ago
Duplicate of this bug: 595073
(Assignee)

Comment 5

7 years ago
Created attachment 479868 [details] [diff] [review]
working patch

This passes tests and trace-tests for -m, -j, and -m -j
Attachment #479835 - Attachment is obsolete: true
Attachment #479868 - Flags: review?(lw)
Attachment #479868 - Flags: review?(dvander)
(Reporter)

Comment 6

7 years ago
Comment on attachment 479868 [details] [diff] [review]
working patch

Sweet!  Only nits:

>     JSObject &scopeChain() const {
>+        if (!(flags_ & JSFRAME_HAS_SCOPECHAIN)) {
>+            ((JSStackFrame*)this)->scopeChain_ = callee().getParent();
>+            ((JSStackFrame*)this)->flags_ |= JSFRAME_HAS_SCOPECHAIN;
>+        }
>         return *scopeChain_;
>     }

Could you make flags_/scopeChain_ 'mutable' instead?

Also, could you add:

  JS_ASSERT_IF(!(flags_ & JSFRAME_HAS_SCOPECHAIN), isFunctionFrame());

>+        if (analysis.usesScopeChain()) {
>+            /*
>+             * Load the scope chain into the frame if necessary.  The scope chain
>+             * is always set for global and eval frames.
>+             */
>+            RegisterID t0 = Registers::ReturnReg;
>+            Jump hasScope = masm.branchTest32(Assembler::NonZero,
>+                                              FrameFlagsAddress(), Imm32(JSFRAME_HAS_SCOPECHAIN));
>+            masm.loadPayload(Address(JSFrameReg, JSStackFrame::offsetOfCallee(fun)), t0);
>+            masm.loadPtr(Address(t0, offsetof(JSObject, parent)), t0);
>+            masm.storePtr(t0, Address(JSFrameReg, JSStackFrame::offsetOfScopeChain()));
>+            hasScope.linkTo(masm.label(), &masm);
>+        }

Since global frames always have scopeChain set, and it is always valid to set a function's scopeChain to callee.getParent(), is there any reason you can't take out the dynamic branch and change the condition to just be "if (fun && analysis.usesScopeChain())" ?

>@@ -817,16 +816,27 @@ RunTracer(VMFrame &f)
>+    /* REVIEW: Can the tracer inspect the scope chain of frames above the entry frame? */

The only on-trace access of JSStackFrame::offsetOfScopeChain is in entryScopeChain.  Nor can I find any builtins that do so, so, AFAICS, no.
Attachment #479868 - Flags: review?(lw) → review+
Comment on attachment 479868 [details] [diff] [review]
working patch


> 
>         j.linkTo(masm.label(), &masm);
>+
>+        if (analysis.usesScopeChain()) {
>+            /*
>+             * Load the scope chain into the frame if necessary.  The scope chain
>+             * is always set for global and eval frames.
>+             */
>+            RegisterID t0 = Registers::ReturnReg;
>+            Jump hasScope = masm.branchTest32(Assembler::NonZero,
>+                                              FrameFlagsAddress(), Imm32(JSFRAME_HAS_SCOPECHAIN));

As discussed on IRC you can omit this code for heavyweight functions, since you know there will be a call object.
Attachment #479868 - Flags: review?(dvander) → review+
(Assignee)

Comment 8

7 years ago
http://hg.mozilla.org/tracemonkey/rev/2824ef10a50f

For JM, 6ms on SS (more than expected), 30ms on V8 (less than expected).

Updated

7 years ago
Whiteboard: fixed-in-tracemonkey

Comment 9

7 years ago
http://hg.mozilla.org/mozilla-central/rev/2824ef10a50f
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.