Closed Bug 1819722 Opened 1 year ago Closed 1 year ago

Implement monomorphic inlining

Categories

(Core :: JavaScript Engine, enhancement, P1)

Product:
Core
Component:
JavaScript Engine
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
113 Branch
Tracking Flags:
Tracking Status
firefox113 --- fixed

People

(Reporter: alexical, Assigned: alexical)

References

(Blocks 2 open bugs)

Details

(Keywords: perf-alert, Whiteboard: [sp3])

Attachments

(2 files)

Bug 1819722 - Monomorphic inlining r?iain
48 bytes, text/x-phabricator-request
Details | Review
Bug 1819722 - Fix monomorphic inlining comment r?iain
48 bytes, text/x-phabricator-request
Details | Review

This came about from conversations with Iain, and it might need a better name. Basically, the idea is we don't need to go through trial inlining if we observe that a function is monomorphic, because we're not going to be able to specialize it any further on a per-caller basis. So, we can just skip the trial inlining phase and use the function's own ICScript. This should save us a little on mprotects, and also if the function has lots of callers, this helps prevent it from staying in Baseline for a long time as the warmup count would be spread across all the callers.

Initial performance numbers are very encouraging: https://treeherder.mozilla.org/perfherder/comparesubtest?originalProject=try&newProject=try&newRevision=17569aaafe2cc7e9bd41bef9d2cd0367d6ee6297&originalSignature=3451139&newSignature=3451139&framework=13&originalRevision=c1f84d9c4c398160f5a0c46d079b967d472cd3c3&page=1&showOnlyImportant=1

As an added bonus this should fix bug 1818830 - but I'm keeping that around for now as a reminder to verify.

