Spun out from bug 555246. Suppose we compute 'this' eagerly except when 'this' would be the global this. In that one case we set fp->argv[-1] and fp->thisv to JSVAL_NULL. This would cost nothing on trace or on property cache hits. It would eliminate all the guards emitted by TR::getThis. And we can inline the most common case of JS_THIS: static JS_ALWAYS_INLINE jsval JS_THIS(JSContext *cx, jsval *vp) { return JSVAL_IS_PRIMITIVE(vp[1]) ? JS_ComputeThis(cx, vp) : vp[1]; }

This wins 3.5% on v8, 23% on v8-richards! But it loses 1.6% on sunspider because of a single test, unpack-code. I'll profile that one today and see what's the matter. TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: *1.016x as slow* 798.9ms +/- 0.8% 811.4ms +/- 0.6% significant ============================================================================= 3d: - 126.2ms +/- 3.4% 122.7ms +/- 3.2% cube: - 44.8ms +/- 8.7% 44.1ms +/- 8.4% morph: 1.030x as fast 30.6ms +/- 1.6% 29.7ms +/- 1.2% significant raytrace: 1.039x as fast 50.8ms +/- 1.7% 48.9ms +/- 0.8% significant access: - 128.9ms +/- 3.0% 125.0ms +/- 0.8% binary-trees: 1.040x as fast 18.3ms +/- 1.9% 17.6ms +/- 2.1% significant fannkuch: - 72.1ms +/- 0.6% 71.6ms +/- 0.7% nbody: - 26.5ms +/- 14.7% 24.3ms +/- 2.0% nsieve: 1.043x as fast 12.0ms +/- 2.8% 11.5ms +/- 3.3% significant bitops: - 35.0ms +/- 1.9% 34.5ms +/- 1.5% 3bit-bits-in-byte: - 1.4ms +/- 26.4% 1.2ms +/- 25.1% bits-in-byte: *1.033x as slow* 9.0ms +/- 0.0% 9.3ms +/- 3.7% significant bitwise-and: - 2.1ms +/- 10.8% 2.1ms +/- 10.8% nsieve-bits: 1.027x as fast 22.5ms +/- 2.2% 21.9ms +/- 1.0% significant controlflow: ?? 8.1ms +/- 2.8% 8.2ms +/- 3.7% not conclusive: might be *1.012x as slow* recursive: ?? 8.1ms +/- 2.8% 8.2ms +/- 3.7% not conclusive: might be *1.012x as slow* crypto: - 50.1ms +/- 1.2% 49.5ms +/- 2.6% aes: ?? 29.3ms +/- 2.3% 29.5ms +/- 4.3% not conclusive: might be *1.007x as slow* md5: - 13.2ms +/- 2.3% 13.0ms +/- 0.0% sha1: 1.086x as fast 7.6ms +/- 4.9% 7.0ms +/- 0.0% significant date: 1.015x as fast 112.1ms +/- 0.5% 110.4ms +/- 0.6% significant format-tofte: *1.010x as slow* 58.2ms +/- 0.8% 58.8ms +/- 0.8% significant format-xparb: 1.045x as fast 53.9ms +/- 0.4% 51.6ms +/- 0.7% significant math: - 39.9ms +/- 1.0% 39.5ms +/- 1.5% cordic: - 11.9ms +/- 3.4% 11.8ms +/- 2.6% partial-sums: 1.005x as fast 21.0ms +/- 0.0% 20.9ms +/- 1.1% significant spectral-norm: 1.029x as fast 7.0ms +/- 0.0% 6.8ms +/- 4.4% significant regexp: - 34.5ms +/- 1.5% 34.1ms +/- 1.8% dna: - 34.5ms +/- 1.5% 34.1ms +/- 1.8% string: *1.089x as slow* 264.1ms +/- 1.6% 287.5ms +/- 0.5% significant base64: ?? 13.3ms +/- 2.6% 13.4ms +/- 2.8% not conclusive: might be *1.008x as slow* fasta: - 60.9ms +/- 3.1% 60.0ms +/- 1.5% tagcloud: 1.028x as fast 85.5ms +/- 1.6% 83.2ms +/- 0.9% significant unpack-code: *1.35x as slow* 79.3ms +/- 4.1% 107.0ms +/- 0.4% significant validate-input: 1.050x as fast 25.1ms +/- 2.8% 23.9ms +/- 0.9% significant TEST COMPARISON FROM TO DETAILS ============================================================================= ** TOTAL **: 1.035x as fast 8074.9ms +/- 0.4% 7802.9ms +/- 0.4% significant ============================================================================= v8: 1.035x as fast 8074.9ms +/- 0.4% 7802.9ms +/- 0.4% significant crypto: 1.017x as fast 777.2ms +/- 0.8% 764.1ms +/- 0.5% significant deltablue: 1.090x as fast 700.8ms +/- 1.3% 642.8ms +/- 1.8% significant earley-boyer: 1.023x as fast 2489.3ms +/- 0.7% 2434.3ms +/- 0.5% significant raytrace: 1.036x as fast 1375.7ms +/- 1.8% 1327.8ms +/- 1.0% significant regexp: - 999.3ms +/- 0.3% 997.6ms +/- 0.7% richards: 1.23x as fast 565.4ms +/- 0.4% 458.5ms +/- 0.9% significant splay: ?? 1167.2ms +/- 0.9% 1177.8ms +/- 1.1% not conclusive: might be *1.009x as slow*

