The default bug view has changed. See this FAQ.

IonMonkey: Add interrupt checks

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jandem, Assigned: dvander)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Reporter)

Description

6 years ago
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?
(Assignee)

Comment 1

6 years ago
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)

Updated

5 years ago
Assignee: general → dvander
Blocks: 745386
No longer blocks: 650180
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 627064 [details] [diff] [review]
x86/x64 patch

ARM tomorrow.
Attachment #627064 - Flags: review?(jdemooij)
(Assignee)

Comment 3

5 years ago
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.
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+
(Reporter)

Comment 5

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 627230 [details] [diff] [review]
function entry

Patch ionStackLimit to interrupt function entry.
Attachment #627230 - Flags: review?(jdemooij)
(Reporter)

Comment 7

5 years ago
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+
(Assignee)

Comment 8

5 years ago
http://hg.mozilla.org/projects/ionmonkey/rev/4ce3983a43f4 with comments addressed
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Duplicate of this bug: 758090
You need to log in before you can comment on or make changes to this bug.