1521491 - Store IC index in jump target ops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Jan de Mooij [:jandem]]

I have some patches to store the IC index (= number of JOF_IC ops) in jump target ops. The interpreter will use this to set the frame's ICEntry* pointer at jump target ops and then it will just bump this pointer after each JOF_IC op. This way the interpreter can use Baseline ICs with minimal overhead compared to the Baseline JIT.

This is easier now that the BCE prologue/main split is gone (bug 1284719) and we can assert correctness in ICScript::create.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1521491 part 1 - Stop treating JSOP_ENDITER as a jump target op. r?nbp!

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1521491 part 2 - Stop treating JSOP_TRY as a jump target op. r?nbp!

Depends on D17112

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1521491 part 3 - Fold BytecodeEmitter::checkTypeSet into BytecodeEmitter::emitCheck. r?tcampbell!

The bytecode emitter used to call checkTypeSet for each JOF_TYPESET op. Despite
correctness asserts in the TypeScript code, this was pretty error prone. Doing
something similar for JOF_IC ops would have made it worse.

The solution is to move this check to BytecodeEmitter::emitCheck (called for
each opcode we emit), so we don't have to worry about this anymore.

Depends on D17113

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1521491 part 5 - Store IC index in jump target ops. r?tcampbell!

The interpreter will use this to set the frame's ICEntry* pointer at jump target
ops and then it will just bump this pointer after each JOF_IC op. This way the
interpreter can use Baseline ICs with minimal overhead compared to the Baseline
JIT.

Depends on D17114

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2058451bd944
part 1 - Stop treating JSOP_ENDITER as a jump target op. r=nbp
https://hg.mozilla.org/integration/autoland/rev/92a00136c2fd
part 2 - Stop treating JSOP_TRY as a jump target op. r=nbp
https://hg.mozilla.org/integration/autoland/rev/a9a95a138e04
part 3 - Fold BytecodeEmitter::checkTypeSet into BytecodeEmitter::emitCheck. r=tcampbell
https://hg.mozilla.org/integration/autoland/rev/159930eacfd7
part 4 - Store IC index in jump target ops. r=tcampbell

Comment 6

[Dorel Luca [:dluca]]

Backed out changeset 159930eacfd7 (bug 1521491) for spidermonkey failure

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=224454044&repo=autoland&lineNumber=6041

Push with failures:
https://hg.mozilla.org/integration/autoland/rev/159930eacfd7a4a9933d26655f017def45919948

Backout:
https://hg.mozilla.org/integration/autoland/rev/536917ba6bcf6a88d83f6d7551a50086cbdee1e9

Comment 7

[Jan de Mooij [:jandem]]

Silly rooting hazard while emitting JSOP_LABEL. Maybe now is a good time to stop using JOF_JUMP for JSOP_LABEL.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1521491 part 4 - Stop using JOF_JUMP for JSOP_LABEL. r?tcampbell!

JSOP_LABEL isn't a jump instruction at all. This patch adds JOF_CODE_OFFSET and
uses it for JSOP_LABEL. This lets us simplify some code and avoids a rooting
hazard with the next patch.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/731a43e841ab
part 4 - Stop using JOF_JUMP for JSOP_LABEL. r=tcampbell

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/037f2dd3a63f
part 5 - Store IC index in jump target ops. r=tcampbell

Comment 11

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/2058451bd944
https://hg.mozilla.org/mozilla-central/rev/92a00136c2fd
https://hg.mozilla.org/mozilla-central/rev/a9a95a138e04

Comment 12

[Jan de Mooij [:jandem]]

Oh wait, parts 4 and 5 still need to merge to m-c but didn't yet because of the big ChromeUtils.import changes. Should happen soon though.

Comment 13

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/731a43e841ab
https://hg.mozilla.org/mozilla-central/rev/037f2dd3a63f

Comment 14

[Eric Rahm [:erahm]]

It looks like there was a 38K base memory regression associated with this.

Jan, is this something we can work around or do we need to accept the regression?

Comment 15

[Ted Campbell [:tcampbell]]

Bug 1523749 will gain back most of this regression, but some of it will probably remain for perf reasons.

Comment 16

[Jan de Mooij [:jandem]]

What Ted said. We'll have to accept this one for now unfortunately.