579183 - loosen-up StackSegment invariants

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

(As a syntactic first patch, I would like to rename CallStack to CallSegment because CallStack is a poor and confusing name.  I have received this feedback from several people and personally agree.)

The goal of the bug is to push CallSegments less frequently.  At the moment, there is an invariant which isn't really necessary that each JS engine activation/reentrance has its own CallSegment.  This means a CallSegment is pushed for every js::Invoke that (sometimes) goes between the arguments and the stackframe.  This gets in the way of bug 539144, which wants argv[0] to be a (jit-time) constant offset from fp.  Fixing this should also give a small speedup in js::Invoke which is showing up in shark on benchmarks.

Comment 1

[Brendan Eich [:brendan]]

CallSegment sounds like a segment of a call -- do you not want CallStackSegment? Or something shorter (CallFlow?).

/be

Comment 2

[Luke Wagner [:luke]]

CallStackSegment is what I originally had in mind... CallSegment was the abbreviation :P

Comment 3

[Brendan Eich [:brendan]]

That's like abbreviating "mailman" to "man" (excuse my gender-generic usage) :-P^2.

/be

Comment 4

[Luke Wagner [:luke]]

Fair enough, CallStackSegment it is.  But now I have to s/cs/css/ all those variable names ;-)

Comment 5

[Luke Wagner [:luke]]

Attachment: just the renaming

s/CallStack/CallStackSegment/
s/callstack/segment/ (in comments)
s/cs/css/ (variable names)
s/previousInThread/previousInMemory/ (based feedback that "thread" made no sense)

Comment 6

[Luke Wagner [:luke]]

Attachment: WIP

This passes trace-tests (but probably breaking a lame decompiler assumption in reftests).  By loosening the invariants I can avoid pushing segments for js::Invoke and do some inlining of StackSpace members called from Invoke.  On TM, this gives a .6% speedup on V8 (30ms) and .9% speedup on SS (5ms).

Comment 7

[Luke Wagner [:luke]]

Forgot to mention in comment 6, but after several bits of feedback I shortened CallStackSegment to StackSegment in the patch.  I thought this would be ambiguous-sounding, but others don't think so and it pairs well with js::StackSpace.

As predicted, bumping the down-frame's sp messes up the decompiler.  In theory, this is solved by a simple test to see whether the given vp is above the simulated stack depth found by ReconstructPCStack.  The problem is that the vp gets turned into spindex at a point where the simulated stack info is not available and, when this info is available, the vp is gone.  I think the solution is to take out spindex and instead pass a Value* + a separate enum param to indicate JSDVG_{IGNORE,SEARCH}_STACK.

Comment 8

[Luke Wagner [:luke]]

Attachment: decompiler changes

This small patch is a prerequisite for the one that pushes less segments.  It teaches the decompiler/js_ReportIsNotFunction not to be fooled by a bumped regs.sp.  Comments in the patch explain.

Comment 9

[Luke Wagner [:luke]]

Attachment: patch

Almost ready for review.

Comment 10

[Luke Wagner [:luke]]

I forgot to mention: I started doing what bug 580337 is talking about by introducing a NativeArgs structure that is used to build the arguments to invoke.  I think the resulting code is much more readable.

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 460220 [details] [diff] [review]
decompiler changes

I use "invariant" too, it sounds Cool and Important, but this bug reminds me of http://www.cs.yale.edu/quotes.html #1 and #62, which are so true.

