550639 - remove JSStackFrame::rval (!)

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

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.

Comment 1

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

Attachment: fix

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.

Comment 2

[Jason Orendorff [:jorendorff]]

I'm kind of iffy on this patch. Does it improve performance measurably? It seems more complicated.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 440675 [details] [diff] [review]
fix

The patch fails this test.

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

assertEq(f(9), 9);

Comment 4

[Jason Orendorff [: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.

Comment 5

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

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.

Comment 6

[Jason Orendorff [:jorendorff]]

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.

Comment 7

[Brendan Eich [:brendan]]

Yes, rval must go. Never give up!

/be

Comment 8

[Luke Wagner [:luke]]

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.