965717 - Fix try/finally, GOSUB, RETSUB mess by duplicating finally blocks

Status: RESOLVED DUPLICATE of bug 1766730

(Core :: JavaScript Engine, defect)

Description

[Andy Wingo [:wingo]]

The current implementation of "finally" has a number of drawbacks:

  (1) It prevents optimization, as GOSUB is not optimized.
  (2) It is the only user of GOSUB and RETSUB.
  (3) It relies on for-in pushing its iter on the stack in order to use the stack depth to dynamically determine which try note applies.  See comment in Interpreter.cpp:TryNoteIter::settle.

There is a simple fix to this: simply duplicate the finally code whenever you would do a GOSUB.  You would also have to reify a "finally" block used when handling exceptions, but because it doesn't return (it just re-throws) it doesn't need GOSUB/RETSUB.  Finally, try notes will have to be organized in a tree in the same way as BlockScopeNotes are, with parent links, so that non-local exits can locally "pop" off try notes.

The implementation of this should be straightforward, all in BytecodeEmitter's NonLocalExitScope helper.

It seems I don't need this for my other work so anyone can take this bug.

Comment 1

[Jan de Mooij [:jandem]]

I've been thinking a bit about Ion-compiling try-finally and it seems we either want to inline the finally block in IonBuilder when we see a GOSUB or at the bytecode level as you're suggesting. I really like this idea.

Also, Baseline compiles GOSUB and RETSUB, but RETSUB currently uses an IC to map from bytecode offset on the stack to native code address. That works and is reasonably fast, but it's a bit of a hack and directly inlining the finally block in the bytecode should be faster of course.

It will increase bytecode size but that's probably ok. We could instrument the emitter, browse around and get some numbers. AFAIK most finally blocks are pretty small though.

Comment 2

[Jason Orendorff [:jorendorff]]

This will probably also fix the amazingly obscure bug 718531.

Comment 3

[Andy Wingo [:wingo]]

This would allow StackFrame and BaselineFrame to remove the "rval" member, I think.

Comment 4

[Shu-yu Guo [:shu]]

I'd like to look at this eventually.

Comment 5

[Lars T Hansen [:lth]]

Not quite the same issue but pertains to Jan's comment, at least:

- try-finally is very slow
- try-catch simulating try-finally is not quite as slow, but slow enough to matter

I expect these patterns to become somewhat more common in JS because, with locks in shared memory, you usually want to be sure to unlock on your way out of a critical section, no matter how you're leaving it, and a catch or finally clause is really needed.

Some experiments here: https://github.com/lars-t-hansen/js-lock-and-condition/issues/3#issuecomment-307998872

Comment 7

[Iain Ireland [:iain]]

Bug 1766730 solves this with a slightly different approach.