547851 - remove JSStackFrame::regs, JSStackFrame::callerFrame.sp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

The contiguous-stack patch makes the value of fp->regs->sp implicit in the up-frame's address for all but the top frame.  For the top stack frame, we can have the context point directly to the active JSFrameRegs.  This would break down if we wanted fp->regs->sp for a random 'fp' (since we have no direct link to the up-frame), but this does not appear to happen anywhere important.  With this in place, we can store a frame's own saved 'pc' in the frame (instead of its up-frame) and defer to cx->regs->pc for the active stack frame's pc.  With this, we can drop 'regs' altogether.

This also works well with JM, which can save the frame's $eip instead of the bytecode pc and map to bytecode pc on demand.

Comment 1

[Luke Wagner [:luke]]

Attachment: WIP v.1

One TODO left, and bugs.

Comment 2

[Luke Wagner [:luke]]

Attachment: better

The patch passes trace, ref, and xpcshell-tests.  Not including the performance gains of the contiguous stack patch (bug 540706) on top of which this patch applies, on Linux with tracing on, V8 is 2.3% faster.  Since it traces so well, SS is unaffected, but with tracing off, SS is 4.9% faster.  Strangely, OS X shows no speedup (with early-boyer 5.9% slower).  Will shark tomorrow.

Comment 3

[Luke Wagner [:luke]]

Upon further investigation of earlyboyer, the entire extra time is spent inside js_Interpret, which was hardly touched; the only changes are in JSOP_CALL/JSOP_RETURN, which do strictly less/simpler work.  JSStackFrame was changed, but only to be smaller.  Changing JSStackFrame to have the old layout had no effect.  Finally, earlyboyer and v8 in general showed no slowdown on Windows, so I am ignoring this.  There should be benefits for JM.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix, commented

Fixed bug in firstUnused().  Windows shows 1% improvement in v8 (with tracing) and SS (without).

Comment 5

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

http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/rev/128edcb4e262
http://hg.mozilla.org/users/danderson_mozilla.com/jaegermonkey/rev/e143da3bfdc0

Comment 6

[Luke Wagner [:luke]]

Attachment: with changes

This patch changes the way cx->regs is handled for calls.  Before, cx->regs (and before than, fp->regs) would start out null when a new frame was pushed and become non-null when js_Interpret started.  The new patch makes cx->regs == NULL iff cx->fp == NULL.  This simplifies several things in the existing code and upcoming in JM.

One change is that, before, if fp->regs was non-null, then fp->regs->pc would be non-null.  With this change, cx->regs will be non-null for slow natives, but cx->regs->pc will be null.

Comment 7

[Luke Wagner [:luke]]

Attachment: rebased

Bug 540706 is about to land, time to get this reviewed.

Comment 8

[Luke Wagner [:luke]]

Umm, ignore that '// TODO: here' bit.

Comment 9

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

Comment on attachment 444534 [details] [diff] [review]
rebased

> struct JSStackFrame
> {
>-    JSFrameRegs         *regs;

yaaaaaay

>+js_DumpStackFrame(JSContext *cx, JSStackFrame *start)
>+{
>+    LeaveTrace(cx);

This could wreak havoc if trying to debug while on trace. Do we need it?

>-    for (fp = js_GetTopStackFrame(cx); fp && !fp->regs; fp = fp->down)
>-        JS_ASSERT(!fp->script);
>+    for (FrameRegsIter i(cx); !i.done() && !i.pc(); ++i)
>+        JS_ASSERT(!i.fp()->script);
>     filename = NULL;
>     lineno = 1;
>-    if (fp) {
>-        op = (JSOp) *fp->regs->pc;
>+    JSStackFrame *fp = cx->fp;
>+    if (cx->fp && cx->regs->pc) {
>+        op = (JSOp) *cx->regs->pc

The old code finds the scripted caller, the new code only checks if the immediate caller is scripted. Is that intended?

r=me though I would like to understand these two cases before landing.

Comment 10

[Luke Wagner [:luke]]

(In reply to comment #9)
> >+js_DumpStackFrame(JSContext *cx, JSStackFrame *start)
> >+{
> >+    LeaveTrace(cx);
> 
> This could wreak havoc if trying to debug while on trace. Do we need it?

Heh, totally.  Replaced with VOUCH_DOES_NOT_REQUIRE_STACK().

> The old code finds the scripted caller, the new code only checks if the
> immediate caller is scripted. Is that intended?

Nope, total thinko, good catch.

Comment 11

[Luke Wagner [:luke]]

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

Comment 12

[Robert Sayre]

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

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #11)
> http://hg.mozilla.org/tracemonkey/rev/c96ba53e745f

So this patch renamed js_DumpStackFrame to js_DumpStackFrameChain in jsobj.h, but didn't rename the implementation.  (It did change the parameters to both, though.)  Was the rename intended, or not?

(I noticed because I had some debugging code in XPConnect that called js_DumpStackFrame.)

Comment 14

[Luke Wagner [:luke]]

Oops.  I must have meant to and then changed my mind and forgot the decl.

Comment 15

[Luke Wagner [:luke]]

(In reply to comment #14)
Fixed.