Consider removing async Ion interrupts

RESOLVED FIXED in Firefox 61

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
We have some crashes in PatchJump under InterruptCheck/patchIonBackedges.

In bug 1435360 we removed async interrupts for wasm. If wasm can get away with it, it seems it should be okay for Ion too.

Furthermore, code patching isn't great (especially with W^X) and it's something I want to avoid as much as possible. In this case it also involves multi-threading and signal handlers; it makes it pretty difficult to reason about this.
(Assignee)

Updated

a year ago
Depends on: 1449135
(Assignee)

Updated

a year ago
Blocks: 1446907
(Assignee)

Comment 1

a year ago
Posted patch Patch (obsolete) — Splinter Review
This will regress micro-benchmarks in particular. Kraken and Octane-crypto will also regress a bit. I don't like this, but the code for this is just too complicated and we're seeing weird crashes when patching backedges.

 49 files changed, 60 insertions(+), 1177 deletions(-)
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8963543 - Flags: review?(luke)
(Assignee)

Comment 2

a year ago
Posted patch Patch (obsolete) — Splinter Review
Removes a bit more code.
Attachment #8963543 - Attachment is obsolete: true
Attachment #8963543 - Flags: review?(luke)
Attachment #8963550 - Flags: review?(luke)
(Assignee)

Comment 3

a year ago
Comment on attachment 8963550 [details] [diff] [review]
Patch

Ugh there's much more we can remove.
Attachment #8963550 - Flags: review?(luke)
(Assignee)

Comment 4

a year ago
Posted patch PatchSplinter Review
61 files changed, 79 insertions(+), 1315 deletions(-)
Attachment #8963550 - Attachment is obsolete: true
Attachment #8963560 - Flags: review?(luke)
Comment on attachment 8963560 [details] [diff] [review]
Patch

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

Wow, that was a lot!
Attachment #8963560 - Flags: review?(luke) → review+
(Assignee)

Comment 6

a year ago
Another problem with urgent interrupts is that they're not just for the (uncommon) slow script dialog case - we call JS_RequestInterruptCallback quite often for workers.

There are some pathological cases with console.log called from a worker (that will use a ConsoleRunnable which apparently does an urgent interrupt every time), but I added some logging and worker interrupts also show up on Google Maps etc.
(Assignee)

Comment 7

a year ago
Oh and I think for workers it's often the main thread that does the JS_RequestInterruptCallback and has to do the backedge patching (at least on Windows). Blocking the main thread like this is not great.
(Assignee)

Comment 8

a year ago
Posted file Worker testcase
Here's a (stupid) worker test case that improves from ~6800 ms ~5300 ms for me on OS X.

That suggests we currently spend >20% of the time in pthread_kill et al.

Comment 9

a year ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81ef11104ebb
Remove async Ion loop interrupts. r=luke
(Assignee)

Comment 10

a year ago
OK I just pushed this. Let's see how it affects AWFY and Talos - it will likely regress Kraken and other (micro-ish) benchmarks, but overall it should be fine hopefully.

A few thoughts:

(1) Safari/JSC doesn't have loop interrupt checks (|while(true) {}| just iloops). Luke and I were talking about this and maybe we can do something similar in content processes or use some heuristics (maybe we should wait for project-Fission).

(2) It might also be interesting to see if enabling our loop unrolling pass for very short loops helps Kraken.

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/81ef11104ebb
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.