This is the patch I used to produce the numbers in comment 1. Very similar to the patch I posted in bug 555246.

Profiled string-unpack-code. Before this patch, we don't call num_parseInt much, because we're on trace and it's pure. With the patch, we call it a lot, which indicates we're not staying on trace. Still looking.

See test case below. Without the patch, we get 1 abort. With the patch, we get extra aborts like these: ... Abort recording of tree ./string-unpack-code.js:13@18 at ./string-unpack-code.js:4@0: Inner tree is an unsupported type of recursion. trace stopped: 4861: control flow should have been recursive Abort recording of tree ./string-unpack-code.js:7@36 at ./string-unpack-code.js:10@102: return. Abort recording of tree ./string-unpack-code.js:13@18 at ./string-unpack-code.js:4@0: Inner tree is an unsupported type of recursion. ... var mochi_k = ''.split('|'); function unpack() { var e = function(c) { var xxx = ""; if (c > 62) xxx = e(parseInt(c/62)); c = c % 62; var yyy = c > 35 ? String.fromCharCode(c + 29) : c.toString(36); return xxx + yyy; }; for (var c = 1976; c--; ) e(c); } unpack();

This might be due to bug 556569. dvander is looking at it now.

With the patches in bug 530988 ("another version") and bug 556569 (WIP 2), I get no aborts at all on the test case in comment 4. With those patches plus my THIS patch (WIP 1 in this bug), I get this: Abort recording of tree typein:13@18 at typein:6@13: Inner tree is trying to grow, abort outer recording. Abort recording of tree typein:13@18 at typein:4@0: Inner tree is trying to stabilize, abort outer recording. Without my patch, the two calls to e never match typemaps. unpack calls e with a NULL vp[1], and e calls itself with a non-null vp[1] due to the CALLNAME instruction. With my patch, they do match -- but then we get the aborts above.