>diff --git a/js/src/jsfun.cpp b/js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp
>+++ b/js/src/jsfun.cpp
>@@ -2557,24 +2557,43 @@ js_ValueToCallableObject(JSContext *cx, 
> 
> void
> js_ReportIsNotFunction(JSContext *cx, const Value *vp, uintN flags)
> {
>     const char *name = NULL, *source = NULL;
>     AutoValueRooter tvr(cx);
>     uintN error = (flags & JSV2F_CONSTRUCT) ? JSMSG_NOT_CONSTRUCTOR : JSMSG_NOT_FUNCTION;
>     LeaveTrace(cx);
>+
>+    /*
>+     * We try to the print the code that produced vp if vp is a value in the

"that produced *vp if vp points to a value on the"

This patch looks fine otherwise, but can you add a test or two, in particular showing the fallback dvg result?

/be

Comment 12

[Luke Wagner [:luke]]

(In reply to comment #11)
> I use "invariant" too, it sounds Cool and Important, but this bug reminds me of
> http://www.cs.yale.edu/quotes.html #1 and #62, which are so true.

Haha, nice.

> This patch looks fine otherwise, but can you add a test or two, in particular
> showing the fallback dvg result?

Does it suffice that 6 decompiler reftests fail with the segment-patch applied without this one?

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> > This patch looks fine otherwise, but can you add a test or two, in particular
> > showing the fallback dvg result?
> 
> Does it suffice that 6 decompiler reftests fail with the segment-patch applied
> without this one?

Probably -- I'm really asking for an example where the diagnostic degrades to the fallback string, and what it was like in (say) Firefox 3.6.

/be

Comment 14

[Luke Wagner [:luke]]

Ah, right.  IIUC, this patch shouldn't affect any error messages now.  With only the segment patch applied, some values would incorrectly take the "have a pc" path, instead of the fallback-string path.  An example from js1_6/decompilation/regress-352613-01.js is:

  "a".replace(/a/, Array.prototype.map)

which should print

  "a" is not a function

but prints 

  "a".replace is not a function

without the decompiler patch.

Comment 15

[Luke Wagner [:luke]]

Attachment: xpconnect changes

I removed the invokeFriendAPI from StackSpace and just had CallMethod go through JS_CallFunction like everybody else.

Comment 16

[Luke Wagner [:luke]]

Attachment: stack segment changes

Finally ready for review.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 460426 [details] [diff] [review]
stack segment changes

Oops, forgot the important case of fast-native called from JSAPI.  That breaks the fast native's assumption that it is called "from the engine", i.e., from an active frame.

Comment 18

[Luke Wagner [:luke]]

Attachment: trivial rename

This patch just renames things as discussed above.

Comment 19

[Luke Wagner [:luke]]

Attachment: simpler strategy

The previous approach (bumping regs->sp) breaks down when fast natives are called from js::Invoke.  Fixing that adds more complexity.  This patch uses a simpler strategy that achieves the same segment-elision for js::Invoke calls from an active segment.

I also noticed that FrameRegsIter showed up in callgrind (from its one hot callsite in the tracer's LeaveTree).  Inlining the 99% case brings the total speedup to

Comment 20

[Luke Wagner [:luke]]

(something bizarre happened while I was switching between tabs)
... 7.3ms (1.4%) on SS and 17ms (2.9%) on v8-deltablue.

Comment 21

[Luke Wagner [:luke]]

Attachment: stack segment changes v.3

Updated to work with the next patch.

Comment 22

[Luke Wagner [:luke]]

Attachment: CallArgs

This patch adds the vp[0]/vp[1]/vp[2] use in lieu of args.callee()/args.thisv()/args[0].  While updating the code to use this, I removed a few now-unnecessary auto rooters and also was able to simplify the case of Invoke called from the interpreter.  SS now shows a 9ms (1.7%) speedup.

Comment 23

[Robert Sayre]

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

Comment 24

[Jeff Walden [:Waldo]]

Comment on attachment 461410 [details] [diff] [review]
stack segment changes v.3

>+JS_REQUIRES_STACK
>+ExecuteFrameGuard::~ExecuteFrameGuard()
>+{
>+    if (!cx)
>+        return;

if (!pushed())

Do this in the other frame guard destructors, too (preserving likelihood hints).

(I must be missing something if this is all I have! ;-) )

Comment 25

[Jeff Walden [:Waldo]]

Comment on attachment 461412 [details] [diff] [review]
CallArgs

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

>+        args.callee() = calleev;
>+        args.thisv() = thisv;
>+        Value *argv = args.argv();
>         if (REDUCE_MODE(mode))
>-            *sp++ = *vp;
>-        *sp++ = tvr.value();
>-        sp++->setInt32(i);
>-        *sp++ = objv;
>+            *argv++ = *vp;
>+        argv[0] = tvr.value();
>+        argv[1].setInt32(i);
>+        argv[2] = objv;

Assigning to "argv" (psych!) is quite confusing to read.  This seems much preferable to me:

        args.callee() = calleev;
        args.thisv() = thisv;
        Value *argv = args.argv();
        size_t j = 0;
        if (REDUCE_MODE(mode))
            argv[j++] = *vp;
        argv[j++] = tvr.value();
        argv[j++].setInt32(i);
        argv[j++] = objv;


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

>+/*
>+ * This type can be used to call Invoke when the arguments have already been
>+ * pushed onto the stack as part of normal exeuction.
>+ */

"execution"


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

> /*
>+ * Abstracts the layout of the stack passed to natives from the engine and from
>+ * natives to js::Invoke.
>+ */
>+struct CallArgs
>+{
>+    Value *argv_;
>+    uintN argc_;
>+  protected:
>+    CallArgs() {}
>+    CallArgs(Value *argv, uintN argc) : argv_(argv), argc_(argc) {}
>+  public:
>+    Value *base() const { return argv_ - 2; }
>+    Value &callee() const { return argv_[-2]; }
>+    Value &thisv() const { return argv_[-1]; }
>+    Value &operator[](unsigned i) const { return argv_[i]; }

Assert in-range.

We're not horribly far off from being able to assert in-rangeness for all argument value gets/sets (possibly with a DEBUG-asserting iterator for the case or two where we bump along a pointer writing to arguments).  But it's not important to get that done here and now.

Comment 26

[Brendan Eich [:brendan]]

(In reply to comment #25)
> We're not horribly far off from being able to assert in-rangeness for all
> argument value gets/sets (possibly with a DEBUG-asserting iterator for the case
> or two where we bump along a pointer writing to arguments).

A good goal.

> But it's not important to get that done here and now.

File a bug and someone own it -- we should drive across the goal line for a TD. FIXME citation if appropriate.

/be

Comment 27

[Luke Wagner [:luke]]

Thanks for all the reviews!

http://hg.mozilla.org/tracemonkey/rev/7acc0fe02f45

Comment 28

[Robert Sayre]

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