1503170 - Clean up JSOP_GOSUB/JSOP_RETSUB, remove RetSub IC

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

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.)

Comment 1

[Jan de Mooij [:jandem]]

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?

Comment 2

[Ted Campbell [:tcampbell]]

I think making GOSUB take an index and use the same list as yieldandawait makes sense. Maybe nonLocalOffsets? we need some ideas here.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1503170 part 1 - Rename yieldAndAwait{Index,Offset} to resume{Index,Offset}. r?tcampbell

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1503170 part 2 - Use resume{Index,Offset} for JSOP_GOSUB/JSOP_RETSUB, remove RetSub IC. r?tcampbell

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

Comment 5

[Jan de Mooij [:jandem]]

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);

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1503170 part 3 - Remove unused CGResumeOffsetList::numAwaits, move numYields to BytecodeEmitter. r?tcampbell

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90677f8ecf41
part 1 - Rename yieldAndAwait{Index,Offset} to resume{Index,Offset}. r=tcampbell

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/90677f8ecf41

Comment 10

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/5e76645a4e5c
https://hg.mozilla.org/mozilla-central/rev/71bab79a1a0a