Closed Bug 593882 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: bhackett1024)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

In the fastest scripted IC fast path, funobj->getParent() is a constant so we can bake the constant into the fast path.
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
Blocks: 557378
Attached patch WIP patch (obsolete) — Splinter Review
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
Err, that microbenchmark should be bar(0);, not bar;
Attached patch working patchSplinter Review
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)
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+
http://hg.mozilla.org/tracemonkey/rev/2824ef10a50f

For JM, 6ms on SS (more than expected), 30ms on V8 (less than expected).
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/2824ef10a50f
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.