1458567 - Don't call the interrupt callback when interrupting for GC or Ion

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

When running jit-tests with --ion-eager and off-thread compilation, we sometimes get spurious Debugger test failures because (1) when we invoke the interrupt callback we call the Debugger onStep handler and (2) we interrupt to attach finished Ion compilations.

In general it seems silly to invoke the interrupt callback just because we wanted to GC or finish Ion compilations.

The attached patch adds an InterruptReason for this (replacing InterruptMode). Now JSContext::interruptFlags_ is a bitmask of interrupt reasons, so we only have to invoke the interrupt callback if this was actually requested.

The patch also removes JSContext::interruptRegExpJit_ - now we can just check if the InterruptReason::CallbackUrgent bit is set.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8972606 [details] [diff] [review]
Patch

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

Wow, such a great find; we should've done this years ago.

Comment 2

[Jan de Mooij [:jandem]]

I wouldn't be surprised if this ends up fixing some weird (browser/devtools) intermittents...

Comment 3

[Jan de Mooij [:jandem]]

Pretty cool, we have a test that checks "dom.max_chrome_script_run_time" is accessed between 20 and 55 times during startup, and with this patch it's only 4 times :)

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Fix test

Johann, we used to call Gecko's interrupt callback also for JS-internal interrupts, so we ended up querying the "dom.max_chrome_script_run_time" pref quite a lot.

I'm fixing that in this bug, so now I got only 4 lookups instead of 20-55, so I changed the minimum to 3 instead of 20.

Comment 5

[Johann Hofmann [:johannh]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> Created attachment 8972838 [details] [diff] [review]
> Fix test
> 
> Johann, we used to call Gecko's interrupt callback also for JS-internal
> interrupts, so we ended up querying the "dom.max_chrome_script_run_time"
> pref quite a lot.
> 
> I'm fixing that in this bug, so now I got only 4 lookups instead of 20-55,
> so I changed the minimum to 3 instead of 20.

Amazing, thank you. I'm fine with this patch, but it sounds like you can entirely remove this section (the limit for failing the test for startup is 40 calls, the minimum of 20 is just to declare that it can be below that occasionally).

Is there any reason why you're not removing the entry?

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Johann Hofmann [:johannh] from comment #5)
> Is there any reason why you're not removing the entry?

No, I was just trying to make the most minimal change as I'm not familiar with the test. You mean like this?

-    "dom.max_chrome_script_run_time": {
-      min: 20,
-      max: 55,
-    },

I can do that.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Fix test, v2

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8972843 [details] [diff] [review]
Fix test, v2

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

Yup, that looks perfect, thank you. As long as that pref doesn't go over 40 accesses during startup we're good without a whitelist entry.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/693e9f7a2b59
part 1 - Don't invoke interrupt callback and Debugger onStep hook for internal JS engine interrupts. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/1eb83ac9acee
part 2 - Remove now unnecessary dom.max_chrome_script_run_time entry from browser_preferences_usage.js test. r=johannh

Comment 10

[Jan de Mooij [:jandem]]

Pushed part 1 with some minor changes we discussed on IRC yesterday.

Comment 11

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/693e9f7a2b59
https://hg.mozilla.org/mozilla-central/rev/1eb83ac9acee