885514 - IM: Compile try-finally

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Till Schneidereit [:till]]

I don't know if we're actually planning on doing this at all, so please close as WONTFIX if not. At the very least, we have a bug on file documenting that decision, then.

Comment 1

[Andy Wingo [:wingo]]

See bug 962599

Comment 2

[Iain Ireland [:iain]]

I think I've worked out a reasonable pathway to accomplish this, so I'm commandeering this bug.

A rough outline:

1. Remove JSOp::Gosub, which is basically just a JSOp::Goto that lies about its effect on the stack, and replace it with a regular JSOp::Goto.
2. JSOp::Retsub is currently a complicated opcode: it pops two values, and then uses one of them to decide whether it should throw the other as an exception, or use it as a resume index to jump back to the right place. We can simplify it by moving the exception-throwing behaviour into regular bytecode, reducing JSOp::Retsub into something like an indirect goto.
3. When we retsub from a finally block, we currently jump back to the opcode after the gosub. In the common case (the gosubs at the end of the try block and the catch block), we will immediately goto the opcode after the finally block. We can be more efficient by instead pushing a resume index that jumps directly to our final destination.
4. In cases involving non-local jumps (return statements, breaks, continues) we have to jump somewhere else. If iterators need to be closed, we may need to jump back and forth from iterator closure code to finally blocks. If we aren't in such a case, and the only possible target of the retsub is the opcode immediately following the finally block, then we can omit the retsub entirely and just fall through. This gives us Warp support effectively for free: we don't need to implement JSOp::Retsub, and JSOp::Finally is a no-op. (Note that, just like catch blocks today, Warp would not support resuming in the finally block when unwinding; we would bail out to blinterp instead. Being able to handle finally at all should still be a big win.)
5. We can incrementally add Warp support for more cases (eg returns) by desugaring them to normal jumps in the finally emitter. One approach, which is similar to what V8 does, would be to convert the retsub into a tableswitch, replacing the throwing boolean with a per-jump-to-finally-block token for the switch.

I believe this plan gives us a fairly smooth route to supporting almost everything in Warp. One possible exception is iterator closures during non-local jumps (eg a return that must execute a finally and then close an iterator): the resulting spaghetti-CFG might cause problems for some Ion optimizations. We can burn that bridge when we come to it, but for now I think we can cover 80% of the cases with 20% of the work.

I have a working implementation of parts 1 and 2. I'll put those up for review, then start working on parts 3 and 4.

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 885514: Refactor try/finally to use goto r=jandem

JSOp::Gosub is just a JSOp::Goto that lies about its effect on the stack. We can start simplifying try/finally by using a JSOp::Goto instead.

Currently, to make the stack depth work out when returning from a finally, Gosub pretends to pop two values (the arguments to retsub), and Finally pretends to push them. This means that bytecode analysis can treat Gosub as if it falls through, and the stack depth of the jumptarget following the gosub will be correct. This patch uses a different approach: bytecode analysis marks the return address as reachable when we push it using JSOp::ResumeIndex. This means that the bytecode no longer has to lie about stack depth.

One small complication is in the decompiler, where the stack depth at the finally jumptarget is 2 deeper than at the try op. We previously pretended that those values were pushed by the finally; to make the decompiler work, we now point to the JSOp::Try itself.

JSOp::Gosub is no longer used. The next patch removes it.

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 885514: Remove JSOp::Gosub r=jandem

Depends on D140269

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 885514: Simplify JSOp::Retsub r=jandem

Currently, JSOp::Retsub takes two arguments, and uses one to decide whether to throw the second or use it as a resume index. This patch splits out the exception-handling logic into explicit bytecode, leaving JSOp::Retsub as a simple indirect jump.

To make the generated bytecode work out nicely, I've swapped the order of the arguments: now throwing is on top, and the exception/resumeindex argument is underneath it.

Depends on D140270

Comment 6

[Katelyn Gadd (:kael)]

Sorry for bug noise: Is it the case that try-finally is still a full deopt in spidermonkey? I was under the impression that had been at least partially fixed (like how v8 got rid of the deopt a while ago), but your progress on this bug made me realize it might not actually have been fixed and I've been writing some low-performance JS :-) Or is it just that you're working on improving its performance by making it efficient in some of the higher-performance code generators specifically?

Comment 7

[Iain Ireland [:iain]]

finally is fully supported in three of our four tiers, but is completely unsupported in Ion/Warp: we disable top-tier compilation entirely for scripts that use finally. (For reference, our current tiers are the C++ interpreter, the baseline interpreter, the baseline compiler, and Ion/Warp.)

