Closed Bug 579183 Opened 11 years ago Closed 11 years ago

loosen-up StackSegment invariants

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(5 files, 5 obsolete files)

(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.
Blocks: 539144
CallSegment sounds like a segment of a call -- do you not want CallStackSegment? Or something shorter (CallFlow?).

/be
CallStackSegment is what I originally had in mind... CallSegment was the abbreviation :P
That's like abbreviating "mailman" to "man" (excuse my gender-generic usage) :-P^2.

/be
Fair enough, CallStackSegment it is.  But now I have to s/cs/css/ all those variable names ;-)
Attached patch just the renaming (obsolete) — Splinter Review
s/CallStack/CallStackSegment/
s/callstack/segment/ (in comments)
s/cs/css/ (variable names)
s/previousInThread/previousInMemory/ (based feedback that "thread" made no sense)
Attachment #459514 - Flags: review?(dvander)
Attachment #459514 - Flags: review?(dvander) → review+
Attached patch WIP (obsolete) — Splinter Review
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).
Attachment #459514 - Attachment is obsolete: true
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.
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.
Attachment #460104 - Attachment is obsolete: true
Attachment #460220 - Flags: review?(brendan)
Attached patch patch (obsolete) — Splinter Review
Almost ready for review.
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 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
Attachment #460220 - Flags: review?(brendan) → review+
(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?
(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
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.
I removed the invokeFriendAPI from StackSpace and just had CallMethod go through JS_CallFunction like everybody else.
Attachment #460425 - Flags: review?(mrbkap)
Attached patch stack segment changes (obsolete) — Splinter Review
Finally ready for review.
Attachment #460221 - Attachment is obsolete: true
Attachment #460426 - Flags: review?(jwalden+bmo)
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.
Attachment #460426 - Flags: review?(jwalden+bmo)
Attachment #460425 - Flags: review?(mrbkap) → review+
Attached patch trivial renameSplinter Review
This patch just renames things as discussed above.
Attachment #460426 - Attachment is obsolete: true
Attachment #461366 - Flags: review?(jwalden+bmo)
Attached patch simpler strategy (obsolete) — Splinter Review
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
Attachment #461370 - Flags: review?(jwalden+bmo)
(something bizarre happened while I was switching between tabs)
... 7.3ms (1.4%) on SS and 17ms (2.9%) on v8-deltablue.
Updated to work with the next patch.
Attachment #461370 - Attachment is obsolete: true
Attachment #461410 - Flags: review?(jwalden+bmo)
Attachment #461370 - Flags: review?(jwalden+bmo)
Attached patch CallArgsSplinter Review
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.
Attachment #461412 - Flags: review?(jwalden+bmo)
Attachment #461366 - Flags: review?(jwalden+bmo) → review+
http://hg.mozilla.org/mozilla-central/rev/b1b935db8b1a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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! ;-) )
Attachment #461410 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #461412 - Flags: review?(jwalden+bmo) → review+
(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
Summary: loosen-up callsegment invariants → loosen-up StackSegment invariants
Thanks for all the reviews!

http://hg.mozilla.org/tracemonkey/rev/7acc0fe02f45
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/b16d966d3b6f
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 604210
You need to log in before you can comment on or make changes to this bug.