1448887 - Consider removing async Ion interrupts

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

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.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

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(-)

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

Removes a bit more code.

Comment 3

[Jan de Mooij [:jandem]]

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

Ugh there's much more we can remove.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Patch

61 files changed, 79 insertions(+), 1315 deletions(-)

Comment 5

[Luke Wagner [:luke]]

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

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

Wow, that was a lot!

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Jan de Mooij [:jandem]]

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.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: 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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/81ef11104ebb
Remove async Ion loop interrupts. r=luke

Comment 10

[Jan de Mooij [:jandem]]

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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/81ef11104ebb