My goal here is to pluck the lowest-hanging fruit, making it possible for most code using try/finally to be optimized in Ion. It will still be the case that A) there may be some more complicated cases left to address, and B) when unwinding exceptions, we don't resume in the top tier. We resume in the baseline interpreter, and then potentially re-enter Ion via OSR.

Comment 8

[Lars T Hansen [:lth]]

Being able to handle finally at all should still be a big win.

Yes please!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5f6b9e2fc3bb
Refactor try/finally to use goto r=jandem
https://hg.mozilla.org/integration/autoland/rev/1cb0b352b024
Remove JSOp::Gosub r=jandem
https://hg.mozilla.org/integration/autoland/rev/eeaeb40f8b86
Simplify JSOp::Retsub r=jandem

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/5f6b9e2fc3bb
https://hg.mozilla.org/mozilla-central/rev/1cb0b352b024
https://hg.mozilla.org/mozilla-central/rev/eeaeb40f8b86

Comment 11

[Iain Ireland [:iain]]

Attachment: Bug 885514: Improve control flow after finally r=jandem

At the end of a try block, we currently jump to the finally block, retsub back to the end of the try block, and then immediately jump to the code following the finally block. The same is true for the end of a catch block.

As a first step in cleaning this up, this patch rewrites the try and catch blocks to instead push a resume index that will jump directly to the code following the finally block, without the extra hop. Prior to this patch, a try block looked like this:

Try
[Body]
ResumeIndex <N> // push resume index for jump target below
False
Goto <N>        // jump to finally block
JumpTarget      // target of ResumeIndex
Goto <N>        // jump to code after finally block

After this patch, a try block looks like this:

Try
[Body]
ResumeIndex <N> // push resume index for code after finally block
False
Goto <N>        // jump to finally block

One subtle note: in TryEmitter::emitEnd, we emit the jump target first and then allocate the resume index based on the last target offset. This is necessary because there's a jump target emitted by the InternalIfEmitter in the finally block, and emitJumpTarget will alias consecutive jump targets. If we allocated the resume index first, like the existing code in jumpToFinally, then it would not point to the jump target.

Comment 12

[Iain Ireland [:iain]]

Attachment: Bug 885514: Don't emit retsub if all paths fall through r=jandem

This means that we don't have to implement retsub in Warp to handle simple cases.

Depends on D142036

Comment 13

[Iain Ireland [:iain]]

Attachment: Bug 885514: Don't emit top-level GetRval/SetRval in TryEmitter r=arai

Depends on D142037

Comment 14

[Iain Ireland [:iain]]

Attachment: Bug 885514: Support bailouts for finally r=jandem

This is mostly modeled after the catch code. The main difference is that finally expects to resume in code where there are two additional values on the stack, so we store the exception in the ExceptionBailoutInfo and write it into the stack frame in BaselineStackBuilder.

Depends on D142117

Comment 15

[Iain Ireland [:iain]]

Attachment: Bug 885514: Support finally in Warp r=jandem

This supports any try-finally that doesn't emit a JSOp::Retsub. Currently that means that we don't support break, continue, or return inside the try block, but there's room to fix that in the future.

Depends on D142118

Comment 16

[Iain Ireland [:iain]]

Attachment: Bug 885514: Add tests r=jandem

Depends on D142119

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/946807443c67
Improve control flow after finally r=jandem
https://hg.mozilla.org/integration/autoland/rev/909dc8b88dd6
Don't emit retsub if all paths fall through r=jandem
https://hg.mozilla.org/integration/autoland/rev/8cd3f1a267ae
Don't emit top-level GetRval/SetRval in TryEmitter r=arai
https://hg.mozilla.org/integration/autoland/rev/7f75f8b705e7
Support bailouts for finally r=jandem
https://hg.mozilla.org/integration/autoland/rev/f5e7bea40350
Support finally in Warp r=jandem
https://hg.mozilla.org/integration/autoland/rev/3358bc3f9c08
Add tests r=jandem

Comment 18

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/946807443c67
https://hg.mozilla.org/mozilla-central/rev/909dc8b88dd6
https://hg.mozilla.org/mozilla-central/rev/8cd3f1a267ae
https://hg.mozilla.org/mozilla-central/rev/7f75f8b705e7
https://hg.mozilla.org/mozilla-central/rev/f5e7bea40350
https://hg.mozilla.org/mozilla-central/rev/3358bc3f9c08

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Can we close this bug?

Comment 20

[Iain Ireland [:iain]]

Sure. There is some follow-up work, but I can do it in another bug.