Last Comment Bug 700030 - IonMonkey: Add interrupt checks
: IonMonkey: Add interrupt checks
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: ---
Assigned To: David Anderson [:dvander]
: Jason Orendorff [:jorendorff]
: 758090 (view as bug list)
Depends on:
Blocks: LandIon
  Show dependency treegraph
Reported: 2011-11-05 06:35 PDT by Jan de Mooij [:jandem]
Modified: 2012-07-27 17:00 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

x86/x64 patch (6.94 KB, patch)
2012-05-24 18:40 PDT, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Splinter Review
ARM patch (4.19 KB, patch)
2012-05-24 19:59 PDT, David Anderson [:dvander]
marty.rosenberg: review+
Details | Diff | Splinter Review
function entry (3.57 KB, patch)
2012-05-25 07:52 PDT, David Anderson [:dvander]
jdemooij: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2011-11-05 06:35:43 PDT
Filing this bug to discuss possible ways to implement timeouts/interrupts. We need this for performance testing at some point, even though it may be hardly noticeable.

Probably the easiest way is to add MInterruptCheck/LInterruptCheck instructions, which can test some memory location and bail out if needed. This is what JM and TM do.

Another idea is combining the interrupt check and stack overflow check, like V8: reset the stack limit when an interrupt happens. I have read comments about using memory protection for guarding against stack overflow though, so I don't know if we want to do this?
Comment 1 User image David Anderson [:dvander] 2011-11-07 15:42:40 PST
Thanks for filing this - I don't think we have to worry about it just yet, as you said, the perf impact is usually very small, but it's necessary for browser support.

v8's stack check still has to read from a global address, so I think it ends up being the same as what we have in JM+TI.
Comment 2 User image David Anderson [:dvander] 2012-05-24 18:40:53 PDT
Created attachment 627064 [details] [diff] [review]
x86/x64 patch

ARM tomorrow.
Comment 3 User image David Anderson [:dvander] 2012-05-24 19:59:23 PDT
Created attachment 627074 [details] [diff] [review]
ARM patch

ARM patch tonight, actually.

I don't immediately understand v8's approach, but it seems like they do actually have separate tests (via one IR, StackCheck) for function entry versus loop edges. Both require loading from a static variable.
Comment 4 User image Marty Rosenberg [:mjrosenb] 2012-05-24 21:09:07 PDT
Comment on attachment 627074 [details] [diff] [review]
ARM patch

Review of attachment 627074 [details] [diff] [review]:

This is probably a good candidate for an in-register, per-compartment global (e.g. we sacrifice a register to hold this bit, along with some other bits, like the GC state)

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +1392,5 @@
> +{
> +    typedef bool (*pf)(JSContext *);
> +    static const VMFunction interruptCheckInfo = FunctionInfo<pf>(InterruptCheck);
> +
> +    OutOfLineCode *ool = oolCallVM(interruptCheckInfo, lir, (ArgList()), StoreNothing());

Looks like there are an extra set of parens around ArgList
Comment 5 User image Jan de Mooij [:jandem] 2012-05-25 02:07:12 PDT
Comment on attachment 627064 [details] [diff] [review]
x86/x64 patch

Review of attachment 627064 [details] [diff] [review]:

r=me with comment below addressed. Out of curiosity, how does this affect performance?

::: js/src/ion/IonBuilder.cpp
@@ +2314,5 @@
>  {
>      assertValidLoopHeadOp(pc);
>      insertRecompileCheck();
> +
> +    current->add(MInterruptCheck::New());

JM also inserts the check before function calls, in inlineCallHelper. I think we should do something similar to handle recursive calls etc. Can you measure how much these checks at every non-inlined call will slow us down? If it's significant, we can try something else like not checking when we're inside a loop or something.
Comment 6 User image David Anderson [:dvander] 2012-05-25 07:52:05 PDT
Created attachment 627230 [details] [diff] [review]
function entry

Patch ionStackLimit to interrupt function entry.
Comment 7 User image Jan de Mooij [:jandem] 2012-05-25 08:21:58 PDT
Comment on attachment 627230 [details] [diff] [review]
function entry

Review of attachment 627230 [details] [diff] [review]:

::: js/src/ion/VMFunctions.cpp
@@ +89,5 @@
>  {
> +    JS_CHECK_RECURSION(cx, return false);
> +
> +    if (cx->runtime->interrupt) {
> +        cx->runtime->ionStackLimit = cx->runtime->nativeStackLimit;

As discussed on IRC, I think we should try to do this where we clear the interrupt flag.
Comment 8 User image David Anderson [:dvander] 2012-05-25 14:07:45 PDT with comments addressed
Comment 9 User image David Anderson [:dvander] 2012-07-27 17:00:05 PDT
*** Bug 758090 has been marked as a duplicate of this bug. ***

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