dormantNext takes up space in in every JSStackFrame even though it's only needed for a few. In pursuance of slimmer JSStackFrames, this patch keeps the list in the JSContext and removes the dormantNext member. TSS is unaffected. Also, js_Execute is pretty messy with all its exit paths, so I RAII'd that up. This fixes, I believe, two latent bugs: 1. The second "goto out" does not js_FreeRawStack 2. the "if (!chain)" returns instead of going through out2 There is one issue with the patch: in two places, dormantStacks.append(), which can OOM, is called from the jsapi functions (JS_GetFrameThis, JS_SaveFrameChain) that have an infallible API. OOM seems extraordinarily unlikely, and both functions have quite a few callers just in Mozilla, so I put in an abort.

The abort-on-OOM thing was weird. This patch avoids that by adding a new type JSCallStack which describes a set of JSStackFrames. JSCallStacks are pushed and popped when the dormant chain would be modified and the chain of JSCallStacks replaces the chain of dormant stack frames. I belive JSCallStack can also be used to store the initial variables object, which might allow me to remove JSStackFrame::varobj. I'm going to see if that actually works before trying to land this.

The callstack, liked by fp->down, aka the dynamic link, has nothing to do with scope (no dynamic scope in JS, ignoring eval and with, which have constraints that make them unlike dynamic scope in LISP or Perl or sundry other languages). So the variable object may be the same, or not, for each frame in a "callstack". The fp->varobj can be computed more slowly if necessary. I stored it there in the mid-nineties during some fast hacking. Since we optimize local variables to stack slots, and globals to global object slots (gvars), it is high time to examine my old field-expedient design decision. Good idea to factor out dormantFoo, though ;-). /be

