For bug 1000780 (and likely other self-hosted functions in the future) we need a fast way to determine whether a JS frame is constructing. Atm we look at the caller's pc for JIT frames (constructing iff caller's pc == JSOP_NEW), but this is slow. The plan is to store this in the numActualArgs_ word, so that we can add an intrinsic that's just a few instructions when inlined in Ion. For inlined frames, this intrinsic can be a no-op. We should also add a shell function so that we can easily test this.

This patch adds the IsConstructing intrinsic from Till's patch in bug 1000780 and makes it very fast for Ion code: if the calling script is inlined, it's a no-op. Else, it's just a load + and. Initially I wanted to store the IsConstructing-flag in the argc word (see comment 0), but it turned out to be much nicer and cleaner to use a different CalleeTokenTag. Till, can you test if this helps bug 1000780?

Comment on attachment 8480800 [details] [diff] [review] Patch Review of attachment 8480800 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Jan de Mooij [:jandem] from comment #1) > Till, can you test if this helps bug 1000780? It very much does! Results for the benchmark from bug 1000780 without and with this patch: Without: --- calling performance --- call native, 0,0 args: 2.5029296875 call self-h, 0,0 args: 72.77392578125 call native, 2,3 args: 2.72802734375 call self-h, 2,3 args: 90.06396484375 call native, 0,5 args: 2.506103515625 call self-h, 0,5 args: 95.594970703125 call native, 5,0 args: 2.486083984375 call self-h, 5,0 args: 61.323974609375 With: --- calling performance --- call native, 0,0 args: 2.591064453125 call self-h, 0,0 args: 3.740966796875 call native, 2,3 args: 2.59619140625 call self-h, 2,3 args: 15.347900390625 call native, 0,5 args: 2.505859375 call self-h, 0,5 args: 5.168212890625 call native, 5,0 args: 2.43310546875 call self-h, 5,0 args: 4.60302734375

(In reply to Till Schneidereit [:till] from comment #2) > It very much does! Results for the benchmark from bug 1000780 without and > with this patch: Awesome, thanks for testing. But wasn't the self-hosted one supposed to be faster than native, or is this due to bug 1001850? The fix for that bug doesn't work in many cases so this still seems worth doing, and is also simpler.

(In reply to Jan de Mooij [:jandem] from comment #3) > (In reply to Till Schneidereit [:till] from comment #2) > > It very much does! Results for the benchmark from bug 1000780 without and > > with this patch: > > Awesome, thanks for testing. But wasn't the self-hosted one supposed to be > faster than native, or is this due to bug 1001850? The fix for that bug > doesn't work in many cases so this still seems worth doing, and is also > simpler. Mostly the native version got faster, yes. However the numbers in bug 1001850 comment 4 were quite a bit better than what I have now, so I'll have to look into that some more. The numbers for the 2,3 case regressed substantially in the self-hosted version, so something's going on there. I think we should be able to get to ~parity on calling performance, at which point this'd be very much worth doing: .bind itself is a bottleneck in some cases, and the native implementation has substantial complexity in the engine, which we could remove or at least pare down. I'll look into this some more next week, and we might even get this landed before bug 911142 is resolved, I think.

Comment on attachment 8480800 [details] [diff] [review] Patch Review of attachment 8480800 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jit/arm/Trampoline-arm.cpp @@ +428,5 @@ > > // Load the number of |undefined|s to push into r6. > masm.ma_ldr(DTRAddr(sp, DtrOffImm(IonRectifierFrameLayout::offsetOfCalleeToken())), r1); > + masm.mov(r1, r6); > + masm.andPtr(Imm32(CalleeTokenMask), r6); nit: masm.ma_and(Imm32(CalleeTokenMask), r1, r6); ::: js/src/vm/SelfHosting.cpp @@ +813,5 @@ > JS_FN("ThrowError", intrinsic_ThrowError, 4,0), > JS_FN("AssertionFailed", intrinsic_AssertionFailed, 1,0), > JS_FN("SetScriptHints", intrinsic_SetScriptHints, 2,0), > JS_FN("MakeConstructible", intrinsic_MakeConstructible, 1,0), > + JS_FN("_IsConstructing", intrinsic_IsConstructing, 0,0), Any reason for having an underscore as prefix?

(In reply to Nicolas B. Pierron [:nbp] from comment #5) > > + JS_FN("_IsConstructing", intrinsic_IsConstructing, 0,0), > > Any reason for having an underscore as prefix? Yes, we want to move all the intrinsics to that naming scheme to make them at least slightly more visible as internal, non-sandboxed functions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c0bf50217f7 Just realized we can optimize some more code now; follow-up patch soon.

We can now remove IsTopFrameConstructing and add BaselineFrame::isConstructing() instead.

Comment on attachment 8483601 [details] [diff] [review] Part 2 - Remove IsTopFrameConstructing Review of attachment 8483601 [details] [diff] [review]: ----------------------------------------------------------------- Nice, this avoid iterating the stack when we already know the location of the calleeToken.

Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/0e5fa80ca0b9