Closed Bug 547851 Opened 12 years ago Closed 11 years ago

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

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: luke, Assigned: luke)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 4 obsolete files)

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.
Attached patch WIP v.1 (obsolete) — Splinter Review
One TODO left, and bugs.
Assignee: general → lw
Status: NEW → ASSIGNED
Attached patch better (obsolete) — Splinter Review
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.
Attachment #429284 - Attachment is obsolete: true
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.
Attached patch fix, commented (obsolete) — Splinter Review
Fixed bug in firstUnused().  Windows shows 1% improvement in v8 (with tracing) and SS (without).
Attachment #429917 - Attachment is obsolete: true
No longer depends on: 550210
Attached patch with changes (obsolete) — Splinter Review
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.
Attachment #430206 - Attachment is obsolete: true
Attached patch rebasedSplinter Review
Bug 540706 is about to land, time to get this reviewed.
Attachment #432847 - Attachment is obsolete: true
Attachment #444534 - Flags: review?
Attachment #444534 - Flags: review? → review?(dvander)
Umm, ignore that '// TODO: here' bit.
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.
Attachment #444534 - Flags: review?(dvander) → review+
(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.
http://hg.mozilla.org/tracemonkey/rev/c96ba53e745f
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/c96ba53e745f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
(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.)
Oops.  I must have meant to and then changed my mind and forgot the decl.
(In reply to comment #14)
Fixed.
Depends on: 588558
Depends on: 607513
Depends on: 621137
You need to log in before you can comment on or make changes to this bug.