1870279 - 30 - 6.86% perf_reftest_singletons getElementById-1.html / perf_reftest_singletons attr-selector-1.html + 3 more (Linux) regression on Fri December 8 2023

Status: NEW

(Core :: Performance, defect, P3)

Description

[Andrej (:aglavic)]

Perfherder has detected a talos performance regression from push 5e7ca7b9c60d764540bfc20b243ffaa67dfbfaa9. 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)
30%	perf_reftest_singletons getElementById-1.html	linux1804-64-shippable-qr	e10s fission stylo webrender	61.86 -> 80.42
9%	perf_reftest_singletons nth-index-2.html	linux1804-64-shippable-qr	e10s fission stylo webrender	1.27 -> 1.38
9%	perf_reftest_singletons window-named-property-get.html	linux1804-64-shippable-qr	e10s fission stylo webrender	620.25 -> 678.50
8%	perf_reftest_singletons nth-index-1.html	linux1804-64-shippable-qr	e10s fission stylo webrender	1.27 -> 1.38
7%	perf_reftest_singletons attr-selector-1.html	linux1804-64-shippable-qr	e10s fission stylo webrender	155.12 -> 165.76

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.

For more information on performance sheriffing please see our FAQ.

Comment 1

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

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

Comment 2

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

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

Comment 3

[Donal Meehan [:dmeehan]]

:fdoty could this be triaged? Tracking this due to the 30% change, but not sure about the impact.
Fx122 is now in beta, wondering if this can be investigated/fixed in the for release

Comment 4

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

The bug is marked as tracked for firefox122 (beta). However, the bug still isn't assigned.

:fdoty, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 5

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

This is a reminder regarding comment #4!

The bug is marked as tracked for firefox122 (beta). We have limited time to fix this, the soft freeze is in 14 days. However, the bug still isn't assigned.

Comment 6

[Frank Doty [:fdoty]]

mleclair will be reviewing

Comment 7

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

This is a reminder regarding comment #4!

The bug is marked as tracked for firefox122 (beta). We have limited time to fix this, the soft freeze is in 8 days. However, the bug still isn't assigned.

Comment 8

[Frank Doty [:fdoty]]

This regression is a WONTFIX - the improvements to the overall speedometer 3 score outweighs this regression. We'll continue to review changes to ensure the good continues to outweigh the bad.

Comment 9

[Donal Meehan [:dmeehan]]

Setting 122 to wontfix, feel free to resolve the bug as wontfix when ready.

Comment 10

[Jeff Muizelaar [:jrmuizel]]

I tried reproducing the slowdown locally and I don't see anywhere near the same performance difference getElementById-1.html between the two builds.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

I was using the wrong before build. I do see a difference in the profile when using a correct before build. The fast build has an indirect call in the hot path and the slow build does not.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Specifically: https://share.firefox.dev/3O9KRLp shows that in the fast build we're inlining nsTHashtable<mozilla::IdentifierMapEntry>::s_MatchEntry into PLDHashTable::SearchTable. In the slow build we do not, we call it indirectly.

Comment 13

[Markus Stange [:mstange]]

Bug 1843951 has a patch to turn that indirect call into a direct call always, making it always inlineable (and not just via PGO speculation).

Here's a try build with that patch applied: https://treeherder.mozilla.org/jobs?repo=try&revision=18188722c944afc3be4ce7d9fb84040fcb84e94a&selectedTaskRun=QW9m0XiwSnCo68KLmVZQXw.0

It gets a score of 40.66ms on getElementById-1.html, which is quite a bit faster than both the scores in this perf alert (61.86 -> 80.42).

Comment 14

[Bob Hood [:bhood]]

Frank, can we please get Priority/Severity settings applied here?

Comment 15

[Bas Schouten (:bas.schouten)]

None for now as far as impact as this is not a real website. Might be worth landing mstange's patch though.

Comment 16

[Frank Doty [:fdoty]]

(In reply to Markus Stange [:mstange] from comment #13)

Bug 1843951 has a patch to turn that indirect call into a direct call always, making it always inlineable (and not just via PGO speculation).

Here's a try build with that patch applied: https://treeherder.mozilla.org/jobs?repo=try&revision=18188722c944afc3be4ce7d9fb84040fcb84e94a&selectedTaskRun=QW9m0XiwSnCo68KLmVZQXw.0

It gets a score of 40.66ms on getElementById-1.html, which is quite a bit faster than both the scores in this perf alert (61.86 -> 80.42).

Does this deserve a second pass to determine if this patch should be landed?

Comment 17

[Markus Stange [:mstange]]

It might. The patch isn't ready yet though - it has some gtest failures.

An alternative approach to consider would be to switch DocumentOrShadowRoot::mIdentifierMap to use mozilla::HashSet instead of nsTHashTable.
We have a gtest benchmark which reports lower times for HashSet than for nsTHashTable.