Currently the SAVE_SP_AND_PC macro that must be called before any function call from js_Interpret that can potentially trigger GC or call js_Interpret recursively is defined like: #define SAVE_SP_AND_PC(fp) (asserts, (fp)->sp = sp, (fp)->pc = pc) In this form the macro requires to load fp into a register since from the native stack in most cases in the interpreter this will be the first usage of fp in the interpreter case. It is possible to avoid this extra load if JSStackFrame would not contain sp and pc directly but rather it would hold a pointer to a regs struct holding sp and pc. Such struct can be put on the native stack allowing to implement SAVE_SP_AND_PC like in #define SAVE_SP_AND_PC(fp) (asserts, (regs.sp = sp, regs.pc = pc) which avoids an extra load. Such arrangement may also allow in principle to eliminates sp and pc completely together with SAVE_SP_AND_PC, but that would require for a compiler to figure out that the best thing to do is always load after a function call regs.sp and regs.sp into the internal register.

Debuggers and resolve hooks and such need to find cx->fp->pc, e.g. I have a more radical proposal: get rid of the pc and sp locals in the interpreter and let the compiler (under strict aliasing rules) optimize. Then we never have the bug of a missing SAVE. But, we do need the compiler to optimize to avoid gratuitous reloads. /be

(In reply to comment #1) > Debuggers and resolve hooks and such need to find cx->fp->pc, e.g. They still will be able to find pc and sp using cx->fp->regs->(pc, sp) where regs point to a structure placed on a native stack. > > I have a more radical proposal: get rid of the pc and sp locals in the > interpreter and let the compiler (under strict aliasing rules) optimize. Then > we never have the bug of a missing SAVE. But, we do need the compiler to > optimize to avoid gratuitous reloads. Theoretically a good compiler can notice that fp->sp is used very often and load fp->sp into a register after calling a method. But first it would need to load fp into a register to access sp and pc. With the proposed change the compiler would need to notice that only regs.sp is used very often and after a method calls it would not need to reload regs as regs is equivalent to native_sp+some_offset. What this proposal effectively means is an extra level of indirection when accessin sp and pc outside js_Interpret with one less level of indirection when accessing them in the interpreter loop.

Would be good to rule out the possible win of the more radical proposal, which is also winning in terms of source complexity. I may be dreaming, and I have small hopes for gcc, but MSVC is a different animal, and ICC beckons. /be

(In reply to comment #3) > Would be good to rule out the possible win of the more radical proposal, which > is also winning in terms of source complexity. I may be dreaming, and I have > small hopes for gcc, but MSVC is a different animal, and ICC beckons. IMO the source complexity of fp->pc is higher then regs.pc (or fr.pc aka frame register) as one has to think why fp is not null here. I.e. any extra indirection means extra source complexity for me. So the total complexity would drop as changing fp->pc into fp->regs->pc would be done in small number of places. But I can create patches to try both approaches.

Quick responses: - 'regs' is the winning name. - fp is non-null by construction, clueful hackers (who else would touch the JS interpreter? :-P) know htis. - performance is king, you are probably right about avoiding fp loads and spills. - The failure-to-SAVE hazard has been annoying, hasn't it? So long as we fix this and improve perf I'm happy. /be

Here is a patch that is build on top of the patch from the bug 419632 comment 11. It eliminates SAVE_SP_AND_PC and friends. Instead sp and pc is stored in JSFrameRegs struct allocated on the stack with JSStackFrame.regs hoolding a pointer to it. The biggest complexity in the patch comes from JSStackFrame.regs management for non-current frames. The patch passes shell and mochi tests.

We don't have unit tests for it yet, but the debugger code pokes at frames pretty heavily -- could you test Firebug in a patched build a bit?

With the default -Os option the patch slows down things by 1%. Still the combined effect of this patch and the patch from bug 419632 provides the total win over the trunk of about 0.5%.

With a build compiled at -O3 the situation with the patch reverses: it shows 4% gain on the code without the patch. It seems that GCC 4.1 only at -O3 can do the proper job of eliminating redundant JSFrameRegs updates.

(In reply to comment #7) > We don't have unit tests for it yet, but the debugger code pokes at frames > pretty heavily -- could you test Firebug in a patched build a bit? > Do you have particular tests in mind? It seems working in a debug build normally.

Comment on attachment 308156 [details] [diff] [review] v1 (quilt patch) + * From this point the control must flow through the label exit2. Nit: s/the control/control/ Great to see this patch go in. /be

Comment on attachment 308156 [details] [diff] [review] v1 (quilt patch) r+ carries over to updated final patch. /be

The changes for this bug conflicts with changes to fix bug 421806. I prefer to land the latter first as the bug is a regression.

The new version addresses the nit from the comment 11 and merges with the proposed fix for the bug 421806. I will ask for a1.9? after landing that.

This is a normal CVS diff generated after landing the bug 421806. Asking for 1.9 approval since the patch eliminates a particular class of GC hazards that plagued the interpreter from time to time. The patch may also improve performance with a good compiler.

Comment on attachment 308997 [details] [diff] [review] v2 (cvs diff) a+ schrep.

I plan to check in this tomorrow morning European time.

I checked in the patch from the comment 15 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205387520&maxdate=1205387522&who=igor%25mir2.org

The new version with extra changes so the code compiles with !JS_THREADED_INTERP plus compilation fixes for dtrace.

The previous patch has not included all the files.

I backed out the checked in patch from the comment 15 due to compilation errors on Windows.

I checked in the patch from the comment 21 to the trunk: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1205390874&maxdate=1205390943&who=igor%25mir2.org

DEBUG && !JS_THREADSAFE shell was busted without this. Committed: js/src/jsinterp.c 3.475 /be

This causes startup crash (at least) on my 64bit-linux, both debug and opt. #0 0x00000032d7097581 in nanosleep () from /lib64/libc.so.6 #1 0x00000032d70973a4 in sleep () from /lib64/libc.so.6 #2 0x00002aaaaaae5539 in ah_crap_handler (signum=11) at nsSigHandlers.cpp:149 #3 <signal handler called> #4 js_Interpret (cx=0x6851a0) at /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/jsinterp.c:2883 #5 0x00002aaaaad6c933 in js_Execute (cx=0x6851a0, chain=0x7c0ff0, script=<value optimized out>, down=0x0, flags=2048, result=0x7fffba409680) at /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/jsinterp.c:1513 #6 0x00002aaaaad26900 in JS_ExecuteScript (cx=0x6851a0, obj=0x76df40, script=0x7b5b20, rval=0x7fffba409680) at /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/jsapi.c:4865 #7 0x00002aaab0224b1a in mozJSComponentLoader::GlobalForLocation (this=0x675610, aComponent=0x6b5150, aGlobal=0x787418, aLocation=0x787420) at /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp:1270 #8 0x00002aaab0227492 in mozJSComponentLoader::LoadModule (this=0x675610, aComponentFile=0x6b5150, aResult=0x7fffba409a70) at /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp:607 #9 0x00002aaaab265d38 in nsComponentManagerImpl::AutoRegisterComponent (this=0x67d2e0, aComponentFile=0x6b5150, aDeferred=@0x7fffba409c20, minLoader=0) at /home/smaug/mozilla/mozilla_cvs/mozilla/xpcom/components/nsComponentManager.cpp:2988 #10 0x00002aaaab265fcd in nsComponentManagerImpl::LoadLeftoverComponents (this=0x67d2e0, aLeftovers=@0x7fffba409c30, aDeferred=@0x7fffba409c20, minLoader=0) at /home/smaug/mozilla/mozilla_cvs/mozilla/xpcom/components/nsComponentManager.cpp:3043 #11 0x00002aaaab269ef2 in nsComponentManagerImpl::AutoRegister (this=0x67d2e0, aSpec=0x0) at /home/smaug/mozilla/mozilla_cvs/mozilla/xpcom/components/nsComponentManager.cpp:3301 #12 0x00002aaaab233593 in NS_InitXPCOM3_P (result=0x7fffba409d28, binDirectory=<value optimized out>, appFileLocationProvider=<value optimized out>, staticComponents=0x2aaaaad090e0, componentCount=<value optimized out>) at /home/smaug/mozilla/mozilla_cvs/mozilla/xpcom/build/nsXPComInit.cpp:655 #13 0x00002aaaaaae64f6 in ScopedXPCOMStartup::Initialize (this=0x7fffba409fb0) at /home/smaug/mozilla/mozilla_cvs/mozilla/toolkit/xre/nsAppRunner.cpp:960 #14 0x00002aaaaaae665c in ProfileLockedDialog (aProfileDir=0x66c650, aProfileLocalDir=0x7c0ff0, aUnlocker=0x7baac0, aNative=0x7c0ff0, aResult=0x6851a0) at /home/smaug/mozilla/mozilla_cvs/mozilla/toolkit/xre/nsAppRunner.cpp:1625 #15 0x00002aaaaaaea53e in XRE_main (argc=<value optimized out>, argv=<value optimized out>, aAppData=<value optimized out>) at /home/smaug/mozilla/mozilla_cvs/mozilla/toolkit/xre/nsAppRunner.cpp:2003 #16 0x0000000000400eb9 in main (argc=3, argv=0x0) at /home/smaug/mozilla/mozilla_cvs/mozilla/browser/app/nsBrowserApp.cpp:158

(In reply to comment #25) > This causes startup crash (at least) on my 64bit-linux, both debug and opt. > #4 js_Interpret (cx=0x6851a0) at > /home/smaug/mozilla/mozilla_cvs/mozilla/js/src/jsinterp.c:2883 I can not debug this since I do not have access to 64 bit box. So here is questions: 1. Do you have any extension installed? 2. What values are in ifp->callerRegs.pc, (JSOp*)*ifp->callerRegs.pc, ifp->callerRegs.sp, fp->down->spbase and fp->argc when this crashes.

If you still have an account on sm-valgrind01, it's 64-bit.

(In reply to comment #27) > If you still have an account on sm-valgrind01, it's 64-bit. > The trouble is that I was not able to run a browser from it that I can access remotely either via VNC or running direct X11 tunnel, but I could run xpshell. To Smaug: does xpcshell crash with the same stack?

(gdb) p ifp->callerRegs.pc $1 = (jsbytecode *) 0x7bf89e ":" (gdb) p (JSOp*)*ifp->callerRegs.pc $2 = (enum JSOp *) 0x3a (gdb) p ifp->callerRegs.sp $3 = (jsval *) 0x7bcb68 (gdb) p fp->down->spbase $4 = (jsval *) 0x7adb68 (gdb) p fp->argc $5 = 1 (gdb) p fp->script->filename $14 = 0x7bf8dd "file:///home/smaug/mozilla/mozilla_cvs/mozilla/ff_build/dist/bin/modules/XPCOMUtils.jsm" (gdb) p fp->script->lineno $15 = 114 http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/xpconnect/loader/XPCOMUtils.jsm&rev=1.9&mark=114#107 (gdb) p regs $17 = {pc = 0x7bf89e ":", sp = 0x8007bcb58} I have DOMi and XForms extensions compiled. xpcshell does crash too, same file and lineno (gdb) p fp->script->filename $3 = 0x779b9d "file:///home/smaug/mozilla/mozilla_cvs/mozilla/ff_build/dist/bin/modules/XPCOMUtils.jsm" (gdb) p fp->script->lineno $4 = 114 The last debug output before the crash is: *** Registering components in: mozSpellCheckerModule

(gdb) p fp $1 = (JSStackFrame *) 0x7ca4c8 (gdb) p fp->down $2 = (JSStackFrame *) 0x7ffff19d5a70 (gdb) p fp->down->script $3 = (JSScript *) 0x7bdb50 (gdb) p fp->down->script->filename $4 = 0x7b898d "file:///home/smaug/mozilla/mozilla_cvs/mozilla/ff_build/dist/bin/components/nsDownloadManagerUI.js" (gdb) p fp->down->script->lineno $5 = 1

It seems this is a GCC bug. The crash happens on the line 2883 of the following fragment from jsinterp.c, js_Interpret, JSOP_RETURN case: 2880 /* Store the return value in the caller's operand frame. */ 2881 regs = ifp->callerRegs; 2882 regs.sp += 1 - 2 - ifp->frame.argc; 2883 regs.sp[-1] = fp->rval; I replace the line 2880 by: memcpy(&regs, &ifp->callerRegs, sizeof(regs)) then xpcshell starts normally without the crash. Without the memcpy the value of reg.sp gets ifp->callerRegs.sp with the highest bit set.

(In reply to comment #31) > Without the memcpy the value > of reg.sp gets ifp->callerRegs.sp with the highest bit set. ^^^^^^^^^^ I meant the bit 32.

As discussed on IRC, that doesn't help. Still crashing

I think this is it. The code was not 64-bit clean.

That patch doesn't work. XPCShell starts but browser crashes during startup.

Comment on attachment 309143 [details] [diff] [review] fix for 64bit Argh, I fixed a 64-bit impurity like this many years ago. Wouldn't (int) work just as well? (ptrdiff_t) suggests byte pointer difference, but that's not what is happening here. /be

I checked in that small patch. I will check for an hour or so for more bugs. If nothing comes out, I will back out.

That patch seems to work for me (I didn't get a browser crash up to now).

I backed out the patch and the follow fixes.

This is what was landed + follow ups.

Do you still need the cast now that you're doing -=? As I understand the bug, |1 - 2 - argc| is evaluated in a 32-bit unsigned context, so some really big number (negative in 2's complement) gets added to the pointer, which ends up with the right value if the pointer is 32 bits, but with a value (1 << 32) too high if it's 64 bits. The change to subtract a positive number should be enough to address that, no?

(In reply to comment #41) > The change to subtract a positive number should be enough to address that, no? I want the cast for the sake of clarity and hinting of possible overflow problems.

(In reply to comment #38) > That patch seems to work for me (I didn't get a browser crash up to now). > Which patch has worked for you?

With the patch from comment 40 the test suite for js shell passes on 64 bit Linux. I can not verify it with the browser as it refuses to run under VNC server due to some fonts issues.

I suggest re-landing, unless Smaug can come back with more traces of problems from a Firefox build. /be

BTW, I still don't like (ptrdiff_t) and prefer (int) -- Igor, did I miss your reply on this point? /be

(In reply to comment #42) > (In reply to comment #41) > > The change to subtract a positive number should be enough to address that, no? > > I want the cast for the sake of clarity and hinting of possible overflow > problems. Shouldn't we prefer comments for cases like this? Less casting is better, in my opinion, as it allows the compiler to warn (or error) in cases where the cast might mask a serious issue.

Sure, comment wanted, cast optional but ptrdiff_t says byte-address-delta to me (maybe I am peculiar that way). /be

Comment on attachment 309160 [details] [diff] [review] v4 Is this patch the original patch + attachment 309143 [details] [diff] [review]? Then I tested that and browser did crash during startup as I said earlier.

Crashed where? Probably the best thing to do is both you guys (who are in roughly the same time zone ;-) get on IRC to do remote debugging the slow way. /be

The v4 patch isn't the original patch + attachment 309143 [details] [diff] [review] after all? V4 seems to work here.

It looks like v4 doesn't incorporate the v2-v3 delta (!JS_THREADED_INTERP change)

changes*

The new version adds the missing changes from the comment 20 that I missed in v4 and use the following cast: /* Store the return value in the caller's operand frame. */ regs.sp -= 1 + (size_t) ifp->frame.argc;

Comment on attachment 309336 [details] [diff] [review] v5 >@@ -125,49 +125,46 @@ jsdtrace_jsvaltovoid(JSContext *cx, jsva > } > /* NOTREACHED */ > } > > char * > jsdtrace_function_name(JSContext *cx, JSStackFrame *fp, JSFunction *fun) > { ... >+ JS_ASSERT(fp->argv == regs->sp - (int)GET_ARGC(pc)); > /* >- * Be paranoid about bugs to-do with generating pc storage when >- * attempting to descend into the operand stack basement. >+ * Odd to have a leading blank comment-line. Better to put a real blank line after the new assertion and before the major comment. >+ * FIXME bug 422864: update this code to use the pc stack from the >+ * decompiler. > */ >- if ((uintptr_t)(pc - script->code) >= script->length) >- return dempty; > break; > } > > switch ((JSOp) *pc) { > case JSOP_CALLNAME: > case JSOP_CALLPROP: > case JSOP_NAME: > case JSOP_SETNAME: r=me with that nit picked -- hope this is it (except for the dtrace followup bug, which is a 1.9 blocker). /be

(In reply to comment #43) > (In reply to comment #38) > > That patch seems to work for me (I didn't get a browser crash up to now). > > > > Which patch has worked for you? What worked for me was to apply your patch in attachment 309143 [details] [diff] [review] (comment 34) on top of the trunk at that time. I didn't get the browser crash Smaug mentioned in comment 35.

(In reply to comment #57) > Created an attachment (id=309487) [details] > v5b The patch fixes the nit from the comment 55.

Comment on attachment 309487 [details] [diff] [review] v5b a1.9=beltzner

I checked in the patch from the comment 58 to the trunk: Checking in dom/src/base/nsDOMClassInfo.cpp; /cvsroot/mozilla/dom/src/base/nsDOMClassInfo.cpp,v <-- nsDOMClassInfo.cpp new revision: 1.539; previous revision: 1.538 done Checking in js/src/js.c; /cvsroot/mozilla/js/src/js.c,v <-- js.c new revision: 3.207; previous revision: 3.206 done Checking in js/src/jsapi.c; /cvsroot/mozilla/js/src/jsapi.c,v <-- jsapi.c new revision: 3.433; previous revision: 3.432 done Checking in js/src/jscntxt.c; /cvsroot/mozilla/js/src/jscntxt.c,v <-- jscntxt.c new revision: 3.135; previous revision: 3.134 done Checking in js/src/jsdbgapi.c; /cvsroot/mozilla/js/src/jsdbgapi.c,v <-- jsdbgapi.c new revision: 3.135; previous revision: 3.134 done Checking in js/src/jsdtracef.c; /cvsroot/mozilla/js/src/jsdtracef.c,v <-- jsdtracef.c new revision: 3.5; previous revision: 3.4 done Checking in js/src/jsexn.c; /cvsroot/mozilla/js/src/jsexn.c,v <-- jsexn.c new revision: 3.107; previous revision: 3.106 done Checking in js/src/jsfun.c; /cvsroot/mozilla/js/src/jsfun.c,v <-- jsfun.c new revision: 3.269; previous revision: 3.268 done Checking in js/src/jsgc.c; /cvsroot/mozilla/js/src/jsgc.c,v <-- jsgc.c new revision: 3.295; previous revision: 3.294 done Checking in js/src/jsinterp.c; /cvsroot/mozilla/js/src/jsinterp.c,v <-- jsinterp.c new revision: 3.478; previous revision: 3.477 done Checking in js/src/jsinterp.h; /cvsroot/mozilla/js/src/jsinterp.h,v <-- jsinterp.h new revision: 3.87; previous revision: 3.86 done Checking in js/src/jsiter.c; /cvsroot/mozilla/js/src/jsiter.c,v <-- jsiter.c new revision: 3.99; previous revision: 3.98 done Checking in js/src/jsiter.h; /cvsroot/mozilla/js/src/jsiter.h,v <-- jsiter.h new revision: 3.30; previous revision: 3.29 done Checking in js/src/jsobj.c; /cvsroot/mozilla/js/src/jsobj.c,v <-- jsobj.c new revision: 3.453; previous revision: 3.452 done Checking in js/src/jsopcode.c; /cvsroot/mozilla/js/src/jsopcode.c,v <-- jsopcode.c new revision: 3.304; previous revision: 3.303 done Checking in js/src/jsstr.c; /cvsroot/mozilla/js/src/jsstr.c,v <-- jsstr.c new revision: 3.205; previous revision: 3.204 done Checking in js/src/jsxml.c; /cvsroot/mozilla/js/src/jsxml.c,v <-- jsxml.c new revision: 3.205; previous revision: 3.204 done Checking in js/src/liveconnect/nsCLiveconnect.cpp; /cvsroot/mozilla/js/src/liveconnect/nsCLiveconnect.cpp,v <-- nsCLiveconnect.cpp new revision: 1.49; previous revision: 1.48 done

Have we got to a point where we can measure the performance impact? thanks, - moh

tsspider did drop a bit http://graphs.mozilla.org/graph.html#spst=range&spstart=0&spend=1205745885&bpst=cursor&bpstart=0&bpend=1205745885&m1tid=151821&m1bl=0&m1avg=0

There is a negative impact on my Mac Mini PowerPC G4 1.25 GHz: Sunspider total was 9845.0ms on the last hourly build before landing (Build 20080317_0143), it became 10738.4ms on the first build after (Build 20080317_0216) and later builds.

(In reply to comment #63) > There is a negative impact on my Mac Mini PowerPC G4 1.25 GHz: > Sunspider total was 9845.0ms on the last hourly build before landing (Build > 20080317_0143), it became 10738.4ms on the first build after (Build > 20080317_0216) and later builds. It indicates that GCC 4.0 (Mac builds uses this version, right ?) even at -O3 can not do the proper job of eliminating redundant register loads with Power PC. I guess we have to live with this since the bug is not only about performance but also about eliminating a source of a particular kind of GC hazards.

I can not watch tinderbox myself before the freeze.

(In reply to comment #65) > I can not watch tinderbox myself before the freeze. > Please ifnore this, it is intended for another bug.