Closed Bug 1448887 Opened 3 years ago Closed 3 years ago

Consider removing async Ion interrupts


(Core :: JavaScript Engine: JIT, enhancement)

Not set



Tracking Status
firefox61 --- fixed


(Reporter: jandem, Assigned: jandem)




(2 files, 2 obsolete files)

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.
Depends on: 1449135
Blocks: 1446907
Attached 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
Attachment #8963543 - Flags: review?(luke)
Attached 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)
Comment on attachment 8963550 [details] [diff] [review]

Ugh there's much more we can remove.
Attachment #8963550 - Flags: review?(luke)
Attached 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]

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

Wow, that was a lot!
Attachment #8963560 - Flags: review?(luke) → review+
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.
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.
Attached 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.
Pushed by
Remove async Ion loop interrupts. r=luke
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.
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.