540706 - use contiguous buffer for stack frames and values

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

There are several advantages to keeping jsvals and JSStackFrames in a single contiguous segment of memory in the same manner as JSC.  E.g., fp->slot and fp->argv (with a little work (bug 539144)) can instead be replaced by ((char*)fp + a constant offset).  This means a smaller JSStackFrame, and either one less load per slot access or less register pressure (if slot/argv are are cached).  For JaegerMonkey on x86, this is good because we'd like to keep 'fp' in a fixed register and use that value for all slot access.

There are a few cases where stack frames technically form a tree and share argvs and other things that break a simple LIFO model, but I believe these can be addressed by copying.

Comment 1

[Luke Wagner [:luke]]

Attachment: WIP

Though still very much a WIP, I thought I'd post progress.

js::StackSpace (jscntxt.h) holds the big buffer.  The layout can be described by the following regular expression, where CS = js::CallStack, JSF = JSStackFrame, and V = jsval:

 (CS V* (JSF V*)*)*

With a static assert that sizeof(CS) and sizeof(JSF) are multiples of sizeof(V), there is no space between a CS/JSF and neighboring jsvals which allows for fp->slots and ifp->mark to be removed (in this patch), argv to be removed (with some work), and regs/sp/callerRegs to be removed (next bug).  To have a constant-size frame, JSInlineStackFrame will be merged with JSStackFrame.  This wastes memory for non-inline calls, but these shouldn't have too many activations on the stack at one time anyway.

Comment 2

[Luke Wagner [:luke]]

Attachment: WIP 2

Its shaping up.  I realized that for this to work, JSStackFrame::regs needs to always be non-null so that fp->regs->sp can always be used to guard args to the right of a stack frame.  Fortunately, I already had to move JSInlineStackFrame members into JSStackFrame (need constant size for fp->slots == (jsval *)&fp[1]), so regs can be initialized to &fp->callerRegs and callerRegs.sp initialized to &fp[1].

Comment 4

[Luke Wagner [:luke]]

Attachment: WIP 3

Done with interpreter work.  Still need to run the full battery of tests and then fixup jstracer.

Comment 5

[Luke Wagner [:luke]]

Attachment: just need tracing

Only missing changes to the tracer, but I've been using --disable-jit to debug/benchmark.  Ref tests demonstrated to me that I cannot (1) borrow fp->sp (messes up exception handling/decompiler) or (2) set fp to NULL (messes up "is this context running" questions), so this patch uses a different strategy to handle rooting js_Invoke args.  Also, generators required more care than given.  Passes trace, ref, xpcshell, and mochi tests.  Benchmarking interpreter-only, the patch is a 3.5% speedup on SS on Linux:

  1.035x as fast    2048.4ms +/- 0.4%   1979.6ms +/- 0.2%     significant

Comment 6

[Luke Wagner [:luke]]