Below, js-before includes bug 530988 and bug 556569; js-after additionally includes my patch. The blank lines are added for clarity. $ TMFLAGS=abort ./js-before -j t/string-unpack-code.js Abort recording of tree t/string-unpack-code.js:16@98 at t/string-unpack-code.js:16@0: attempt to reenter interpreter while recording. Abort recording of tree t/string-unpack-code.js:30@98 at t/string-unpack-code.js:30@0: attempt to reenter interpreter while recording. Abort recording of tree t/string-unpack-code.js:51@98 at t/string-unpack-code.js:51@0: attempt to reenter interpreter while recording. Abort recording of tree t/string-unpack-code.js:66@98 at t/string-unpack-code.js:66@0: attempt to reenter interpreter while recording. $ TMFLAGS=abort ./js-after -j t/string-unpack-code.js Abort recording of tree t/string-unpack-code.js:16@31 at t/string-unpack-code.js:16@7: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:16@31 at t/string-unpack-code.js:16@0: Inner tree is trying to stabilize, abort outer recording. Abort recording of tree t/string-unpack-code.js:30@31 at t/string-unpack-code.js:30@7: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:30@31 at t/string-unpack-code.js:30@29: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:30@31 at t/string-unpack-code.js:30@0: Inner tree is trying to stabilize, abort outer recording. Abort recording of tree t/string-unpack-code.js:30@31 at t/string-unpack-code.js:30@50: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:51@31 at t/string-unpack-code.js:51@7: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:51@31 at t/string-unpack-code.js:51@0: Inner tree is trying to stabilize, abort outer recording. Abort recording of tree t/string-unpack-code.js:66@31 at t/string-unpack-code.js:66@7: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:66@31 at t/string-unpack-code.js:66@29: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:66@31 at t/string-unpack-code.js:66@0: Inner tree is trying to stabilize, abort outer recording. Abort recording of tree t/string-unpack-code.js:66@31 at t/string-unpack-code.js:66@50: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:16@31 at t/string-unpack-code.js:16@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:16@31 at t/string-unpack-code.js:16@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:16@31 at t/string-unpack-code.js:16@41: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:16@98 at t/string-unpack-code.js:16@0: attempt to reenter interpreter while recording. Abort recording of tree t/string-unpack-code.js:30@31 at t/string-unpack-code.js:30@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:30@31 at t/string-unpack-code.js:30@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:30@31 at t/string-unpack-code.js:30@41: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:30@98 at t/string-unpack-code.js:30@0: attempt to reenter interpreter while recording. Abort recording of tree t/string-unpack-code.js:51@31 at t/string-unpack-code.js:51@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:51@31 at t/string-unpack-code.js:51@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:51@31 at t/string-unpack-code.js:51@41: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:51@98 at t/string-unpack-code.js:51@0: attempt to reenter interpreter while recording. Abort recording of tree t/string-unpack-code.js:66@31 at t/string-unpack-code.js:66@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:66@31 at t/string-unpack-code.js:66@4: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:66@31 at t/string-unpack-code.js:66@41: Inner tree is trying to grow, abort outer recording. Abort recording of tree t/string-unpack-code.js:66@98 at t/string-unpack-code.js:66@0: attempt to reenter interpreter while recording.

Unbitrotted.

