Closed Bug 1458567 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: jandem, Assigned: jandem)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch PatchSplinter Review
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.
Attachment #8972606 - Flags: review?(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.
Attachment #8972606 - Flags: review?(luke) → review+
I wouldn't be surprised if this ends up fixing some weird (browser/devtools) intermittents...
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 :)
Attached patch Fix test (obsolete) — Splinter Review
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.
Attachment #8972838 - Flags: review?(jhofmann)
(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?
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij) → needinfo?(jhofmann)
Attached patch Fix test, v2Splinter Review
Attachment #8972838 - Attachment is obsolete: true
Attachment #8972838 - Flags: review?(jhofmann)
Attachment #8972843 - Flags: review?(jhofmann)
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.
Attachment #8972843 - Flags: review?(jhofmann) → review+
Flags: needinfo?(jhofmann)
Pushed by jandemooij@gmail.com:
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
Pushed part 1 with some minor changes we discussed on IRC yesterday.
https://hg.mozilla.org/mozilla-central/rev/693e9f7a2b59
https://hg.mozilla.org/mozilla-central/rev/1eb83ac9acee
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.