(In reply to Doug Thayer [:dthayer] (he/him) from comment #0)

This should save us a little on mprotects

Baseline IC code is shared and an extra ICScript for trial inlining doesn't require mprotects because we reuse the script's Baseline JIT code. Am I missing something?

Whiteboard: [sp3]
Severity: -- → N/A
Priority: -- → P2
Priority: P2 → P1
Attachment #9320905 - Attachment description: WIP: Bug 1819722 - Monomorphic inlining → Bug 1819722 - Monomorphic inlining r?iain

https://hg.mozilla.org/mozilla-central/rev/1c6df7a4bef8

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox113: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
Backout by sstanca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/45fc9bb4de9c
Backed out changeset 1c6df7a4bef8 for likely causing jit crashes.

crash signatures which were likely regressions from this change

Status: RESOLVED → REOPENED
status-firefox113: fixed → ---
Flags: needinfo?(dothayer)
Resolution: FIXED → ---
Target Milestone: 113 Branch → ---
Regressions: 1825217
Regressions: 1825220

(In reply to Pulsebot from comment #3)

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c6df7a4bef8
Monomorphic inlining r=iain

== Change summary for alert #37889 (as of Thu, 30 Mar 2023 07:23:38 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new) Performance Profiles
4% google-docs ContentfulSpeedIndex windows10-64-shippable-qr fission warm webrender 950.17 -> 912.42 Before/After
4% google-docs ContentfulSpeedIndex (geomean) linux1804-64-shippable-qr fission warm webrender 1,293.16 -> 1,243.63 Before/After
3% google-docs PerceptualSpeedIndex windows10-64-shippable-qr fission warm webrender 957.14 -> 925.08 Before/After

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37889

(In reply to Pulsebot from comment #5)

Backout by sstanca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/45fc9bb4de9c
Backed out changeset 1c6df7a4bef8 for likely causing jit crashes.

== Change summary for alert #37901 (as of Thu, 30 Mar 2023 13:18:19 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new) Performance Profiles
13% google-docs LastVisualChange windows10-64-shippable-qr fission warm webrender 4,241.58 -> 4,799.83 Before/After
10% google-docs SpeedIndex windows10-64-shippable-qr fission warm webrender 1,157.58 -> 1,270.08 Before/After
7% google-docs ContentfulSpeedIndex windows10-64-shippable-qr fission warm webrender 905.00 -> 968.67 Before/After
7% google-docs SpeedIndex linux1804-64-shippable-qr cold fission webrender 1,695.42 -> 1,810.58 Before/After
7% google-docs PerceptualSpeedIndex windows10-64-shippable-qr fission warm webrender 916.67 -> 978.67 Before/After
7% google-docs ContentfulSpeedIndex linux1804-64-shippable-qr cold fission webrender 1,514.50 -> 1,615.17 Before/After
6% google-docs LastVisualChange linux1804-64-shippable-qr cold fission webrender 6,113.33 -> 6,493.33 Before/After
6% yahoo-mail LastVisualChange linux1804-64-shippable-qr bytecode-cached fission warm webrender 641.67 -> 680.00 Before/After
5% google-docs PerceptualSpeedIndex linux1804-64-shippable-qr cold fission webrender 1,429.67 -> 1,504.08 Before/After
4% google-docs SpeedIndex macosx1015-64-shippable-qr cold fission webrender 1,520.81 -> 1,588.25 Before/After
4% google-docs SpeedIndex windows10-64-shippable-qr cold fission webrender 1,367.00 -> 1,427.25 Before/After
4% ares6 windows10-64-shippable-qr fission webrender 37.34 -> 38.83
3% linkedin SpeedIndex macosx1015-64-shippable-qr fission warm webrender 686.12 -> 705.08 Before/After
3% linkedin LastVisualChange windows10-64-shippable-qr fission warm webrender 2,052.75 -> 2,108.25
2% linkedin ContentfulSpeedIndex windows10-64-shippable-qr fission warm webrender 1,364.25 -> 1,393.08

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37901

(In reply to Pulsebot from comment #5)

Backout by sstanca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/45fc9bb4de9c
Backed out changeset 1c6df7a4bef8 for likely causing jit crashes.

== Change summary for alert #37976 (as of Mon, 03 Apr 2023 17:13:54 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
3% kraken windows10-64-shippable-qr e10s fission stylo webrender 1,149.99 -> 1,110.25
3% kraken macosx1015-64-shippable-qr e10s fission stylo webrender 1,365.11 -> 1,327.82
2% kraken windows10-64-shippable-qr e10s fission stylo webrender 1,142.58 -> 1,114.39

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37976

No longer regressions: 1825220

(In reply to Alexandru Ionescu (needinfo me) [:alexandrui] from comment #8)

(In reply to Pulsebot from comment #5)

Backout by sstanca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/45fc9bb4de9c
Backed out changeset 1c6df7a4bef8 for likely causing jit crashes.

== Change summary for alert #37901 (as of Thu, 30 Mar 2023 13:18:19 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new) Performance Profiles
13% google-docs LastVisualChange windows10-64-shippable-qr fission warm webrender 4,241.58 -> 4,799.83 Before/After
10% google-docs SpeedIndex windows10-64-shippable-qr fission warm webrender 1,157.58 -> 1,270.08 Before/After
7% google-docs ContentfulSpeedIndex windows10-64-shippable-qr fission warm webrender 905.00 -> 968.67 Before/After
7% google-docs SpeedIndex linux1804-64-shippable-qr cold fission webrender 1,695.42 -> 1,810.58 Before/After
7% google-docs PerceptualSpeedIndex windows10-64-shippable-qr fission warm webrender 916.67 -> 978.67 Before/After
7% google-docs ContentfulSpeedIndex linux1804-64-shippable-qr cold fission webrender 1,514.50 -> 1,615.17 Before/After
6% google-docs LastVisualChange linux1804-64-shippable-qr cold fission webrender 6,113.33 -> 6,493.33 Before/After
6% yahoo-mail LastVisualChange linux1804-64-shippable-qr bytecode-cached fission warm webrender 641.67 -> 680.00 Before/After
5% google-docs PerceptualSpeedIndex linux1804-64-shippable-qr cold fission webrender 1,429.67 -> 1,504.08 Before/After
4% google-docs SpeedIndex macosx1015-64-shippable-qr cold fission webrender 1,520.81 -> 1,588.25 Before/After
4% google-docs SpeedIndex windows10-64-shippable-qr cold fission webrender 1,367.00 -> 1,427.25 Before/After
4% ares6 windows10-64-shippable-qr fission webrender 37.34 -> 38.83
3% linkedin SpeedIndex macosx1015-64-shippable-qr fission warm webrender 686.12 -> 705.08 Before/After
3% linkedin LastVisualChange windows10-64-shippable-qr fission warm webrender 2,052.75 -> 2,108.25
2% linkedin ContentfulSpeedIndex windows10-64-shippable-qr fission warm webrender 1,364.25 -> 1,393.08

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37901

We seem to have lost more on the way out than we gained on the way in here. Feels like something else landed at the same time this did which regressed things, just less than this improved them. Is this tracked?

Flags: needinfo?(dothayer) → needinfo?(aionescu)
Depends on: 1826595

https://hg.mozilla.org/mozilla-central/rev/bd4da5d6dfe0

Status: REOPENED → RESOLVED
Closed: 1 year ago → 1 year ago
status-firefox113: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch
Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/345c8acf3c32
Fix monomorphic inlining comment r=iain DONTBUILD

(In reply to Andrej Glavic (:andrej) from comment #10)

(In reply to Pulsebot from comment #5)

Backout by sstanca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/45fc9bb4de9c
Backed out changeset 1c6df7a4bef8 for likely causing jit crashes.

== Change summary for alert #37976 (as of Mon, 03 Apr 2023 17:13:54 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
3% kraken windows10-64-shippable-qr e10s fission stylo webrender 1,149.99 -> 1,110.25
3% kraken macosx1015-64-shippable-qr e10s fission stylo webrender 1,365.11 -> 1,327.82
2% kraken windows10-64-shippable-qr e10s fission stylo webrender 1,142.58 -> 1,114.39

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=37976

Regarding the kraken differences, I investigated this and it's just a quirk of how this interacts with our inlining entry threshold heuristic. It feels like this is not representative of the real world, but I will in a separate bug take a look at various values for that threshold to see if we can move our numbers around in a beneficial way.

Regressions: 1826623

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

(In reply to Pulsebot from comment #12)

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd4da5d6dfe0
Monomorphic inlining r=iain

== Change summary for alert #38015 (as of Fri, 07 Apr 2023 01:29:07 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new)
3% kraken windows10-64-shippable-qr e10s fission stylo webrender 1,113.55 -> 1,146.79

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38015

(In reply to Pulsebot from comment #3)

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1c6df7a4bef8
Monomorphic inlining r=iain

== Change summary for alert #38020 (as of Fri, 07 Apr 2023 06:42:56 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
6% ares6-sm linux64-shippable 61.73 -> 58.17
5% ares6-sm linux64-shippable 60.02 -> 56.76

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38020

(In reply to Pulsebot from comment #5)

Backout by sstanca@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/45fc9bb4de9c
Backed out changeset 1c6df7a4bef8 for likely causing jit crashes.

== Change summary for alert #38022 (as of Fri, 07 Apr 2023 07:04:54 GMT) ==

Regressions:

Ratio Test Platform Options Absolute values (old vs new)
5% ares6-sm linux64-shippable 57.77 -> 60.93
4% ares6-sm linux64-shippable 57.96 -> 60.24

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38022

(In reply to Doug Thayer [:dthayer] (he/him) from comment #11)

(In reply to Alexandru Ionescu (needinfo me) [:alexandrui] from comment #8)
We seem to have lost more on the way out than we gained on the way in here. Feels like something else landed at the same time this did which regressed things, just less than this improved them. Is this tracked?

In updated the summary, thanks!

Flags: needinfo?(aionescu)

(In reply to Pulsebot from comment #12)

Pushed by dothayer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bd4da5d6dfe0
Monomorphic inlining r=iain

== Change summary for alert #38030 (as of Fri, 07 Apr 2023 13:32:12 GMT) ==

Improvements:

Ratio Test Platform Options Absolute values (old vs new)
5% ares6-sm linux64-shippable 60.02 -> 56.75
5% ares6-sm linux64-shippable 60.10 -> 57.31
4% sunspider-sm linux64-shippable 224.38 -> 216.17

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=38030

Depends on: 1828555
Blocks: 1812512
Regressions: 1841082
Regressions: CVE-2023-5171
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: