Closed Bug 1503170 Opened 1 year ago Closed 1 year ago

Clean up JSOP_GOSUB/JSOP_RETSUB, remove RetSub IC

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox65 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(3 files)

To implement try-finally, JSOP_GOSUB pushes the bytecode offset of the next pc, then JSOP_RETSUB can use that to return to it. Baseline then has a RetSub IC to map from bytecode pc to native code pc. For bug 1499644 we need to fix this because ICs will be able to outlive the BaselineScript.

It would be much nicer to do something similar to JSOP_YIELD/YieldAndAwaitOffsets: make JSOP_GOSUB push a GoSubIndex and then make JSOP_RETSUB map from GoSubIndex to either bytecode pc or Baseline native code address.

We could then also simplify the bytecode a bit, like this:

JSOP_FALSE
JSOP_UINT24 GoSubIndex
JSOP_GOSUB // just the jump opcode

I wanted to work on this, then realized I should probably wait until the SharedScriptData cleanup (bug 1502481 et al) lands.

(Note: I think bug 965717 is the right long-term fix here, but I don't want to block bug 1499644 on that.)
Also instead of having separate-but-similar lists of indexes/offsets (YieldAndAwait, GoSub, etc) it might be nicer to use a shared list for:

* YieldAndAwait
* GoSub
* Later JSOP_LOOPENTRY, so the Baseline interpreter can do ultra fast OSR like this: JSOP_LOOPENTRY operand => look up in BaselineScript list => jump to it.

Ted, WDYT?
I think making GOSUB take an index and use the same list as yieldandawait makes sense. Maybe nonLocalOffsets? we need some ideas here.
Note: JSOP_GOSUB has a use count of 2 even though it doesn't push/pop anything.
This isn't great but was done to match the old behavior (def count of 0, pushed
2 values).

Depends on D10571
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
The micro-benchmark below improves from 225 ms to 145 ms with the second patch. This was a best case for the IC-based code: a micro-benchmark with 16 GoSubs for 1 RetSub improves from 172 ms to 48 ms.

function f() {
    var x = 0;
    for (var i = 0; i < 10000000; i++) {
        try {
        } finally {
            x++;
        }
    }
    return x;
}
var t = new Date;
f();
print(new Date - t);
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/90677f8ecf41
part 1 - Rename yieldAndAwait{Index,Offset} to resume{Index,Offset}. r=tcampbell
Keywords: leave-open
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5e76645a4e5c
part 2 - Use resume{Index,Offset} for JSOP_GOSUB/JSOP_RETSUB, remove RetSub IC. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/71bab79a1a0a
part 3 - Remove unused CGResumeOffsetList::numAwaits, move numYields to BytecodeEmitter. r=tcampbell
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.