Consider removing the Ion helper thread pausing

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
We have this mechanism where we allow 1 helper thread at a time to do Ion compilation. If a higher priority task comes in, we can pause other compilation threads, process the new task on a new thread, then unpause the paused thread(s).

This adds quite a lot of complexity (to code that's already complicated enough!). It also has some issues:

* We would like to Ion-compile scripts in parallel, especially when the CPU has 4+ cores.

* Pausing compilation threads means these threads are not available for other tasks (GC, Wasm, parsing).

* IonBuilderHasHigherPriority includes the size and warm up count of the outer script, but due to inlining it's not clear whether that's very useful.

Removing the pausing mechanism improves shell benchmarks a bit locally. I'll check how this affects Speedometer on AWFY.
(Assignee)

Updated

2 years ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 1

2 years ago
Posted patch PatchSplinter Review
Mostly just removing code:

 5 files changed, 9 insertions(+), 185 deletions(-)

This improves shell benchmarks locally. I'm not sure about Speedometer (my AWFY-Try run failed for some reason). I think we should just land it and see what happens on AWFY.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8906512 - Flags: review?(luke)
Comment on attachment 8906512 [details] [diff] [review]
Patch

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

Great!
Attachment #8906512 - Flags: review?(luke) → review+

Comment 3

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab8c75e0d422
Remove Ion helper thread pausing mechanism. r=luke
(Assignee)

Comment 4

2 years ago
For posterity: some minor regressions on AWFY but overall this was an improvement. 7-9% on Kraken crypto-ccm, 2.2% on Octane mandreel, small win on Sunspider. Some of the improvements are bigger on 32-bit (> 11% on Kraken crypto-ccm). Not too bad for a code removal patch.

Will be interesting to see if this has any effect on the (slower) QF hardware or on Talos.

Comment 5

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