Closed Bug 700030 Opened 13 years ago Closed 12 years ago

IonMonkey: Add interrupt checks

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: dvander)

References

Details

Attachments

(3 files)

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?
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.
Assignee: general → dvander
Blocks: LandIon
No longer blocks: IonMonkey
Status: NEW → ASSIGNED
Attached patch x86/x64 patchSplinter Review
ARM tomorrow.
Attachment #627064 - Flags: review?(jdemooij)
Attached patch ARM patchSplinter Review
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.
Attachment #627074 - Flags: review?(mrosenberg)
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
Attachment #627074 - Flags: review?(mrosenberg) → review+
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.
Attachment #627064 - Flags: review?(jdemooij) → review+
Attached patch function entrySplinter Review
Patch ionStackLimit to interrupt function entry.
Attachment #627230 - Flags: review?(jdemooij)
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.
Attachment #627230 - Flags: review?(jdemooij) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/4ce3983a43f4 with comments addressed
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.