1382650 - Consider adding a 4th tier using Ion optimization levels

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Jan de Mooij [:jandem]]

Attachment: Hackish prototype

Scripts are currently Ion-compiled when their use count reaches ~1000. Ion does pretty aggressive inlining of scripts, this is necessary for really hot code (like the first 5 Octane tests). On workloads like Speedometer, this inlining is not helping as much and compilation time is an issue.

I'm attaching a hackish prototype to use the optimization level infrastructure Hannes added (I had to fix some regressions and bugs but it mostly worked). With this patch we have 2 Ion tiers: we still compile scripts with use count 1000 but we only inline 1-2 levels deep and don't inline large functions. We also include use count checks and when a script's use count reaches 100,000 we recompile and use the heuristics we have now.

It works surprisingly well. Octane is mostly unaffected or becomes a bit faster, Sunspider and Kraken regress by a few %, and Speedometer wins at least a few points on my machine (not sure how much because it's pretty noisy).

I'm not sure if this is worth pursuing now as this stuff is hard to get right and I think there's lower-hanging fruit still.

Comment 1

[Guilherme Lima]

Similar to bug 1080776?

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Guilherme Lima from comment #1)
> Similar to bug 1080776?

It's similar but a bit different: the approach there is to add a tier between Baseline and current Ion (at warmup count 100) whereas the patch here "delays" full-Ion. In general I don't think Ion-compiling with warmup count 100 (that bug) is a good idea.

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8888286 [details] [diff] [review]
Hackish prototype

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

::: js/src/jit/IonBuilder.cpp
@@ +5421,4 @@
>          MRecompileCheck* check =
>              MRecompileCheck::New(alloc(), target->nonLazyScript(),
>                                   optimizationInfo().inliningRecompileThreshold(),
> +                                 MRecompileCheck::RecompileCheckType::Inlining);

The problem is that the more we inline the more we would be likely to invalidate the compilation because of inlining issues.

I suggest we use a larger threshold factor than 4, or with an exponentiation by the inlining depth, such that we do not invalidate the code too frequently.

Comment 4

[Sean Stangl [:sstangl]]

When Apple added their LLVM-tier JIT, they found that the performance improvements from a 4th tier mostly came from better inlining information.

Although this design would be valuable by itself, it would be even better to have tier-4 Ion able to read type information from tier-3 Ion's ICs.

Comment 5

[Jan de Mooij [:jandem]]

I've been playing with this a bit and it's still a significant win on Speedometer (> 5%), Google Docs (5-10%) etc. SunSpider, Kraken, Octane are either unaffected or show an improvement.

This only tunes inlining options - we could probably get more speedups by making more IonBuilder code or expensive optimization passes run only at the highest optimization level.

Comment 6

[Sean Stangl [:sstangl]]

Just as a thought, I have been wondering whether it would be possible to use AFL-style branch tuples (http://lcamtuf.coredump.cx/afl/technical_details.txt) in the Baseline ICs to generate an implicit trace.

If we had something like that, then we would be able to make separate trace trees by type/shape, receiving the benefits of inlining even in polymorphic code.

I believe the answer is yes, but it's quite a lot of work.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 1 - Remove separate Ion warmup threshold for small functions, as it's equivalent to the normal one. r?nbp!

We used to have a different threshold for small functions but now they're both
set to 1000 so there's no need to special-case small functions.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 2 - Store Ion warmup threshold only in JitOptions. r?nbp!

Storing this also in IonOptimizationLevels.h/cpp is more complicated than
necessary.

Depends on D24153

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 3 - Clean up Ion eager compilation code. r?nbp!

There's a lot of complexity around setting/unsetting the eagerCompilation flag.
It's simpler to determine this based on the warm-up threshold being 0.

The patch also fixes some jit-tests where this patch would result in a change in
behavior.

Depends on D24154

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 4 - Split Ion warmup threshold JitOption in 'normal' and 'full' options. r?nbp!

Also adds a javascript.options.ion.full.threshold browser pref and similar shell
flags.

This doesn't rename the existing prefs yet.

Depends on D24155

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 5 - Calculate OSR warm-up threshold difference based on the warm-up threshold instead of hard-coding 100. r?nbp!

We want this to be more than 100 for the full-optimizations tier. Making this
relative also works better for tests that set a small Ion warm-up threshold.

Also disables OSR in some tests depending on the old behavior.

Depends on D24156

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 6 - Fix ARM64 implementation of branch32(AbsoluteAddress, Imm32). r?nbp!

The old code would assert because we needed too many scratch registers, but it
was dead code until this patch.

Depends on D24157

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 7 - Use a separate Ion optimization level for very hot code. r?nbp!

Ion can do aggressive inlining, but inlining a lot of code has a negative
effect on compilation time and memory usage. It also means we spend more time
in the slower Baseline code while compiling the Ion code off-thread or after an
invalidation.

To address this, Ion now consists of two tiers:

• Normal: the first tier (warm-up threshold of 1,000) only inlines small
  functions one level deep. This tier also has recompile checks to
  recompile the script when it becomes very hot.

• Full: the second tier (warm-up threshold of 100,000) is only used for very
  hot code so we can afford inlining a lot more code.

This improves Speedometer and GDocs by more than 5%.

Depends on D24158

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca8d272b176f
part 1 - Remove separate Ion warmup threshold for small functions, as it's equivalent to the normal one. r=nbp
https://hg.mozilla.org/integration/autoland/rev/30247ab61679
part 2 - Store Ion warmup threshold only in JitOptions. r=nbp
https://hg.mozilla.org/integration/autoland/rev/7ee65d07d83f
part 3 - Clean up Ion eager compilation code. r=nbp

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/ca8d272b176f
https://hg.mozilla.org/mozilla-central/rev/30247ab61679
https://hg.mozilla.org/mozilla-central/rev/7ee65d07d83f

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a812f6daf98e
part 4 - Split Ion warmup threshold JitOption in 'normal' and 'full' options. r=nbp
https://hg.mozilla.org/integration/autoland/rev/0614a9178eef
part 5 - Calculate OSR warm-up threshold difference based on the warm-up threshold instead of hard-coding 100. r=nbp
https://hg.mozilla.org/integration/autoland/rev/6960595971e5
part 6 - Fix ARM64 implementation of branch32(AbsoluteAddress, Imm32). r=nbp

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/a812f6daf98e
https://hg.mozilla.org/mozilla-central/rev/0614a9178eef
https://hg.mozilla.org/mozilla-central/rev/6960595971e5

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/391cc6344efe
part 7 - Use a separate Ion optimization level for very hot code. r=nbp

Comment 19

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/391cc6344efe

Comment 20

[Jan de Mooij [:jandem]]

Unfortunately bug 1536874 made changes to the tp6 tests right before this landed, so I don't know if perf alerts will work well.

That said, I did a Try push comparing Speedometer and tp6 with and without part 7 [0] on Linux64, OS X, Win64:

• Speedometer: 5% faster
• Instagram: 20% faster on Linux64, 13% on Win64. Not sure what's going on.
• GDocs: 7-8% faster
• GSlides: 6-7% faster
• Facebook: 4% faster
• YouTube: 4% faster

And some smaller improvements across the board. Improvements might be bigger on low-end devices.

[0] https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=699add20be66c755397e327ec2474d851aeae1a9&newProject=try&newRevision=8e46faa6645ec0f40fa0c4b04353e8bc8ea0e89c&framework=10

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Bug 1382650 part 8 - Add a JitOption to disable use of Ion optimization levels. r?nbp!

We might uplift these patches to beta. Having a JitOption makes it easier to
turn this off if needed.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3134740d831c
part 8 - Add a JitOption to disable use of Ion optimization levels. r=nbp

Comment 23

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3134740d831c

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 9052204 [details]
Bug 1382650 part 1 - Remove separate Ion warmup threshold for small functions, as it's equivalent to the normal one. r?nbp!

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: None
• User impact if declined: After some discussion we'd like to uplift these patches to FF67 to fix issues with Ion compilation slowing down page loads.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): There's some risk but this has been on Nightly for a week without any issues. We think the perf improvements are worth it.
• String changes made/needed: None

Comment 25

[Pascal Chevrel:pascalc]

Comment on attachment 9052204 [details]
Bug 1382650 part 1 - Remove separate Ion warmup threshold for small functions, as it's equivalent to the normal one. r?nbp!

These patches have been on Nightly for 10 days with no stability or functional regressions reported. This and the significant performance improvements on multiple major sites mean that I am taking the risk of taking these patches for 67 beta 9 as we are still in the middle of the beta cycle. If the uplifts to beta degrades our stability, we can back these patches out.

Comment 26

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7feb497c2ffe
https://hg.mozilla.org/releases/mozilla-beta/rev/02cdde2f8dbd
https://hg.mozilla.org/releases/mozilla-beta/rev/a0419e61efcd
https://hg.mozilla.org/releases/mozilla-beta/rev/9d3bcd768496
https://hg.mozilla.org/releases/mozilla-beta/rev/0e0e954b3cdc
https://hg.mozilla.org/releases/mozilla-beta/rev/e099b8601dc7
https://hg.mozilla.org/releases/mozilla-beta/rev/dfde2a76520d
https://hg.mozilla.org/releases/mozilla-beta/rev/353c9e1559f0

Comment 27

[Narcis Beleuzu [:NarcisB]]

Backed out for spidermonkey bustages on js.cpp

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/a330a98357513bce14a5764b8866aaf9843b9c3a
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=238661638&repo=mozilla-beta&lineNumber=2571

Comment 28

[Florin Strugariu [:Bebe]]

== Change summary for alert #20341 (as of Mon, 08 Apr 2019 08:41:03 GMT) ==

Improvements:

2% tp5o linux64-shippable opt e10s stylo 130.87 -> 127.77

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20341

Comment 29

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2f6ee802781a28f8e41c905da2d0f5d7ca3e1380
https://hg.mozilla.org/releases/mozilla-beta/rev/2e6bccd461f45dc46e328e2ad0254df2faa20736
https://hg.mozilla.org/releases/mozilla-beta/rev/7d87558da81f21659d5ba11df5564142f88e4703
https://hg.mozilla.org/releases/mozilla-beta/rev/d4a898d579ec1f10696774f3678ad97db02b0ae4
https://hg.mozilla.org/releases/mozilla-beta/rev/adcc63bfb4dc36fc8b2ff3571430ef18a78f7ff2
https://hg.mozilla.org/releases/mozilla-beta/rev/e896facf3e28c0389d3941d1947181afba0fd49d
https://hg.mozilla.org/releases/mozilla-beta/rev/2782ec026e7956b640b250e0cacb43c7f72e0a7a
https://hg.mozilla.org/releases/mozilla-beta/rev/b6fdd2540f52de0279a2bd14e7e52736530c987b

Comment 30

[Ionuț Goldan [:igoldan]]

Noticed some perf improvements! \0/

== Change summary for alert #20188 (as of Thu, 28 Mar 2019 21:31:19 GMT) ==

Improvements:

4% tp5o windows10-64-shippable-qr opt e10s stylo 122.10 -> 117.15

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20188

Comment 31

[Florin Strugariu [:Bebe]]

== Change summary for alert #20401 (as of Thu, 11 Apr 2019 05:40:11 GMT) ==

Improvements:

5% raptor-speedometer-firefox osx-10-10 opt 39.67 -> 41.84
2% raptor-speedometer-fennec android-hw-p2-8-0-arm7-api-16-nightly opt 23.98 -> 24.53

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=20401