(In reply to comment #5)
> Also, generators required more care than given.

"... in the previous patch", I meant to say.

Comment 7

[Luke Wagner [:luke]]

Attachment: almost there...

So with tracing support, I was able to run SS and V8 (tested on OS X).  SS was unaffected (which makes sense; we trace that pretty well and tracing has little to gain from this patch) but V8 got 3.3% faster:

                   FROM                 TO
  1.033x as fast   12710.8ms +/- 0.7%   12306.1ms +/- 0.3%   significant

Two issues remain: (1) JSC does some more trickery with VirtualAlloc on Windows, need to see if I need to do that; (2) the fixed buffer size means we can't abort on OOM in LeaveTree, need to do more eager checking, perhaps reusing existing tree-call checks.  (On the bright side, this will mean less ways to abort().)

Comment 8

[Luke Wagner [:luke]]

Attachment: right patch this time

oops, last one was the wrong patch

Comment 9

[Luke Wagner [:luke]]

Attachment: for review

Passes tests, does incremental VirtualAlloc(MEM_COMMIT) on windows, has explanatory comments in jscntxt.h.

The patch is ready for review, but cannot land until blazinglyfastthis removes the security checks in JS_GetFrameThis and js_ComputeGlobalThis (see two TODOs). This should happen soonish.

Waldo, if you need me to split off generator, tracer, or xpconnect changes into separate patches for other reviewers, let me know.

Comment 10

[Luke Wagner [:luke]]

Attachment: tweaks, simplifications, and future-friendly

In addition to tweaks and simplifications, this patch changes the way generators work to not store a JSGenerator* in the JSStackFrame (unioned with callerRegs).  That way, when callerRegs is removed (bug 547851), JSStackFrame can shrink.

Comment 11

[Luke Wagner [:luke]]

Comment on attachment 428495 [details] [diff] [review]
tweaks, simplifications, and future-friendly

Making a few things more regular as irregularities come to light as I work on bug 547851.  Mostly done, new patch when things settle.

Comment 12

[Luke Wagner [:luke]]

Attachment: regular er

This patch ensures that a new callstack is pushed whenever the interpreter reenters, which gives a 1-to-0-or-1 relationship between js_Interpret activations and CallStacks.  This was almost true in the previous patches, so this one just fills in the corner cases.  I'll r? again when blazinglyfastthis gets close to landing.

Comment 13

[Luke Wagner [:luke]]

Attachment: simplify record_EnterFrame

Tiny independent patch

Comment 14

[Luke Wagner [:luke]]

Attachment: more regularer

minor tweaks while working on bug 547851. also, the 1-to-0-or-1 property I was aiming for in the last patch missed forgot about slow native frames pushed on deep bail.

Comment 15

[Luke Wagner [:luke]]

Attachment: rebased, prettied

Comment 16

[David Anderson [:dvander] - inactive, e-mail if emergency]

http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/rev/bf3a610c3ed5

Comment 17

[Luke Wagner [:luke]]

Attachment: bug fixes, tweaks, rebased

With two bug fixes found by Gary fuzzing the JM repo.

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 432262 [details] [diff] [review]
bug fixes, tweaks, rebased

>     JSThread *thread = (JSThread *) js_calloc(sizeof(JSThread));
>     if (!thread)
>         return NULL;
>     JS_INIT_CLIST(&thread->contextList);
>     thread->id = id;
>-    thread->data.init();
>+    if (!thread->data.init())
>+        return NULL;
>     return thread;

Not sure if we care, but this leaks if JSThreadData::init() fails.

Comment 19

[Luke Wagner [:luke]]

Good catch, will fix!

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c64a8e39c2c6

Comment 21

[Luke Wagner [:luke]]

Reopened; the committed patch was just a prelude.

Comment 22

[Luke Wagner [:luke]]

Attachment: rebased

The necessary parts of blazinglyfastthis have landed so this is really ready for review.

Comment 23

[Luke Wagner [:luke]]

Attachment: tweaked

Moved a few bits from the 'remove regs' patch into this one that belong in this one.

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

tiny nit for opt builds:

../jscntxt.cpp: In member function ‘void js::StackSpace::getSynthesizedSlowNativeFrame(JSContext*, js::CallStack*&, JSStackFrame*&)’:
../jscntxt.cpp:381: warning: unused variable ‘nvals’

Comment 25

[Luke Wagner [:luke]]

Thanks, got it.

Comment 26

[Jeff Walden [:Waldo]]

Comment on attachment 436816 [details] [diff] [review]
tweaked

It occurs to me that JSStackFrame::slots() exposes a raw pointer, and therefore it's impossible to do any assertions on the validity of indexes into it.  Is there a reason not to add JSStackFrame::getSlot(), JSStackFrame::setSlot(), and JSStackFrame::getSlot{Ref,Ptr} that would make an effort to assert in-rangeness, to catch what slop we can?  Followup perhaps.


>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp

>-            /* The filter passed *elemroot, so push it onto our result. */
>-            ok = SetArrayElement(cx, newarr, newlen++, *elemroot);
>+            /* The filter passed *args.getvp(), so push it onto our result. */
>+            ok = SetArrayElement(cx, newarr, newlen++, tvr.value());

"it" binds very poorly in this comment, in my opinion: let's try this comment instead.

  /* The filter accepted the element, so push it onto our result. */


>diff --git a/js/src/jscntxt.cpp b/js/src/jscntxt.cpp

> #include "jsstr.h"
> #include "jstracer.h"
>+#include "jsiter.h"

Should be alphabetized with all the others.  Or is this not possible for some reason?  (Caveat applies to other places where I note this.)


>+void
>+StackSpace::finish()
>+{
>+#ifdef XP_WIN
>+    VirtualFree(base, (commitEnd - base) * sizeof(jsval), MEM_DECOMMIT);
>+    VirtualFree(base, 0, MEM_RELEASE);

As far as I can tell from reading docs, the latter call's effect includes the effect of the former.

http://msdn.microsoft.com/en-us/library/aa366892(v=VS.85).aspx


>+    } while(newCommit < request);

Space after while.


>+
>+    /* Cast safe because sCapacityBytes is small. */
>+    int32 size = (int32)(newCommit - commitEnd) * sizeof(jsval);

C++-style cast, and "Cast-safe" or "Cast is safe" (not quite sure which meaning you intended).


>+        if (!cs->inContext()) {

It seems like this case is less likely than the other, so the two blocks should be reversed.


>+            /* Mark slots/args trailing off of the last stack frame. */
>+            JSStackFrame *fp = cs->getCurrentFrame();
>+            js_TraceJSValsBetween(trc, fp + 1, end);

This belongs here.

>+            /* Mark stack frames and slots/args between stack frames. */
>+            JSStackFrame *initialFrame = cs->getInitialFrame();
>+            for (JSStackFrame *f = fp; f != initialFrame; f = f->down) {
>+                js_TraceStackFrame(trc, f);
>+                js_TraceJSValsBetween(trc, f->down + 1, f);
>+            }
>+
>+            /* Mark initialFrame stack frame and leading args. */
>+            js_TraceStackFrame(trc, initialFrame);
>+            js_TraceJSValsBetween(trc, cs + 1, initialFrame);

...but this is kind of abstraction-violating, sort of.  Can the stack-frame trace and frame-values trace be unified into one JSStackFrame::trace method?  The only difference is the start of the range of jsvals.  Maybe something like:

> JSStackFrame::trace(JSTracer* trc, jsval* start)
> {
>   JS_ASSERT_IF(down, reinterpret_cast<jsval*>(down) == start);
>   ...trace the stack frame...
>   TraceValues(trc, start, reinterpret_cast<jsval*>(this), "stack frame values");
> }

Speaking of which, doesn't js::TraceValues do what the js_TraceJSValsBetween method does?  Faux type-safety through templatization that's immediately casted away doesn't seem to win much for me.  We could add the modulus-checking assertions to that method easily enough (and should probably add one to ensure the start and end pointers are aligned, too).


>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h

>+    /* The three mutually exclusive states of a callstack */
>+
>+    bool inContext() const {
>+        JS_ASSERT(!!cx == !!initialFrame);
>+        JS_ASSERT_IF(!initialFrame, !suspendedFrame && !saved);
>+        return cx;
>+    }
>+
>+    bool isActive() const {
>+        JS_ASSERT_IF(suspendedFrame, inContext());
>+        return initialFrame && !suspendedFrame;
>+    }
>+
>+    bool isSuspended() const {
>+        JS_ASSERT_IF(!suspendedFrame, !saved);
>+        JS_ASSERT_IF(suspendedFrame, inContext());
>+        return suspendedFrame;
>+    }

I've been staring at these states both in past patches introducing call stacks and in this patch, and I still have difficulty understanding the difference, and distinguishing between, call stacks in different states.  It would be helpful if there were a comment explaining the practical effect of each state, also describing when and why a user might choose to enter each state.

For these three methods, I am particularly not helped by the comment claiming they are "exclusive" but then seeing in an assertion that |isSuspended()| implies |inContext()|.


>+    /* Substate of suspended, queryable in any state. */
>+
>+    bool isSaved() const {

There should be no blank line here.


>+    /* Transitioning between inContext <--> isActive */
>+
>+    void joinContext(JSContext *cx, JSStackFrame *f) {
>+        JS_ASSERT(!inContext());
>+        this->cx = cx;
>+        initialFrame = f;
>+    }
>+
>+    void leaveContext() {
>+        JS_ASSERT(inContext());
>+        this->cx = NULL;
>+        initialFrame = NULL;
>+    }

If this transitioning is taking place here, each method should begin and end with assertions of the appropriate states of |inContext()|-ness and begin and end with assertions of the appropriate states of |isActive()|-ness, to verify that those queries conform with their latent definitions in these methods.  But, if I'm reading this correctly, the latter method doesn't actually transition to |isActive()| -- |isSuspended()| at best, and even that I'm not sure about.


>+    /* Transitioning between isActive <--> isSuspended */
> 
>     void suspend(JSStackFrame *fp) {
>-        JS_ASSERT(fp && !isSuspended() && contains(fp));
>+        JS_ASSERT(fp && isActive() && contains(fp));
>         suspendedFrame = fp;
>     }
> 
>     void resume() {
>         JS_ASSERT(suspendedFrame);
>         suspendedFrame = NULL;
>     }

This should have the requisite pre- and post-assertions.


>+    void setInitialVarObj(JSObject *o) {
>+        JS_ASSERT(inContext());
>+        initialVarObj = o;
>+    }

Canonical name is obj.


>+JS_STATIC_ASSERT(sizeof(CallStack) % sizeof(jsval) == 0);
>+static const size_t JSValsPerCallStack = sizeof(CallStack) / sizeof(jsval);

I think I prefer these the other way around logically:

>+static const size_t JSValsPerCallStack = sizeof(CallStack) / sizeof(jsval);
>+JS_STATIC_ASSERT(JSValsPerCallStack * sizeof(jsval) == sizeof(CallStack));

Excepting one or two pieces of SpiderMonkey you've written, we've always used C-style constant names -- all caps, words separated by underscores.  CALLSTACK_SIZE_IN_JSVALS is the more idiomatic name for SpiderMonkey.  Even better yet would be js::CallStack::SIZE_IN_JSVALS.


>+    /*
>+     * Return whether nvals can be allocated from the top of the stack.
>+     * N.B. the caller must ensure |from == firstUnused()|.
>+     */
>+    inline bool ensureSpace(JSContext *maybecx, jsval *from, ptrdiff_t nvals) const;

Please clarify that failure is reported (when possible).  The method feels low-level enough that I want the extra reminder, convention implications aside.


>+  public:
>+    static const size_t sCapacityVals   = 512 * 1024;
>+    static const size_t sCapacityBytes  = sCapacityVals * sizeof(jsval);
>+    static const size_t sCommitVals     = 16 * 1024;
>+    static const size_t sCommitBytes    = sCommitVals * sizeof(jsval);
>+
>+    JS_STATIC_ASSERT(sCapacityVals % sCommitVals == 0);

CAPACITY_JSVALS
CAPACITY_BYTES
COMMIT_INCREMENT_JSVALS
COMMIT_INCREMENT_BYTES


>+    /* Kept as a member of JSThreadData; cannot use constructor/destructor. */

Just for sanity, add an unimplemented private operator new.


>+#ifdef DEBUG
>+    template <class T>
>+    bool contains(T *t) const {
>+        char *v = (char *)t;
>+        return v >= (char *)base && v + sizeof(T) <= (char *)end;
>+    }
>+#endif

Technically this is wrong if the last addition overflows the memory space.  I tend to think we should add another check for this -- it's debug-only code anyway, no harm doing so.


>+    /* +1 for slow native's stack frame. */
>+    static const ptrdiff_t sMaxJSValsNeededForTrace =
>+      MAX_NATIVE_STACK_SLOTS + MAX_CALL_STACK_ENTRIES * JSValsPerStackFrame +
>+      (JSValsPerCallStack + JSValsPerStackFrame /* synthesized slow native */);

MAX_TRACE_SPACE_JSVALS?


>+    /* Mark all callstacks, frames, and slots on the stack. */
>+    JS_REQUIRES_STACK
>+    void mark(JSTracer *trc);

One-liner.


>+    /*
>+     * pushInvokeArgs allocates |argc+2| rooted values that will be passed as

Spaces around +.


>+  private:
>+    friend class js::StackSpace;
>+
>+    /* 'fp' must only be changed by calling this function. */
>+    void setCurrentFrame(JSStackFrame *fp) {
>+        this->fp = fp;
>+    }
>+  public:
>+
>     /* Temporary arena pool used while compiling and decompiling. */
>     JSArenaPool         tempPool;

Blank line above public, none below it.


>+#ifdef DEBUG
>+    bool callStackInSync() const {
>+        if (fp) {
>+            JS_ASSERT(currentCallStack->isActive());
>+            if (js::CallStack *prev = currentCallStack->getPreviousInContext())
>+                JS_ASSERT(!prev->isActive());
>+        } else {
>+            JS_ASSERT_IF(currentCallStack, !currentCallStack->isActive());
>+        }
>+        return true;
>+    }
>+#endif

This seems much better to me as |assertCallStackInSync()|, empty #ifndef DEBUG.


>+  private:
>+    /*
>+     * To go from a live generator frame (on the stack) to its generator object
>+     * (see comment js_FLoatingFrameIfGenerator), we maintain a stack of active

typo

>+     * generators, pushing and popping when entering and leaving generator
>+     * frames, respectively.
>+     */
>+    js::Vector<JSGenerator *, 0, js::SystemAllocPolicy> genStack;

0 seems like the wrong default here, at least 1, maybe 2 to avoid allocations.


>diff --git a/js/src/jscntxtinlines.h b/js/src/jscntxtinlines.h

>+#ifdef DEBUG
>+/* Inline so we don't need the friend API. */
>+JS_ALWAYS_INLINE bool
>+StackSpace::isCurrent(JSContext *cx) const
>+{
>+    JS_ASSERT(cx == currentCallStack->maybeContext());
>+    JS_ASSERT(cx->getCurrentCallStack() == currentCallStack);
>+    JS_ASSERT(cx->callStackInSync());
>+    return true;
>+}
>+#endif

assertCurrent()


>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp

>             /* NB: wp is held, so we can safely dereference it still. */
>-            ok = wp->handler(cx, obj, propid,
>+            if (!wp->handler(cx, obj, propid,
>                              SPROP_HAS_VALID_SLOT(sprop, scope)
>-                             ? obj->getSlotMT(cx, sprop->slot)
>-                             : JSVAL_VOID,
>-                             vp, wp->closure);
>-            if (ok) {
>+                                ? obj->getSlotMT(cx, sprop->slot)
>+                                : JSVAL_VOID,
>+                             vp, wp->closure)) {

Misaligned ternary.


>+            /*
>+             * Create a pseudo-frame for the setter invocation so that any
>+             * stack-walking security code under the setter will correctly
>+             * identify the guilty party.  So that the watcher appears to
>+             * be active to obj_eval and other such code, point frame.pc
>+             * at the JSOP_STOP at the end of the script.
>+             *
>+             * The pseudo-frame is not created for fast natives as they
>+             * are treated as interpreter frame extensions and always
>+             * trusted.
>+             */
>+            JSObject *closure = wp->closure;
>+            JSClass *clasp = closure->getClass();
>+            JSFunction *fun;
>+            JSScript *script;
>+            if (clasp == &js_FunctionClass) {
>+                fun = GET_FUNCTION_PRIVATE(cx, closure);
>+                script = FUN_SCRIPT(fun);
>+            } else if (clasp == &js_ScriptClass) {
>+                fun = NULL;
>+                script = (JSScript *) closure->getPrivate();
>+            } else {
>+                fun = NULL;
>+                script = NULL;
>+            }
>+
>+            uintN vplen = 2 + (fun ? (fun->minArgs() +
>+                                      (fun->isInterpreted() ? 0
>+                                                            : fun->u.n.extra))

This can't be going past 99 columns yet, can it?  

>+                                   : 0);

Also not a fan of this massively underhanging 0 -- add a separate |if (fun) / vplen += ...| please.


>+            uintN nslots = script ? script->nslots : 0;

This is wrong, if I'm reading the old code correctly: nslots should be 2 + (...).  I think this is only testable with a compiled-code test.


>+                /* Initialize slots/frame. */
>+                jsval *vp = frame.getvp();
>+                memset(vp, 0, vplen * sizeof(jsval));

Use the new helper that's shown up since this was written.


>+                vp[0] = OBJECT_TO_JSVAL(closure);
>+                JSStackFrame *fp = frame.getFrame();
>+                memset(fp->slots(), 0, nslots * sizeof(jsval));
>+                memset(fp, 0, sizeof(JSStackFrame));

Again, and again.


>+                JS_ASSERT_IF(!fun, !script);

Isn't this dead?  You're in a block guarded by |if (fun)| and never assign to |fun|.


>+            JSBool ok = !wp->setter ||
>+                         (sprop->hasSetterValue()

Reduce indent by one.


>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp

> #include "jsatominlines.h"
> #include "jsobjinlines.h"
>+#include "jscntxtinlines.h"

Alphabetize.


> static JS_REQUIRES_STACK JSBool
> fun_applyConstructor(JSContext *cx, uintN argc, jsval *vp)
> {
>     JSObject *aobj;
>     uintN length, i;
>-    void *mark;
>-    jsval *invokevp, *sp;
>     JSBool ok;
>+    jsval *sp;

Move this to first assignment.


>     if (length > JS_ARGS_LENGTH_MAX)
>         length = JS_ARGS_LENGTH_MAX;
>-    invokevp = js_AllocStack(cx, 2 + length, &mark);
>-    if (!invokevp)
>         return JS_FALSE;
> 
>-    sp = invokevp;
>+    InvokeArgsGuard args;
>+    if (!cx->stack().pushInvokeArgs(cx, length, args))
>+
>+    sp = args.getvp();

This looks...wrong. :-D


>diff --git a/js/src/jsfun.h b/js/src/jsfun.h

>+    bool minArgs()          const { return FUN_MINARGS(this); }

Should return uint16_t, not bool!


>@@ -438,18 +442,22 @@ JS_STATIC_ASSERT(JSSLOT_ARGS_START == JS
> /* Number of extra fixed slots besides JSSLOT_PRIVATE. */
> const uint32 ARGS_FIXED_RESERVED_SLOTS = JSSLOT_ARGS_START - JSSLOT_ARGS_LENGTH;
> 
> /*
>  * Maximum supported value of arguments.length. It bounds the maximum number of
>  * arguments that can be supplied via the second (so-called |argArray|) param
>  * to Function.prototype.apply. This value also bounds the number of elements
>  * parsed in an array initialiser.
>+ *
>+ * The thread's stack is the limiting factor for this number. It is currently
>+ * 2MB, which fits a little less than 2^19 arguments (once the stack frame,
>+ * callstack, etc are included). Pick a max args length that is a little less.
>  */

etc.


>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

> #include "jsatominlines.h"
> #include "jspropertycacheinlines.h"
> #include "jsobjinlines.h"
> #include "jsscopeinlines.h"
> #include "jsscriptinlines.h"
> #include "jsstrinlines.h"
>+#include "jscntxtinlines.h"

Alphabetize.  (Not sure why it isn't, at the moment.)


>+    JSBool ok = (flags & JSINVOKE_CONSTRUCT)
>+                 ? js_InvokeConstructor(cx, args, JS_TRUE)
>+                 : js_Invoke(cx, args, flags);

Mis-indented by one.


>+    vp[0] = invokevp[0];

|*vp = *invokevp| seems more canonical.


>+    /* Initialize fixed slots. */
>+    memset(fp->slots(), 0, script->nfixed * sizeof(jsval));

Newer zeroing thing.


>+        initialVarObj = (down == cx->fp)
>+                          ? down->varobj(cx)
>+                          : down->varobj(cx->containingCallStack(down));

Misaligned by 1.


>         JSObject *obj = chain;
>         if (cx->options & JSOPTION_VAROBJFIX) {
>             while (JSObject *tmp = obj->getParent())
>                 obj = tmp;
>         }

Make that |obj = obj->getGlobal();| while you're here.


>diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h

>+/* 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) */
>+#define JSFRAME_ASSIGNING           0x04 /* a complex (not simplex JOF_ASSIGNING) op
>+                                            is currently assigning to a property */
>+#define JSFRAME_DEBUGGER            0x08 /* frame for JS_EvaluateInStackFrame */
>+#define JSFRAME_EVAL                0x10 /* frame for obj_eval */
>+#define JSFRAME_FLOATING_GENERATOR  0x20 /* frame copy stored in a generator obj */
>+#define JSFRAME_YIELDING            0x40 /* js_Interpret dispatched JSOP_YIELD */
>+#define JSFRAME_ITERATOR            0x80 /* trying to get an iterator for for-in */
>+#define JSFRAME_GENERATOR          0x200 /* frame belongs to generator-iterator */
>+#define JSFRAME_OVERRIDE_ARGS      0x400 /* overridden arguments local variable */
>+
>+#define JSFRAME_SPECIAL       (JSFRAME_DEBUGGER | JSFRAME_EVAL)

Make these all an enum, for better debuggability, while you're here.


>+    bool isGenerator() const { return flags & JSFRAME_GENERATOR; }
>+    bool isFloatingGenerator() const {
>+        JS_ASSERT_IF(flags & JSFRAME_FLOATING_GENERATOR, isGenerator());
>+        return flags & JSFRAME_FLOATING_GENERATOR;
>+    }

Prefer keeping only one |flags & ...|:

  if (flags & JSFRAME_FLOATING_GENERATOR) {
    JS_ASSERT(isGenerator());
    return true;
  }
  return false;


>+JS_STATIC_ASSERT(sizeof(JSStackFrame) % sizeof(jsval) == 0);
>+static const size_t JSValsPerStackFrame = sizeof(JSStackFrame) / sizeof(jsval);

JSStackFrame::SIZE_IN_JSVALS again, and reverse the ordering as for the call stack const noted previously.


> extern JS_REQUIRES_STACK JSBool
>-js_InvokeConstructor(JSContext *cx, uintN argc, JSBool clampReturn, jsval *vp);
>+js_InvokeConstructor(JSContext *cx, const js::InvokeArgsGuard &args,
>+                     JSBool clampReturn);

This can all be one line, doesn't overflow 99 characters.


>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp

> #include "jsvector.h"
> #include "jsversion.h"
>+
> #include "jsstrinlines.h"
>+#include "jscntxtinlines.h"

Alphabetize.


>+        if (!rdata.args.getvp() &&
>+            !cx->stack().pushInvokeArgs(cx, argc, rdata.args))
>+            return false;

A ready() method seems in order here, rather than this semi-oddity.  Also, doesn't the condition fit in 99 characters?  (And if it didn't, whither bracing?)


>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp

> #include "jsatominlines.h"
> #include "jspropertycacheinlines.h"
> #include "jsobjinlines.h"
> #include "jsscopeinlines.h"
> #include "jsscriptinlines.h"
>+#include "jscntxtinlines.h"

Alphabetize.


>diff --git a/js/src/trace-test/tests/basic/testBug550210.js b/js/src/trace-test/tests/basic/testBug550210.js

>+function g(e) {
>+    return ("" + e)
>+}
>+function blah() {
>+    do {
>+        yield
>+    } while ({}(p = arguments))
>+}
>+rv = blah();
>+try {
>+    for (a in rv) ;
>+} catch (e) {
>+    print("" + g(e))

Add semicolons in the relevant places where implicit semicolon insertion occurs.


>+}
>+gc()
>+

Extra trailing newline?


>diff --git a/js/src/xpconnect/src/XPCDispTearOff.cpp b/js/src/xpconnect/src/XPCDispTearOff.cpp

>+        // We use js_Invoke so that the gcthings we use as args will be rooted
>+        // by the engine as we do conversions and prepare to do the function
>+        // call. This adds a fair amount of complexity, but is a good
>+        // optimization compared to calling JS_AddRoot for each item.

"but it's a good"


>diff --git a/js/src/xpconnect/src/xpcwrappedjsclass.cpp b/js/src/xpconnect/src/xpcwrappedjsclass.cpp

>+    // We use js_Invoke so that the gcthings we use as args will be rooted by
>+    // the engine as we do conversions and prepare to do the function call.
>+    // This adds a fair amount of complexity, but is a good optimization
>+    // compared to calling JS_AddRoot for each item.

Again.


Much as I would like to say "good enough", I probably should look this over again after fixes.

Comment 27

[Luke Wagner [:luke]]

(In reply to comment #26)
> It occurs to me that JSStackFrame::slots() exposes a raw pointer, and therefore
> it's impossible to do any assertions on the validity of indexes into it.  Is
> there a reason not to add JSStackFrame::getSlot(), JSStackFrame::setSlot(), and
> JSStackFrame::getSlot{Ref,Ptr} that would make an effort to assert
> in-rangeness, to catch what slop we can?  Followup perhaps.

That sounds like a great followup bug.

> >+void
> >+StackSpace::finish()
> >+{
> >+#ifdef XP_WIN
> >+    VirtualFree(base, (commitEnd - base) * sizeof(jsval), MEM_DECOMMIT);
> >+    VirtualFree(base, 0, MEM_RELEASE);
> 
> As far as I can tell from reading docs, the latter call's effect includes the
> effect of the former.

Initially, I saw http://trac.webkit.org/browser/trunk/JavaScriptCore/interpreter/RegisterFile.cpp#L40 and confirmed on MSDN.  Looking again, it seems like I was reading the MSDN page for WinCE and not the Win2K one you found.  Do we support WinCE?  If so, what is the right symbol for the #ifdef?

> >+        if (!cs->inContext()) {
> 
> It seems like this case is less likely than the other, so the two blocks should
> be reversed.

I thought PGO meant that we could order our branches based on what was clearest, not what will be statically predicted best.  That said, it seems logical to address the 'if (cx->inContext())' case first.

> ...but this is kind of abstraction-violating, sort of.  Can the stack-frame
> trace and frame-values trace be unified into one JSStackFrame::trace method? 
> ...
> Speaking of which, doesn't js::TraceValues do what the js_TraceJSValsBetween
> method does?  Faux type-safety through templatization that's immediately casted
> away doesn't seem to win much for me.

Incidentally, working on the ill-fated dual-stack patch, I changed StackSpace::mark in a way that addresses some of your concerns.  So, give it a second look in the to-be-posted patch.  However, I disagree with trying to pull stack-marking out of StackSpace::mark: stack frames are not independent or encapsulated; rather they are constituent pieces of a single data structure which is managed and marked by StackSpace.  Pulling out that code breaks the textual coherence of the algorithm.

> I've been staring at these states both in past patches introducing call stacks
> and in this patch, and I still have difficulty understanding the difference,
> and distinguishing between, call stacks in different states.
> ...
> For these three methods, I am particularly not helped by the comment claiming
> they are "exclusive" but then seeing in an assertion that |isSuspended()|
> implies |inContext()|.

The CallStack comment does describes all this and more, but you are right that a more specific comment targeted at these three functions would help.  The source of your confusion, which was unhelpfully ignored by the comment in question, is that the three (mutually exclusive) states are !inContext, isActive, and isSuspended.  Better comment added.
 
> Excepting one or two pieces of SpiderMonkey you've written, we've always used
> C-style constant names -- all caps, words separated by underscores. 
> CALLSTACK_SIZE_IN_JSVALS is the more idiomatic name for SpiderMonkey.  Even
> better yet would be js::CallStack::SIZE_IN_JSVALS.

The member definition isn't possible because a class is incomplete inside its definition (already tried :( ).

> >+    /* Kept as a member of JSThreadData; cannot use constructor/destructor. */
> 
> Just for sanity, add an unimplemented private operator new.

The ability to dynamically allocate a StackSpace is orthogonal to whether it may or may not depend on its destructor being called.  It would be perfectly fine to do 's = new StackSpace; s->init(); .... s->finish(); delete s;"

> >+            uintN nslots = script ? script->nslots : 0;
> 
> This is wrong, if I'm reading the old code correctly: nslots should be 2 +
> (...).  I think this is only testable with a compiled-code test.

I think it's right as is.  It's tricky because 'nslots' changes meaning with the patch: before, nslots was the total number of slots to allocate and now it is the number of fixed slots.  I did not do this to torment you; vplen and nslots are used in this manner throughout the patch.  (See StackSpace memfun param names.)  However, 'nfixed' seems like a better name than 'nslots'; I'll use that throughout the patch instead.

> >+    InvokeArgsGuard args;
> >+    if (!cx->stack().pushInvokeArgs(cx, length, args))
> >+
> >+    sp = args.getvp();
> 
> This looks...wrong. :-D

Hah.  Yeah, I guess I should, like, compile with --enable-narcissus at least once, huh?

> >+    bool minArgs()          const { return FUN_MINARGS(this); }

Wow, luke--.  Good catch.  Mmm, implicit bool -> int conversion let a use of this actually compile (and apparently survive fuzzing).

Thanks for all the work reviewing this patch!

Comment 28

[Brendan Eich [:brendan]]

>   /* The filter accepted the element, so push it onto our result. */

"The element passed the filter, so push it onto our result."

You want "it" referring to element not filter.

It's not important that minArgs' return type be uint16 (no stdint _t noise!). An unsigned or uintN would be better.

/be

Comment 29

[Luke Wagner [:luke]]

Attachment: rebased, comments addressed

Comment 30

[Luke Wagner [:luke]]

(Brendan's comments not addressed, in above patch, but now they are)

Comment 31

[Luke Wagner [:luke]]

Attachment: without the fail

In that last rebase I missed an important change to XPCWrappedJSClass::CallMethod.

Comment 32

[Jeff Walden [:Waldo]]

Comment on attachment 443294 [details] [diff] [review]
without the fail

Regarding "It seems like this case is less likely than the other, so the two blocks should be reversed.", it's not about PGO, it's about the reader most likely thinking in terms of the common case: a call stack that's actively in use, with a non-zero number of frames, that's not suspended.  Suspended or non-contexted call stacks are the weirder case, and they can't be dealt with in brief early-exit one-liners as ignorably brief digressions, so it makes sense to push them as far down in control flow as possible to get them out of the way of what the reader will think about the most.

This is perhaps a subtle distinction between arrangement to hand-optimize and to improve readability, but I think it's important to make.


> >+  private:
> >+    friend class js::StackSpace;
> >+
> >+    /* 'fp' must only be changed by calling this function. */
> >+    void setCurrentFrame(JSStackFrame *fp) {
> >+        this->fp = fp;
> >+    }
> >+  public:
> >+
> >     /* Temporary arena pool used while compiling and decompiling. */
> >     JSArenaPool         tempPool;
> 
> Blank line above public, none below it.

This was missed.


>+            if (cs->getInitialVarObj())
>+                JS_CALL_OBJECT_TRACER(trc, cs->getInitialVarObj(), "varobj");

if (JSObject *varobj = ...) / JS_CALL_...(...);


>diff --git a/js/src/jsdbgapi.cpp b/js/src/jsdbgapi.cpp

>+            JSBool ok = !wp->setter ||
>+                        (sprop->hasSetterValue()
>+                        ? js_InternalCall(cx, obj,
>+                                          CastAsObjectJSVal(wp->setter),
>+                                          1, vp, vp)
>+                        : wp->setter(cx, obj, userid, vp));

Nitpick again: you've aligned ? and : with (, not with "s" of "sprop".


>diff --git a/js/src/jsgc.h b/js/src/jsgc.h

Is there a reason one TraceValues doesn't just call the other?


>diff --git a/js/src/jsinterp.cpp b/js/src/jsinterp.cpp

>+        initialVarObj = (down == cx->fp)
>+                         ? down->varobj(cx)
>+                         : down->varobj(cx->containingCallStack(down));

Oops, my mistake, I really meant misalignment by 2 last time -- ?: should line up with the first column of the condition, which is the ( here, not the "d".


>diff --git a/js/src/jsinterp.h b/js/src/jsinterp.h

>+/* JS stack frame flags. */
>+enum JSFrameFlags {

These days probably better as js::FrameFlags.

Comment 33

[Luke Wagner [:luke]]

(In reply to comment #32)

Thanks again.

> >+/* JS stack frame flags. */
> >+enum JSFrameFlags {
> 
> These days probably better as js::FrameFlags.

But then the flags should really be renamed to strip off the JS, and that means touching a lot of code.  Let's not make this fatty any fatter -- another bug perhaps?  Also, even without stripping JS, some uses would need explicit js:: qualification.

Comment 34

[Jeff Walden [:Waldo]]

I'm fine with momentary js::JSFRAME_* redundancy.  If you're not, feel free to leave as is and file a followup.

Comment 35

[Luke Wagner [:luke]]

Attachment: with 32.33 (repeating of course) percent less fail

Failed landing last night.  There were a couple of small problems (syntax error from nit-fix in opt build, overzealous assertion in pushSynthesizedSlowNativeFrame) and one big one: in simplifying js_Execute I moved the call to the thisObject hook before pushing the frame, which is bad.  This patch fixes those problems.  Try-servering, will try to reland.

Comment 36

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/a00078178698

Comment 37

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a00078178698