Closed Bug 885514 Opened 11 years ago Closed 2 years ago

IM: Compile try-finally

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
100 Branch
Tracking Status
firefox100 --- fixed

People

(Reporter: till, Assigned: iain)

References

(Blocks 4 open bugs)

Details

(Whiteboard: [js:t])

Attachments

(9 files)

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.
Blocks: 885526
Blocks: JSIL
Blocks: 922053
Assignee: general → nobody
Severity: normal → --
Type: defect → enhancement
Priority: -- → P3

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.

Assignee: nobody → iireland

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.

Depends on D140269

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

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?

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.

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

Yes please!

Keywords: leave-open
Blocks: 1759825

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.

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

Depends on D142036

Depends on D142037

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

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

Attached file Bug 885514: Add tests r=jandem (deleted) —

Depends on D142119

Pushed by iireland@mozilla.com:
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
Regressions: 1762769
Regressions: 1762770

Can we close this bug?

Flags: needinfo?(iireland)

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

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(iireland)
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 100 Branch
Regressions: 1767181
Regressions: 1768660
Regressions: 1791968
No longer regressions: 1811171
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: