Last Comment Bug 866888 - IonMonkey: Compile try-catch
: IonMonkey: Compile try-catch
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla26
Assigned To: Jan de Mooij [:jandem]
: general
:
Mentors:
: 543875 (view as bug list)
Depends on: 902978 906181 908625
Blocks: IonSpeed JSIL 841956 909389
  Show dependency treegraph
 
Reported: 2013-04-29 13:06 PDT by Jan de Mooij [:jandem]
Modified: 2013-09-03 02:54 PDT (History)
8 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (31.62 KB, patch)
2013-07-18 10:07 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
WIP v2 (49.90 KB, patch)
2013-07-19 07:16 PDT, Jan de Mooij [:jandem]
no flags Details | Diff | Splinter Review
Part 1 - Add try source note (5.98 KB, patch)
2013-07-19 08:12 PDT, Jan de Mooij [:jandem]
luke: review+
Details | Diff | Splinter Review
Part 2 - Add ScriptAnalysis::hasTryFinally (2.17 KB, patch)
2013-07-19 08:21 PDT, Jan de Mooij [:jandem]
bhackett1024: review+
Details | Diff | Splinter Review
Part 3 - Change bailout overrecursion check (2.98 KB, patch)
2013-07-19 08:50 PDT, Jan de Mooij [:jandem]
kvijayan: review+
Details | Diff | Splinter Review
Part 4 - Add bailout tail stub (12.31 KB, patch)
2013-07-19 09:52 PDT, Jan de Mooij [:jandem]
hv1989: review+
Details | Diff | Splinter Review
Part 5 - Compile try-catch (61.92 KB, patch)
2013-07-22 05:10 PDT, Jan de Mooij [:jandem]
kvijayan: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2013-04-29 13:06:52 PDT
Most of our main benchmarks don't use try-catch, but IIRC Octane-gameboy does (not sure if this is in hot code though) and also other benchmarks/websites like JSIL-generated code want this.

Doesn't seem easy, but would be nice if we could make it work. A while ago dvander suggested storing all vars that are accessed in the try-block in the call object, that may make it easier.
Comment 1 David Anderson [:dvander] 2013-04-29 13:10:04 PDT
Would the JSIL-compiled code expect the exception block to stay within IonMonkey?
Comment 2 K. Gadd (:kael) 2013-04-29 13:11:46 PDT
If there's an alternative pattern that Ion can compile more easily (or already compile) that is preferred to traditional try/catch, I may be able to generate it in JSIL.

For example, I've considered hoisting try/catch bodies out into functions so that I don't have to care about the ability to compile the actual try/catch. But at present I'm not doing that because my last tests showed that the overhead of constructing the closures would be enormous.

@dvander: No, exceptions are exceptional conditions in .NET (i.e. not used for flow control or basic logic like KeyError in python) so the catch body doesn't need to be an optimized hot path. Code paths that actually throw don't need to be opted either (Though functions that contain a throw on an untaken branch probably do).
Comment 3 Kannan Vijayan [:djvj] 2013-04-30 12:38:24 PDT
I think ti's reasonable to assume for most code that exceptions are exceptional.  Is there anywhere else in JS which uses exceptions for non-exceptional control flow?

The simple approach would be if the catch block would be entered within an ion frame, then we unwind up to that frame, then invalidate the ion code, and bail back to baseline to handle the try/catch.
Comment 4 K. Gadd (:kael) 2013-04-30 12:58:20 PDT
Exceptions for control flow might be common in parts of the JS community, for nonlocal return or whatever. Rubyists or Functional Programming types, perhaps? I've only ever had cause once to use an exception for control flow, and I don't remember running across examples of it in JS, but I bet they're out there. Someone like dherman might know. I don't know if those use cases are common enough to demand high performance.
Comment 5 Kannan Vijayan [:djvj] 2013-04-30 13:17:50 PDT
In those cases, we will hit a bailout (or maybe a few, depending on how we implement it), and then choose not to ion-compile after that.

E.g. we keep a count on the script of the number of times a catch block has been hit in a script.  If it's beyond a certain threshold, we refrain from ion compiling it in the future.

The benefits of this approach is that it allows Ion to compile try blocks as if the catch block was nonexistant - with full optimization for vars, etc.  It also requires fewer changes to codegen.

For code which DOES use catch blocks for control flow, we'll learn that early on (probably within baseline itself, before we even compile with ion the first time), and then just refuse to compile with Ion.