(In reply to comment #2) > The callstack, liked by fp->down, aka the dynamic link, has nothing to do with > scope (no dynamic scope in JS, ignoring eval and with, which have constraints > that make them unlike dynamic scope in LISP or Perl or sundry other languages). > So the variable object may be the same, or not, for each frame in a > "callstack". Not the "variables object", just the "initial variables object", viz, the one that would be assigned to JSStackFrame::varobj of the first stack frame in the callstack. If I understand correctly (big if...) a particular stack frame's varobj is either the (lazily computed) callobj or this "initial variables object" (possibly copied from the down frame).

eval can change varobj too, to the global object for an indirect eval. See the hacky setCallerVarObj flag in obj_eval. /be

(In reply to comment #4) Heh, I just came across that, to my great dismay. Do you think it is possible, without too many changes, to achieve the same result by passing the varobj through parameters (to the compiler, js_Execute, et al called by obj_eval) instead of transmitting it through the stack frame?

(In reply to comment #5) > (In reply to comment #4) > > Heh, I just came across that, to my great dismay. Do you think it is possible, > without too many changes, to achieve the same result by passing the varobj > through parameters (to the compiler, js_Execute, et al called by obj_eval) > instead of transmitting it through the stack frame? That would be cleaner, and I hope no slower. Blake, any thoughts? /be

I was wondering the same thing as luke, actually. I've never really liked the hack of passing parameters to the compiler through the currently-active stack frame. IMO this is worth trying. If there are perf regressions from it, we'll deal with those when we get there.

Igor and I (mostly Igor) managed to fix the compiler not to use JSStackFrame: $ grep 'fp->' jsparse.cpp jsemit.cpp jsparse.cpp: * link (cx->fp/fp->down). They do not need to entrain and jsparse.cpp: * local slot's stack index from fp->spbase. jsemit.cpp: * declared via const or var, optimize pn to access fp->vars using the jsemit.cpp: * fp->vars slot after cg->ngvars and to protect regexp slots from GC we set jsemit.cpp: * fp->nvars to ngvars + nregexps. So that's not the issue. Is obj_eval just doing something it did for ages, which no longer needs to be done? /be

(In reply to comment #8) > Igor and I (mostly Igor) managed to fix the compiler not to use JSStackFrame: Cool! Perhaps more luck still: it seems like the write to caller->varobj (guarded by setCallerVarObj) is not observed by compileScript or js_Execute since staticLevel is set to 0 which causes callerFrame to later be set to NULL. My patch which asserts that recomputed-varobj == fp->varobj (at most fp->varobj use sites) passes trace-tests and ref-tests. Can you see any problems with just dropping this assignment to varobj?

Nope, think we've collectively proved that it's deadwood. /be

This patch removes varobj as discussed above. Try server is clean, so I think time for review. On Ubuntu, with tracing on, performance does not seem significantly modified on SS (some runs show slowdown (sharked to js_Interpret), but this seems to be noise), but with tracing off, there is a 1.8% speedup, which bodes well for JaegerMonkey.

Oops, left in some debugging deadbeef.

Comment on attachment 421281 [details] [diff] [review] remove debugging residue This is taking awhile, given that all this is beyond my traditional sphere of understanding. Posting a first bit so as not to hold up the entire thing: >diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h >+/* >+ * Wraps a stack frame which has been temporarily popped from its call stack >+ * and needs to be GC-reachable. See JSContext::{push,pop}GCReachableFrame. >+ */ >+struct JSGCReachableFrame >+{ >+ JSGCReachableFrame *next; >+ JSStackFrame *frame; >+ >+ JSGCReachableFrame(JSStackFrame *f) >+ : next(NULL), frame(f) {} >+}; I'm reading linearly, but my precog sense predicts an assignment to |next| which should be abstracted into a setter for cycle-detection (and maybe non-null-checking?) purposes. Also, if frame is never assigned to outside of the constructor (seems plausible) I'd make it * const. >diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp >--- a/js/src/jsdbgapi.cpp >+++ b/js/src/jsdbgapi.cpp >@@ -1214,40 +1214,37 @@ JS_GetFrameCallObject(JSContext *cx, JSS > * null returned above or in the #else > */ > return js_GetCallObject(cx, fp); > } > > JS_PUBLIC_API(JSObject *) > JS_GetFrameThis(JSContext *cx, JSStackFrame *fp) > { >- JSStackFrame *afp; >- > if (fp->flags & JSFRAME_COMPUTED_THIS) > return JSVAL_TO_OBJECT(fp->thisv); /* JSVAL_COMPUTED_THIS invariant */ > > /* js_ComputeThis gets confused if fp != cx->fp, so set it aside. */ >- if (js_GetTopStackFrame(cx) != fp) { >- afp = cx->fp; >+ JSStackFrame *afp = js_GetTopStackFrame(cx); >+ JSGCReachableFrame reachable(afp); >+ if (afp != fp) { > if (afp) { >- afp->dormantNext = cx->dormantFrameChain; >- cx->dormantFrameChain = afp; > cx->fp = fp; >+ cx->pushGCReachableFrame(reachable); > } > } else { > afp = NULL; > } > > if (fp->argv) > fp->thisv = OBJECT_TO_JSVAL(js_ComputeThis(cx, JS_TRUE, fp->argv)); > > if (afp) { > cx->fp = afp; >- cx->dormantFrameChain = afp->dormantNext; >- afp->dormantNext = NULL; >+ cx->popGCReachableFrame(); > } The pushing/popping here is not quite easily scrutable. I had to stare at it for fifteen seconds or so to be sure it was properly manipulating the reachable-frame stack. It looks like maybe it would be possible to push/pop via a guard object instead. The weirdness of only needing to push/pop in some circumstances but not others counsels adding a |bool saveFrame| argument that would determine whether the guard object did anything, making it almost a pseudo-guard object. I don't recall seeing guard objects of this nature before, but I think it's worth having one here.

So I didn't try to be very clean about JSGCReachableFrame code because its completely specific to the situation in js_ComputeGlobalThis/JS_GetFrameThis and, since the entire situation is going away with Blake's "blazingly fast" patches, JSGCReachableFrames can soon be ripped out (as added to the FIXME comment in js_ComputeGlobalThis). It felt like over-doing it to make a whole guard class that did what you were talking about (I do that elsewhere, though).

Comment on attachment 421281 [details] [diff] [review] remove debugging residue Looking more at this, should you even be conditioning the gc-reachable-frames bit at all? GC is rare. Extra work performed always, just to avoid tracing an extra frame every so often (moreover -- only when the GC happens to be nested beneath this-computation or exotic debugging code), doesn't seem to pass cost-benefit analysis. At that point you might as well just make JSGCReachableFrame always do the pushing/popping in its ctor/dtor, I think. (Alternately use the fine LazilyConstructed class I saw you working on last week and use that instead.) The definition of JSCallStack seems like it would be better in jscntxt.h adjacent to the location where JSCallStack is actually used. Is there a particular reason it's in jsinterp.h right now? I certainly would have been helped by its definition being by its use. In this function: void pushCallStack(JSCallStack *newcs) by the assertion in it, I'd like a comment to the effect that "We should have cleared the context's frame pointer if we suspended -- and if a frame has since become present we should have pushed a new call stack which doesn't have a suspended frame." >diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h >+struct JSCallStack >+{ >+ /* If this callstack is suspended, the top of the callstack. */ >+ JSStackFrame *suspendedFrame; This field, assuming I'm wrapping my head around it correctly, has two separate meanings, conditioned on whether this call stack is the current call stack and whether the frame chain has been saved. If the frame chain's been saved, this will contain the frame saved (previous frames accessible via |suspendedFrame->down|); also, |cx->fp == NULL|. If the chain hasn't been saved and !DEBUG, the contents are undefined; otherwise if DEBUG, then if this call stack is at the top it's NULL, and if this call stack isn't at top it's whatever frame was currently in use when a call stack was pushed atop this one. Is that correct? It makes my head spin to think through this. The assertions point toward this definition, but they don't clearly and concisely reify it. I would much prefer if this field and |saved| were moved adjacent to each other, |suspendedFrame| were made private, and all access to this field were gated through accessor functions that verified the correct meaning of the current value of the field at all times and recorded it on setting. (This meaning of |saved|, if the above definition is correct, makes it a tristate: SUSPENDED_FRAME, CURRENT_CALL_STACK, PREVIOUS_CALL_STACK.) I *think* the meaning is preserved by the assertions, but I'm not completely certain of it, and the lack of encapsulation now does much to reduce my confidence in them. >+ /* The variables object on entry to the down-most stack frame. */ >+ JSObject *initialVarObj; Rather than down-most (down being an ugly, ambiguous word when talking about stacks, regrettably; I remember having a discussion about maybe changing stack frame "down" nomenclature at least once before in the old office), use "...to the first stack frame in this call stack." "Variable object" (note lack of "s", which together with the ES3 spec's PDF suckage means it's even harder to look up) is actually an ES3ism. ES5 now uses "environment record" as the term here (distinguished as "declarative" for variable storage in functions/catch blocks and most lexical variable names, or "object" for |with| and global variables where the record wraps an object and stores values in it). I think this would correspond to the "global environment record", right? Seems worth pointing that out here if so, and I think the name should be something similar to that. Maybe globalEnvRecord? Suggestions welcome, but I'd like to avoid referring to obsolescent terms here (also point to an ES5 chapter/verse for the reader's reference). ES3 is dead, long live ES5, and while a transition period is inevitable, we should do our best to make the source understandable through reference only to ES5. >+ /* The next callstack in the list rooteded at JSContext::topCallStack. */ >+ JSCallStack *nextSuspended; Typo. "next" is also a not-happy-making description, in the same way that "down" is ugly for frames. Maybe "previous", because it was the one in place before this call stack? (The irony of instead using the exact opposite adjective is not lost on me.) I think I would get rid of the "suspended" suffix; it does little to help with understanding, in my opinion, and it's kind of verbose. While "topCallStack" is accurate, I think I would prefer "currentCallStack" instead to avoid even implying stack directionality. I think this also goes well with the field name "previous" as "what was current before this call stack". >diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp >-js_NewGenerator(JSContext *cx, JSStackFrame *fp) >+js_NewGenerator(JSContext *cx) Fewer arguments, nice... > { > JSObject *obj; > uintN argc, nargs, nslots; > JSGenerator *gen; > jsval *slots; > >+ JSStackFrame *fp = cx->fp; This should be preceded by JS_ASSERT_NOT_ON_TRACE(cx). That's the case for all callers right now, to be sure, but it might not always be, potentially. Heck, come to think of it, this is probably a requirement to not break static-analysis builds. :-) Some naming changes where I'm not completely sure of the right name, a few places with some unanswered questions, etc. seem to add up to wanting another swing at review here -- but broadly looks good, next review should be much faster.

(In reply to comment #15) Thanks for the review, lots of helpful suggestions. Before full reply with new patch, I'd like to discussion this: > "Variable object" (note lack of "s", which together with the ES3 spec's PDF > suckage means it's even harder to look up) is actually an ES3ism. ES5 now uses > "environment record" as the term here (distinguished as "declarative" for > variable storage in functions/catch blocks and most lexical variable names, or > "object" for |with| and global variables where the record wraps an object and > stores values in it). I think this would correspond to the "global environment > record", right? Seems worth pointing that out here if so, and I think the name > should be something similar to that. Maybe globalEnvRecord? Suggestions > welcome, but I'd like to avoid referring to obsolescent terms here (also point > to an ES5 chapter/verse for the reader's reference). My ES5 suggested a slightly different. Assuming our JSStackFrame is roughly associated with an Execution Context, then I think fp->varobj corresponds to the VariableEnvironment component introduced in 10.3 Table 19. In particular, the rules 10.4 describes match what happens to varobj. The "global environments"'s Environment Record is just the initial VariableEnvironment when executing a global code. So, shortening "Execution Context's VariableEnvironment" to something manageable, I'd like to switch varobj to envRecord.

Sorry, not paying attention while typing that last sentence. I'd like to switch varobj to varEnv.

The ES5 spec uses longSmallTalkInspiredNames, which we eschew in member names. Indeed envRecord abbreviates "environment" -- but not "record". What's more, so long as the type is JSObject* or equivalent (jsval), this is not an environment record in the ES5 sense (an abstract type with concrete "object" and "declarative" implementations). So "record" seems wrong. To balance yin and yang, how about either keeping varobj, or else using envobj. /be

I didn't read comment 17 (responding to bugmail, ugh). But "varEnv" is a bit off too if the type is JSObject*. This is truly a bikeshed color debate but we have surrounding code that does not camelCaps such short names. Also, do we really win by changing the name here? Consider JSOPTION_VAROBJFIX in the public API. /be

(In reply to comment #18) > To balance yin and yang, how about either keeping varobj, or else using envobj. Awesome

envobj works for me.

(In reply to comment #15) > Looking more at this, should you even be conditioning the gc-reachable-frames > bit at all? GC is rare. Extra work performed always, just to avoid tracing an > extra frame every so often (moreover -- only when the GC happens to be nested > beneath this-computation or exotic debugging code), doesn't seem to pass > cost-benefit analysis. Not quite. The condition already exists, so the branch is already being paid. Putting the pushGCReachableFrame in the branch means avoiding computation when the branch isn't taken. It's also (1) temporary (2) local, so I don't think its worth the fuss. > Is that correct? It makes my head spin to think through this. Yes. I realized that, for a later bug, I actually need suspendedFrame and saved for non-DEBUG purposes, so I made them non-DEBUG here. The invariants are thus: - cx->currentCallStack->suspendedFrame == NULL iff cx->fp != NULL - if cx->fp != NULL, cx->currentCallStack's newest frame is cx->fp, otherwise its cx->currentCallStack->suspendedFrame - if a callstack cs (in cx) != cx->currentCallStack, cs->suspendedFrame != NULL Also, agreed with all the public JSCallStack state; better now :) > > { > > JSObject *obj; > > uintN argc, nargs, nslots; > > JSGenerator *gen; > > jsval *slots; > > > >+ JSStackFrame *fp = cx->fp; > > This should be preceded by JS_ASSERT_NOT_ON_TRACE(cx). Good catch, although I think, instead, the function should be JS_REQUIRES_STACK.

This patch includes a fix for corner case I realized yesterday. The comment above SlowVarObj in jsinterp.cpp explains it and fixes it.

Comment on attachment 423855 [details] [diff] [review] better >diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h >+class JSCallStack >+{ >+ /* The varobj on entry to initialFrame (ECMA 252v5 10.3). */ >+ JSObject *initialVarObj; Citations to ES5 should be "ES5 10.3" or somesuch like that -- not to mention, 252? An even better reason to just say "ES5". I though we were switching to varEnv for these and accessors, and VariableEnvironment in comments? >+ void suspend(JSStackFrame *fp) { >+ JS_ASSERT(fp && !isSuspended()); >+ suspendedFrame = fp; >+ } This could use an assertion that |fp| has |initialFrame| in its down-chain. >+ void setInitialVarObj(JSObject *o) { initialVarObj = o; } >+ JSObject *getInitialVarObj() const { return initialVarObj; } More naming. >+ /* Mark the top callstack as suspended, without pushing a new one. */ >+ void saveTopCallStack() { I hate to say it, but we now have an amalgam of "top", "current", and "active" names for the most relevant call stack at any point. That's just confusing. How about: JSContext::currentCallStack for the field, JSContext::activeCallStack for the accessor, JSContext::{saveActive,restore}CallStack (note restore doesn't have an adjective in it, because the call stack being restored isn't -- at the point of the call -- the active one)? >+JS_ALWAYS_INLINE JSObject * >+JSStackFrame::varobj(JSCallStack *cs) >+{ >+#ifdef DEBUG >+ JSStackFrame *start; >+ JSStackFrame *stop; >+ if (cs->isSuspended()) { >+ start = cs->getSuspendedFrame(); >+ stop = cs->getInitialFrame()->down; >+ } else { >+ start = cs->context()->fp; >+ stop = cs->context()->activeCallStack()->getInitialFrame()->down; >+ } >+ JSStackFrame *f; >+ for (f = start; f != stop && f != this; f = f->down) {} >+ JS_ASSERT(f == this); >+#endif >+ return fun ? callobj : cs->getInitialVarObj(); >+} >+ >+JS_ALWAYS_INLINE JSObject * >+JSStackFrame::varobj(JSContext *cx) >+{ >+#ifdef DEBUG >+ JSStackFrame *start = cx->fp; >+ JSStackFrame *stop = cx->activeCallStack()->getInitialFrame()->down; >+ JSStackFrame *f; >+ for (f = start; f != stop && f != this; f = f->down) {} >+ JS_ASSERT(f == this); >+#endif >+ return fun ? callobj : cx->activeCallStack()->getInitialVarObj(); >+} JSStackFrame::varenvHelper(JSCallStack *cs, JSStackFrame *start, JSStackFrame *stop) may be in order here, since these methods overlap somewhat. >diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h >@@ -150,16 +149,26 @@ struct JSStackFrame { > JSObject *calleeObject() { > JS_ASSERT(argv); > return JSVAL_TO_OBJECT(argv[-2]); > } > > JSObject *callee() { > return argv ? JSVAL_TO_OBJECT(argv[-2]) : NULL; > } >+ >+ /* >+ * Get the object associated with the Execution Context's >+ * VariableEnvironment (ECMA 262v5 10.3). The given JSCallStack must >+ * contain this stack frame. >+ */ >+ JSObject *varobj(JSCallStack *cs); ES5 10.3 >diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp >+JS_REQUIRES_STACK JSObject * >+js_NewGenerator(JSContext *cx) > { > JSObject *obj; > uintN argc, nargs, nslots; > JSGenerator *gen; > jsval *slots; > >+ JSStackFrame *fp = cx->fp; >+ > obj = js_NewObject(cx, &js_GeneratorClass, NULL, NULL); > if (!obj) > return NULL; > > /* Load and compute stack slot counts. */ > argc = fp->argc; > nargs = JS_MAX(argc, fp->fun->nargs); > nslots = 2 + nargs + fp->script->nslots; Move the fp definition/assignment down to just before its first use. js_NewObject shouldn't change |cx->fp| (and I don't think even modifies it). >diff --git a/js/src/jsscript.cpp b/js/src/jsscript.cpp >--- a/js/src/jsscript.cpp >+++ b/js/src/jsscript.cpp >@@ -325,36 +325,36 @@ script_exec_sub(JSContext *cx, JSObject > /* > * Emulate eval() by using caller's this, var object, sharp array, etc., > * all propagated by js_Execute via a non-null fourth (down) argument to > * js_Execute. If there is no scripted caller, js_Execute uses its second > * (chain) argument to set the exec frame's varobj, thisv, and scopeChain. > * > * Unlike eval, which the compiler detects, Script.prototype.exec may be > * called from a lightweight function, or even from native code (in which >- * case fp->varobj and fp->scopeChain are null). If exec is called from >- * a lightweight function, we will need to get a Call object representing >- * its frame, to act as the var object and scope chain head. >+ * fp->scopeChain are null). If exec is called from a lightweight >+ * function, we will need to get a Call object representing its frame, to >+ * act as the var object and scope chain head. (in which case fp->varobj is null). Wrapping's already messed up, might as well keep it grammatically sensible while doing so.

(In reply to comment #24) > I though we were switching to varEnv for these and accessors, and > VariableEnvironment in comments? comment 18 and 19 argue against VariableEnvironment and for keeping varobj. I agree, and the name, ES5 or not, makes sense: its the object where you stick vars.

This patch, in addition to addressing review points, - puts JSCallStack into namespace js and drops JS. This meshes well with additional new types in bug 540706. - factors js_ContainingCallStack out of the varobj() debug assertions and the new assertion suggested by Waldo. In doing this, I ended up making a slightly more precise assertion that caught a [debug-only] bug in the cx->varobj use in js_GetArgsObject. (Need to do a slow search since we don't know fp is in the active callstack.)

Since this patch went on I get a segmentation fault when starting SeaMonkey with multiple profiles on trying to start the second or subsequent times after building. See bug 545044 for details. The workaround is to select the profile from the command line using the -P commandline switch. Backing out the patch on this bug fixes the issue.

Thanks, I'll build and try to reproduce.

For the record, it was bug 541456 causing the regression, not this one.