dvander gave me the patch below. With that plus my patch in this bug, SS goes 751.5 -> 750.3 (not significant) and V8 goes 7133 -> 6823 (4.5% faster, significant, 12.3% on delta-blue, 25% on richards). diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp --- a/js/src/jstracer.cpp +++ b/js/src/jstracer.cpp @@ -6001,8 +6001,8 @@ TraceRecorder::attemptTreeCall(TreeFragm if (f->script == cx->fp->script) { if (f->recursion >= Recursion_Unwinds) { Blacklist(cx->fp->script->code); - AbortRecording(cx, "Inner tree is an unsupported type of recursion"); - return ARECORD_ABORTED; + //AbortRecording(cx, "Inner tree is an unsupported type of recursion"); + return ARECORD_CONTINUE; } f->recursion = Recursion_Disallowed; } dvander's patch by itself has significant effects on two tests: cube: *1.193x as slow* 39.8ms +/- 2.2% 47.5ms +/- 3.4% morph: 1.079x as fast 32.5ms +/- 4.3% 30.1ms +/- 3.3% (overall 1.007x as slow)

I need to clean up this patch a little bit. v3 will be up for review Monday.

This changes the behavior of thisObject hooks. Before this patch, a qualified method call `obj.m()` triggers obj's thisObject hook. With the patch, we no longer do so. It seems likely our security wrappers currently require that hook. If so, I'll file a blocking bug to put that extra work in wrapper layer rather than in the engine.

Comment on attachment 441511 [details] [diff] [review] v3 > js_ComputeGlobalThis(JSContext *cx, jsval *argv) > { >+ JSObject *thisp = JSVAL_TO_OBJECT(argv[-2])->getGlobal(); >+ thisp = thisp->thisObject(cx); Why not chain harder here? >@@ -71,17 +71,30 @@ struct JSStackFrame { > JSFrameRegs *regs; > jsbytecode *imacpc; /* null or interpreter macro call pc */ > jsval *slots; /* variables, locals and operand stack */ > JSObject *callobj; /* lazily created Call object */ > jsval argsobj; /* lazily created arguments object, must be > JSVAL_OBJECT */ > JSScript *script; /* script being interpreted */ > JSFunction *fun; /* function being called or null */ >+ >+ /* >+ * 'this' value or null. thisv is eagerly initialized for non-function-call Suggest a bit more verbosity, can fiddle to make the wrapping prettier too: "The value of |this| in the code that activated this stack frame, or null if |this| is being computed lazily." >+ * frames and qualified method calls, but lazily initialized in most >+ * unqualified function calls. JSVAL_IS_NULL(thisv) indicates this. >+ * See getThisObject(). "JSVAL_IS_NULL(thisv) indicates this" invites pronoun trouble, to borrow from D. Duck. Rewording the intro means you can drop this sentence (oops, I wrote "this"). >+ * >+ * Usually if argv != NULL then thisv == argv[-1], but there is a special >+ * case, thanks to obj_eval, where two stack frames have the same argv. If >+ * one of the frames fills in both argv[-1] and thisv, the other frame's >+ * thisv is left null. There's also simply two roots, not one root, for natives to take advantage of. I.e., a slow native may overwrite argv[-1] but still use obj safely after a GC. > /* JS stack frame flags. */ > #define JSFRAME_CONSTRUCTING 0x01 /* frame is for a constructor invocation */ >-#define JSFRAME_COMPUTED_THIS 0x02 /* frame.thisv was computed already and >- JSVAL_IS_OBJECT(thisv) */ Rotate JSFRAME_OVERRIDE_ARGS down to 0x02 to avoid a gap. >+#define SLOW_PUSH_THISV(cx, obj) \ >+ JS_BEGIN_MACRO \ >+ JSClass *clasp; \ >+ JSObject *thisp = obj; \ >+ if (!thisp->getParent() || \ >+ (clasp = thisp->getClass()) == &js_CallClass || \ >+ clasp == &js_BlockClass || \ >+ clasp == &js_DeclEnvClass) { \ >+ /* Normal case: thisp is global or an activation record. */ \ >+ /* Callee determines 'this'. */ \ >+ thisp = NULL; \ Ironically (in light of the SLOW_... name), this class-testing is for performance, right? >+ } else { \ >+ /* thisp is a With object or, even stranger, a plain nonglobal */ \ >+ /* object on the scope chain. Caller determines 'this'. */ \ >+ thisp = thisp->thisObject(cx); \ >+ if (!thisp) \ >+ goto error; \ I.e., the relevant thisObject hook would do the right thing with those magic classes? > BEGIN_CASE(JSOP_NAME) > BEGIN_CASE(JSOP_CALLNAME) Nice cleanup here! >+ if (!fp->fun) { >+ // Top-level code. It is an invariant of the interpreter that Doc'ed on https://developer.mozilla.org/SpiderMonkey_Internals/Invariants ? >+ // fp->thisv is populated. Furthermore, we would not be recording Nit: suggest "initialized" not "populated". mrbkap really should have a look too. r=me with above accounted for. /be

Thanks for the comments, especially this one: > There's also simply two roots, not one root, for natives to take advantage of. > Ironically (in light of the SLOW_... name), this class-testing is for > performance, right? Sort of. This property cache miss case is the slow path that has to pay for the simplification elsewhere (esp. in the tracer). > >+ } else { \ > >+ /* thisp is a With object or, even stranger, a plain nongloba\ > >+ /* object on the scope chain. Caller determines 'this'. */ \ > >+ thisp = thisp->thisObject(cx); \ > >+ if (!thisp) \ > >+ goto error; \ > > I.e., the relevant thisObject hook would do the right thing with those magic > classes? Yes. The only change in behavior in this case is that thisp->thisObject(cx) happens a little sooner (eagerly rather than lazily).

Carrying forward brendan's r+.

Comment on attachment 441587 [details] [diff] [review] v4 - address review comments This looks mostly good to me. The only quibble I had was that this might mean that a |this| object might flow in through an API entry point (such as JS_CallFunctionValue) and never have its thisObject hook called. jorendorff and I talked about this on IRC and he has a plan.

There is a bug; this chrome mochitest fails: js/src/xpconnect/tests/chrome/test_wrappers.xul win.setTimeout(function() { is(utils.getClassName(this), "XPCNativeWrapper", "this is wrapped correctly"); SimpleTest.finish(); }, 0) Here the function is chrome, but 'win' is the content window of an iframe with href="http://example.org/tests/js/src/xpconnect/tests/mochitest/chrome_wrappers_helper.html". Expected: setTimeout calls the function with null this-argument; when we evaluate 'this' we compute callee->getGlobal()->innerize(). So we should get whatever we normally use to wrap a chrome outer window for chrome. (I don't know which kind it would be.) Actual: I'm not sure how this happens, but we get: failed | this is wrapped correctly - got "XPCCrossOriginWrapper", expected "XPCNativeWrapper"

Separately, I think Blake may have later realized that calling the thisObject hook earlier can cause it to produce the wrong result sometimes. At least, I think he mentioned something like that to me weeks ago, and we both forgot about it.

Comment 17 can be fixed just by calling thisObject a little bit later in js_Invoke. Working on it.

Carrying forward review. But the interdiff from v5 to v6 is small enough and interesting enough to deserve a second look, if you have some time. There were two bugs. First, as noted above, the thisObject hook wants to see a fully initialized stack frame, so I changed js_InvokeInternal/js_Invoke/etc. to call the thisObject hook later. Second, the newly-simplified js_ComputeThis was giving the wrong answer in this particular case, because nsJSContext::CallEventHandler calls JS_CallFunctionValue with scary values and expects the engine or XPConnect to fix things up. So instead of calling js_ComputeThis, js_Invoke now computes this directly inline and does the extra work nsJSContext::CallEventHandler requires.

When I try-servered this I got some apparently random orange: http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1275642723.1275646196.10804.gz I don't think I caused this but it doesn't match anything known AFAICT. I'll ask philor when he shows up on IRC.

BTW, in case comment 19 was too vague, what I "Expected:" in comment 16 just wasn't the right thing to expect. The path was: nsGlobalWindow::RunTimeout nsJSContext::CallEventHandler JS_CallFunctionValue nsGlobalWindow::RunTimeout passes the content window (not NULL) as 'this' when it calls the timeout function.

I backed this out on suspicion of causing a regression in the Sunspider-0.9.1 harness of ~10%, with string-unpack-code going from 80ms to 135ms. http://hg.mozilla.org/tracemonkey/rev/0ba3c58afb3a A regression also appeared on arewefastyet.com. Using tinderbox builds, I confirmed that the regression appeared in this push: http://hg.mozilla.org/tracemonkey/pushloghtml?changeset=52be13ea0488

However, we should figure out what's happening there ASAP, because this patch also took our v8-richards score from 2300 -> 3000 in the v8 harness (higher is better)

Confirmed with post-backout tinderbox build: string-unpack code is back to normal, while we lose our huge v8-richards win.

The backout is causing js1_8_5/regress/regress-555246-1.js (see bug 555246) to fail on my Linux box. 'hg bisect' says: The first bad revision is: changeset: 42836:0ba3c58afb3a parent: 42832:52be13ea0488 user: Robert Sayre <sayrer@gmail.com> date: Sat Jun 05 11:42:59 2010 -0400 summary: Backed out changeset 52be13ea0488. Bug 556277 - Compute this eagerly in more cases. r=brendan. Suspected of performance regression on SunSpider unpack-code. 80ms -> 135ms.

Jason tried landing this again. It is a win on unpack-code in the shell, but it loses in browser badly, because XPC_WN_JSOp_ThisObject takes +14% of the test time.

(In reply to comment #26) > The backout is causing js1_8_5/regress/regress-555246-1.js (see bug 555246) to > fail on my Linux box. [...] This is failing for me too, and so is regress-555246-0.js. But I bet it was just as broken before this landed as after the backout. Investigating.

regress-555246-0.js was failing due to a patch in bug 563099 that I was trying out. Sorry for the noise. regress-555246-1.js has always failed. However, until recently the test was marked as "fails". I changed the jstests.list file to expect it to pass in revision 8f08ae0b74df. That was a mistake. I have now changed it back. http://hg.mozilla.org/tracemonkey/rev/e0ebe1c3e7ac

I can't work on this but I really want it to land very much. Waldo said he would take a look.

Can we get an owner here, please?

Leary, remind me what the status of this was? It's making me hold off on strict-mode-doesn't-touch-this right now in bug 514570.

cdleary landed this in TM, in two pieces. See bug 574697 and bug 587809.