1906350 - 96.23 - 5.92% perf_reftest_singletons id-getter-7.html / pdfpaint xfa_bug1722030_1.pdf + 21 more (Linux, OSX, Windows) regression on Wed June 26 2024

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Andrej (:aglavic)]

Perfherder has detected a talos performance regression from push bd37e7af3a662cccc22dec549ecccc09408cdcb9. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
96%	perf_reftest_singletons id-getter-7.html	windows10-64-shippable-qr	e10s fission stylo webrender	353.27 -> 693.20
96%	perf_reftest_singletons id-getter-3.html	windows10-64-shippable-qr	e10s fission stylo webrender	354.40 -> 693.87
96%	perf_reftest_singletons id-getter-4.html	windows10-64-shippable-qr	e10s fission stylo webrender	354.58 -> 693.48
96%	perf_reftest_singletons id-getter-5.html	windows10-64-shippable-qr	e10s fission stylo webrender	354.06 -> 692.46
95%	perf_reftest_singletons id-getter-6.html	windows10-64-shippable-qr	e10s fission stylo webrender	354.86 -> 693.33
67%	perf_reftest_singletons id-getter-6.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	451.51 -> 755.63
63%	perf_reftest_singletons id-getter-5.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	466.77 -> 758.50
62%	perf_reftest_singletons id-getter-3.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	466.71 -> 758.12
62%	perf_reftest_singletons id-getter-4.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	467.32 -> 758.93
62%	perf_reftest_singletons id-getter-7.html	macosx1015-64-shippable-qr	e10s fission stylo webrender	466.37 -> 755.72
...	...	...	...	...
37%	perf_reftest_singletons id-getter-5.html	linux1804-64-shippable-qr	e10s fission stylo webrender	496.13 -> 681.95
36%	perf_reftest_singletons id-getter-4.html	linux1804-64-shippable-qr	e10s fission stylo webrender	502.12 -> 681.66
30%	perf_reftest_singletons id-getter-2.html	windows10-64-shippable-qr	e10s fission stylo webrender	613.56 -> 794.83
24%	perf_reftest_singletons id-getter-2.html	windows10-64-shippable-qr	e10s fission stylo webrender	635.46 -> 790.22
6%	pdfpaint xfa_bug1722030_1.pdf	macosx1015-64-shippable-qr	e10s fission stylo webrender-sw	8,909.02 -> 9,436.33

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1014

For more information on performance sheriffing please see our FAQ.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1903037

Comment 3

[Donal Meehan [:dmeehan]]

:willyelm could this be triaged for severity? (adding a ni since you are the triage owner)
I was wondering about the impact since the pref change is a high percentage.

:jandem what's the approach for fixing this in beta for Fx129?
Does the patch in the depends on Bug 1906312 look like a large change to take into beta?

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Donal Meehan [:dmeehan] from comment #3)

:jandem what's the approach for fixing this in beta for Fx129?
Does the patch in the depends on Bug 1906312 look like a large change to take into beta?

Yeah, we should probably uplift that to beta.

Bug 1906312 won't fix this regression completely though. There's some refcounting overhead that happens now that's hard to optimize without making the code less robust. Previously the code had some optimizations for this that don't really fit in the new system. These are all micro-benchmarks specifically targeting this code path so unless we see this show up somewhere else we can probably accept that regression.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Bug 1906350 - Avoid refcounting overhead for string buffers passed from DOM code. r?sfink!

The RefPtr<StringBuffer> argument of the new JS APIs can have pretty significant performance
overhead due to the atomic refcount used for StringBuffer.

This patch adds separate APIs that only use RefPtr when we're going to use the buffer for
a new JS string. This is a more similar to the behavior with the old external-string code
and fixes most of the reported performance regression.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bda0e83ceba
Avoid refcounting overhead for string buffers passed from DOM code. r=sfink

Comment 7

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/8bda0e83ceba

Comment 8

[Jan de Mooij [:jandem]]

This is fixed now.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jandem, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox129 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 9412250 [details]
Bug 1906350 - Avoid refcounting overhead for string buffers passed from DOM code. r?sfink!

Beta/Release Uplift Approval Request

• User impact if declined: Small perf regression in some cases.
• 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: Low
• Why is the change risky/not risky? (and alternatives if risky): Code is similar to what we did before.
• String changes made/needed:
• Is Android affected?: Yes

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9412250 [details]
Bug 1906350 - Avoid refcounting overhead for string buffers passed from DOM code. r?sfink!

Approved for 129.0b6

Comment 13

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/4b8cb6e252cd

Comment 14

[Andrej (:aglavic)]

(In reply to Pulsebot from comment #6)

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8bda0e83ceba
Avoid refcounting overhead for string buffers passed from DOM code. r=sfink

Perfherder has detected a talos performance change from push 8bda0e83ceba4450938b5cdcce406bb49bb8c4ac.

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
53%	perf_reftest_singletons id-getter-6.html	windows11-64-shippable-qr	e10s fission stylo webrender	319.11 -> 150.95
52%	perf_reftest_singletons id-getter-4.html	windows11-64-shippable-qr	e10s fission stylo webrender	316.22 -> 150.94
52%	perf_reftest_singletons id-getter-7.html	windows11-64-shippable-qr	e10s fission stylo webrender	315.34 -> 150.94
52%	perf_reftest_singletons id-getter-3.html	windows11-64-shippable-qr	e10s fission stylo webrender	314.45 -> 150.81
52%	perf_reftest_singletons id-getter-5.html	windows11-64-shippable-qr	e10s fission stylo webrender	314.35 -> 150.81
...	...	...	...	...
3%	pdfpaint tensor-allflags-withfunction.pdf	windows10-64-shippable-qr	e10s fission stylo webrender	195.64 -> 190.62

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

You can run these tests on try with ./mach try perf --alert 1355

For more information on performance sheriffing please see our FAQ.

TY for fixing the regression!