It's a good compromise.  Code which uses exceptions for control flow gets "kind of fast" baseline treatment.  Code which has try/catch, but where exceptions are exceptional, gets the full Ion treatment and runs the same speed as if the try/catch was not there at all.
Comment 6 Jan de Mooij [:jandem] 2013-07-18 10:07:01 PDT
Created attachment 777898 [details] [diff] [review]
WIP

Bug 716647 has some overlap with Ion try-catch support so I decided to get this done first.

Attached is a hackish WIP patch that works on a bunch of simple tests I tried. Ion compiles the try-block and ignores the catch block completely. When an exception is thrown, we bailout and resume into baseline code. Invoking the bailout machinery from the exception handler turned out to be pretty straight-forward.

There's some known problems with inlining but these should be easy to fix.

Some notes:

(1) Phi elimination does not know about uses in the catch block (because we don't compile it) so it's too aggressive. This testcase prints "undefined" instead of "5".

function f() {
    try {
        var foo = 5;
        for (var i=0; i<10000; i++) {
            if (i > 4000)
                throw 3;
        }
    } catch(e) {
        print(foo);
    }
}
f();

Should be possible to fix this by making phi elimination less aggressive in code containing try blocks.

(2) OSR is problematic in case like this:

try {
    throw 2;
} catch(e) { }

while (true) {}

Because Ion only compiles the try-block, it also doesn't compile the loop (since the "throw" terminates control flow. Initially I think we should disable OSR for scripts containing try-blocks - later on we can fix this by making the block following the try a separate entry point (like the OSR block), so that UCE does not eliminate it etc.

(3) For try-finally, I think we should inline the |finally| body in all places that have a GOSUB to it. When we have to resume into the |finally| block for an exception, we can bailout like we do for try-catch. Try-finally will be follow-up work though.
Comment 7 Jan de Mooij [:jandem] 2013-07-19 07:16:19 PDT
Created attachment 778464 [details] [diff] [review]
WIP v2

Final WIP. Passes jit-tests and jstests with --ion-eager. I'm pretty happy with the way try-catch support fits in, it didn't require any major changes.

I will split this up now (although the patch isn't very big) and add more comments/cleanup. I also want to try to land this the v8 way: behind a shell flag, then remove the flag after fuzzing etc.
Comment 8 Jan de Mooij [:jandem] 2013-07-19 08:12:59 PDT
Created attachment 778482 [details] [diff] [review]
Part 1 - Add try source note

This adds a source note to JSOP_TRY to store the offset to the jump at the end of the try block. Ion needs this to know where the try block ends and the jump then tells us where the catch block ends.

We could loop over the try notes to see where the catch block starts, then look at the previous op somehow, but I think this is faster and simpler.
Comment 9 Jan de Mooij [:jandem] 2013-07-19 08:21:01 PDT
Created attachment 778484 [details] [diff] [review]
Part 2 - Add ScriptAnalysis::hasTryFinally

IonBuilder will go from the end of the try block to the instruction following the catch/finally blocks. This means it could skip over a JSOP_FINALLY and happily continue, even though we don't handle finally yet.

This patch adds ScriptAnalysis::hasTryFinally so that we can abort explicitly if there are |finally| blocks.
Comment 10 Jan de Mooij [:jandem] 2013-07-19 08:50:43 PDT
Created attachment 778499 [details] [diff] [review]
Part 3 - Change bailout overrecursion check

The overrecursion check works fine atm because we always bailout the topmost frame. However, with try-catch it will be possible to bailout an older frame, so if the stack looks like this, older to newer:

frame 1 (has try-catch)
frame 2
frame 3
frame 4 (throws)

Unwinding frames 2-4 may free quite a lot of memory and we should check the new sp instead of the current one.
Comment 11 Jan de Mooij [:jandem] 2013-07-19 09:52:41 PDT
Created attachment 778528 [details] [diff] [review]
Part 4 - Add bailout tail stub

Moves the bailout-tail code into its own shared stub. Currently we generate only two copies of the bailout tail code per runtime, so it's not really a memory win, but it's necessary to avoid a cyclic dependency: the exception handler code will use the bailout code, the bailout code can invoke the exception handler.
Comment 12 Jan de Mooij [:jandem] 2013-07-22 05:10:27 PDT
Created attachment 779147 [details] [diff] [review]
Part 5 - Compile try-catch

The OSR problem I mentioned in comment 6 turned out to be easier to fix than I thought, so this patch properly supports OSR. It required some analysis code to avoid OSR'ing in the catch block though.

I also added a --ion-compile-try-catch shell flag. Once fuzzing etc is done I will remove this flag.
Comment 13 Hannes Verschore [:h4writer] 2013-07-22 05:18:28 PDT
Comment on attachment 778528 [details] [diff] [review]
Part 4 - Add bailout tail stub

Review of attachment 778528 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ion/x64/Trampoline-x64.cpp
@@ +738,5 @@
> +
> +    masm.generateBailoutTail(rdx, r9);
> +
> +    Linker linker(masm);
> +    return linker.newCode(cx, JSC::OTHER_CODE);

I'm not really sure what the CodeKind is used for? I only see it being added to a counter that is never read...
Would it make sense to make JSC::ION_CODE for this? I could foresee situations where did is correct and others that would be wrong to mark this ION_CODE. But since it isn't used I can't really judge this.

::: js/src/ion/x86/Trampoline-x86.cpp
@@ +458,5 @@
>          uint32_t frameSize = FrameSizeClass::FromClass(frameClass).frameSize();
>          masm.addl(Imm32(BailoutDataSize + sizeof(void *) + frameSize), esp);
>      }
>  
> +    // Jump to shared bailout tail. The BailoutInfo pointer has to be in ecx.

Why the change from ebx to ecx?
Comment 14 Jan de Mooij [:jandem] 2013-07-22 06:14:13 PDT
(In reply to Hannes Verschore [:h4writer] from comment #13)
> I'm not really sure what the CodeKind is used for? I only see it being added
> to a counter that is never read...

See ExecutableAllocator::sizeOfCode, it uses the counters for about:memory.

> Would it make sense to make JSC::ION_CODE for this? I could foresee
> situations where did is correct and others that would be wrong to mark this
> ION_CODE. But since it isn't used I can't really judge this.

It doesn't really matter I think since it's only a small amount of generated code. The other shared stubs also use OTHER_CODE though so I think that's more consistent.

> Why the change from ebx to ecx?

The bailout tail stub expects the BailoutInfo pointer in ecx. There are two places where we jump to the bailout tail stub: one of them already stores the pointer in ecx, the other had it in ebx so I had to change it to use ecx as well.
Comment 15 Hannes Verschore [:h4writer] 2013-07-22 06:20:49 PDT
(In reply to Jan de Mooij [:jandem] from comment #14)
> It doesn't really matter I think since it's only a small amount of generated
> code. The other shared stubs also use OTHER_CODE though so I think that's
> more consistent.
Ok

> 
> > Why the change from ebx to ecx?
> 
> The bailout tail stub expects the BailoutInfo pointer in ecx. There are two
> places where we jump to the bailout tail stub: one of them already stores
> the pointer in ecx, the other had it in ebx so I had to change it to use ecx
> as well.
Oh I see it now. Right.
Comment 17 Till Schneidereit [:till] 2013-07-23 03:22:37 PDT
*** Bug 543875 has been marked as a duplicate of this bug. ***
Comment 19 Kannan Vijayan [:djvj] 2013-07-30 09:35:14 PDT
Comment on attachment 779147 [details] [diff] [review]
Part 5 - Compile try-catch

Review of attachment 779147 [details] [diff] [review]:
-----------------------------------------------------------------

Nice to have this in, finally (no pun intended ;).

::: js/src/ion/BaselineBailouts.cpp
@@ +1358,5 @@
>        default:
>          MOZ_ASSUME_UNREACHABLE("Unknown bailout kind!");
>      }
>  
> +    if (isExceptionBailout) {

The code within this conditional can be moved into |CheckFrequentBailouts|, which can check both the regular bailout frequency and the exception bailout frequency, every time it's called.

That seems like the natural place to put this logic instead of inlining it.

::: js/src/ion/BytecodeAnalysis.cpp
@@ +45,5 @@
>      // Clear all BytecodeInfo.
>      mozilla::PodZero(infos_.begin(), infos_.length());
>      infos_[0].init(/*stackDepth=*/0);
>  
> +    Vector<CatchFinallyRange, 0, IonAllocPolicy> catchFinallyRanges;

Is a vector of these really necessary?  We're using this to record only the fact that a loop is inside a catch or finally block... but for that purpose only the outermost catch or finally block should matter.

@@ +137,5 @@
> +
> +          case JSOP_LOOPENTRY:
> +            for (size_t i = 0; i < catchFinallyRanges.length(); i++) {
> +                if (catchFinallyRanges[i].contains(offset))
> +                    infos_[offset].loopEntryInCatchOrFinally = true;

Since try/catch/finally regions can't straddle each other, and must be perfectly nested, I don't think this loop is necessary.  The first time we enter a catch/finally block, we record the offset of the end of that block, initialize the CatchFinallyRange, and mark it active.  When we reach that end pc, we mark the CatchFinallyRange inactive.

This loop can probably become a single check of
  if (isCatchFinallyRangeActive && catchFinallyRange.contains(...)) { ... }

::: js/src/ion/IonBuilder.cpp
@@ +3225,5 @@
> +        return abort("Try-catch support disabled");
> +
> +    // Try-finally is not yet supported.
> +    if (script()->analysis()->hasTryFinally())
> +        return abort("Finally block");

Can't both of these checks be moved to pre-compilation?  The analysis should be available at the start of compilation, so we don't have to start compiling and then abort halfway through when we find out we're hitting a try or a try/finally.

Rethinking this.. I suppose having the check done during compilation would allow us to still compile in situations where try/catch/finally shows up in the source, but is otherwise unreachable.  Was that the reason behind doing this check during compilation instead of before?

@@ +3259,5 @@
> +    // and later passes will optimize this MTest to a no-op.
> +    //
> +    // If the code after the try block is unreachable (control flow in both the
> +    // try and catch blocks is terminated), only create the try block, to avoid
> +    // parsing unreachable code.

Using MTest here for its implicit handling later on during compilation seems like it might be error prone.

Would it be reasoanble to have a custom MTryBranch control-flow instruction that performs this?
Comment 20 Jan de Mooij [:jandem] 2013-08-08 08:44:32 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/383ccc9eb488

Preffed off for now, will file a separate bug to enable it after fuzzing etc.

(In reply to Kannan Vijayan [:djvj] from comment #19)
> The code within this conditional can be moved into |CheckFrequentBailouts|,
> which can check both the regular bailout frequency and the exception bailout
> frequency, every time it's called.

Good suggestion, done.

> Is a vector of these really necessary?  We're using this to record only the
> fact that a loop is inside a catch or finally block... but for that purpose
> only the outermost catch or finally block should matter.

> Since try/catch/finally regions can't straddle each other, and must be
> perfectly nested, I don't think this loop is necessary.  The first time we
> enter a catch/finally block, we record the offset of the end of that block,
> initialize the CatchFinallyRange, and mark it active.  When we reach that
> end pc, we mark the CatchFinallyRange inactive.
> 
> This loop can probably become a single check of
>   if (isCatchFinallyRangeActive && catchFinallyRange.contains(...)) { ... }

The problem is that we push a CatchFinallyRange when we enter the *try* block, not when we enter its catch/finally block. The try block can contain loops which can OSR, but not if they are inside a nested catch block... It's a bit unfortunate but it seemed the easiest solution (this is one of the places where baseline compilation from the AST instead of bytecode would be a lot simpler...)

> Can't both of these checks be moved to pre-compilation?  The analysis should
> be available at the start of compilation, so we don't have to start
> compiling and then abort halfway through when we find out we're hitting a
> try or a try/finally.

The check for the pref is hard to move somewhere else without adding a hasTryBlocks analysis bit, and hopefully the pref will be removed soon anyway. I did move the check for try-finally blocks into Compile in Ion.cpp, but I now realize that should probably have an abort message... Will fix that.

> Using MTest here for its implicit handling later on during compilation seems
> like it might be error prone.

Why would it be error prone?
Comment 21 Kannan Vijayan [:djvj] 2013-08-08 09:35:01 PDT
(In reply to Jan de Mooij [:jandem] from comment #20)
> Why would it be error prone?

MTest doesn't intinsically suggest that the intent is to keep both of its successor blocks alive.

If someone comes along and implements some DCE-like optimization that prunes MTests with constants and turns them into MGoto, dropping the second block.. it would not be obvious to them that doing that would make try/catch optimization incorrect.
Comment 22 Jan de Mooij [:jandem] 2013-08-08 10:05:25 PDT
(In reply to Kannan Vijayan [:djvj] from comment #21)
> If someone comes along and implements some DCE-like optimization that prunes
> MTests with constants and turns them into MGoto, dropping the second block..
> it would not be obvious to them that doing that would make try/catch
> optimization incorrect.

The unreachable code elimination pass already does that..

Usually the successor block will also be reachable from the end of the try block. If the try block always returns or throws, and there's no OSR, it's fine to eliminate the code following the try-catch (it must be unreachable). If we're doing OSR, the patch in bug 880377 will prevent this from being a problem (apparently bad type information can also lead to bogus conditions).
Comment 23 Ryan VanderMeulen [:RyanVM] 2013-08-08 13:28:29 PDT
https://hg.mozilla.org/mozilla-central/rev/383ccc9eb488
Comment 24 Carsten Book [:Tomcat] 2013-08-12 03:53:20 PDT
https://hg.mozilla.org/mozilla-central/rev/73912c9ba403

Note You need to log in before you can comment on or make changes to this bug.