Closed Bug 550639 Opened 11 years ago Closed 11 years ago

remove JSStackFrame::rval (!)


(Core :: JavaScript Engine, defect)

Not set





(Reporter: luke, Assigned: dvander)




(1 file)

FWIW, JSStackFrame::rval almost isn't necessary; it sits mostly unused until right before returning.  I see there is trickiness with JSOP_SETRVAL and JSOP_POPV, but it seems like these might be changed to do some stack trickery instead.  As is, this will save 1 word in JSStackFrame and 1 load/store for each JSOP_RETURN.  With the dual stack layout (bug 549143), assuming we want to store rval unboxed, the cost is more like 3 loads/stores/bytes.

One seeming problem is JS_{Set,Get}FrameReturnValue in the debug API, but mxr says this isn't actually used, so we can remove it.
Blocks: 557378
Assignee: general → dvander
Depends on: 560988
Summary: remove JSStackFrame::rval (?) → remove JSStackFrame::rval (!)
Attached patch fixSplinter Review
The interpreter has to take special care not to let inline calls muck with return-in-eval. This passes trace-tests and jstests, and includes one new test case that wasn't covered.
Attachment #440675 - Flags: review?(jorendorff)
I'm kind of iffy on this patch. Does it improve performance measurably? It seems more complicated.
Comment on attachment 440675 [details] [diff] [review]

The patch fails this test.

function f(a) {
    try {
        return a;
    } finally {
        if (a)
            f(a - 1);

assertEq(f(9), 9);
Attachment #440675 - Flags: review?(jorendorff)
At first, it seemed to me that the test in comment 3 meant that in the worst case, we really do need an rval slot per stack frame.

But that's not necessarily true. We could use the operand stack for return value slots. I think this can still be done.
Yuck, thanks. With fat values it'll be pretty annoying to propagate 128-bit values to rval just to write them back again. With the method JIT it's just extra instructions, and the inline return path is hot.

I'll see how much of a win it is to not have rval in JM - if it doesn't matter I'll drop this bug.
Don't give up! The more I think about this the more I want rval gone.

I think the simple way to do it is to say, the caller provides a rooted return-value address, always. js_Interpret can take one as an argument, as in your patch. The only problem is inline calls, as in comment 3; and in those cases we can use &argv[-2], which is a location on the caller's operand stack. (This is how JSFastNatives work already.)

And look, we'll save a move in JSOP_RETURN under inline_return.  ;)

         /* Store the return value in the caller's operand frame. */
         regs.sp -= 1 + (size_t) ifp->frame.argc;
-        regs.sp[-1] = fp->rval;

Sorry for the discouraging words earlier. Maybe the individual perf win here won't be measurable, but I have to believe that trimming the fat as a general trend points us in the right direction.
Yes, rval must go. Never give up!

No longer blocks: 557378
Bug 587707 and the followup bug 593882 don't set or read from rval on fast paths, so its probably not worth it to go through all the effort just to save a few bytes.
Closed: 11 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.