1906078 - Test case takes two minutes to load

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Gregory Pappas [:gregp]]

Attachment: 1000x1000.html

Steps to reproduce:

1. Navigate to the attached test case

Actual results:
Takes about 2 minutes to load

Expected results:
Takes about 4 seconds to load, like in older Firefox builds and Chromium

2023-06-07 profile: https://share.firefox.dev/4cNlOYm
2024-07-02 profile: https://share.firefox.dev/466RVAt

Got this regression range
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2c8e99462d7dcbac2df0948cb9ab65d40d1d2fb7&tochange=256876c3862b6c4c8c51676f10d5ca440f86c0a8

Comment 1

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

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

:emilio, since you are the author of the regressor, bug 1835923, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Hmm, fun, it seems split-off can have poor performance in some cases / basically copying the whole list, which is what I was trying to avoid there.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Might be worth an upstream rust issue fwiw...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

We can fix this by splitting off from the back instead.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1906078 - Avoid pathological VecDeque behavior on parallel traversal. r=#style

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9596795522df
Avoid pathological VecDeque behavior on parallel traversal. r=dshin

Comment 7

[tszentpeteri]

https://hg.mozilla.org/mozilla-central/rev/9596795522df

Comment 8

[Donal Meehan [:dmeehan]]

:emilio do you want to add a beta and esr128 uplift request on this?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9411159 [details]
Bug 1906078 - Avoid pathological VecDeque behavior on parallel traversal. r=#style

Beta/Release Uplift Approval Request

• User impact if declined: Simple perf improvement, specially in pathological cases with lots of siblings.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Load test-case.
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Trivial-ish change to very hot / well-covered code-path.
• String changes made/needed: none
• Is Android affected?: Yes

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: See above
• User impact if declined: See above
• Fix Landed on Version: 130
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): See above.

Comment 10

[Donal Meehan [:dmeehan]]

Comment on attachment 9411159 [details]
Bug 1906078 - Avoid pathological VecDeque behavior on parallel traversal. r=#style

Approved for 129.0b2

Comment 11

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/302d75c4ebf2

Comment 12

[Daniel Bodea [:danibodea]]

The said test case takes a long time to load/render in Nightly v128.0a1 (2024-06-07), Nightly v128.0a1 (2024-07-07) and Beta v129.0b1 and it is almost instantly loaded in Nightly v130.0a1 (2024-07-09). Beta v129.0b2 is not yet available; it will be verified in the coming days. This was verified on Windows 10, MacOS 11 and Ubuntu 22.

It is important to mention that the general browser performance is diminished when this test case is displayed in both affected and fixed builds, but I imagine this is an expected side-effect of the test case.

Comment 13

[Daniel Bodea [:danibodea]]

This fix has also been verified in Beta v129.0b2 on Windows 10, MacOS 11 and Ubuntu 22. The loading time is significantly reduced.

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9411159 [details]
Bug 1906078 - Avoid pathological VecDeque behavior on parallel traversal. r=#style

Approved for 128.1esr.

Comment 15

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/2578d1f8740c

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Ok we didn't get a perf alert for this, so let's try to land a test.

Comment 17

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1906078 - Add a perf-reftest-singleton for this bug. r=#perftest-reviewers,#style

Comment 18

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

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)

Comment 19

[Daniel Bodea [:danibodea]]

This fix has also been verified in ESR v128.1.0esr on Windows 10 and MacOS 11. The loading time is significantly reduced.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6659230d6f56
Add a perf-reftest-singleton for this bug. r=perftest-reviewers,firefox-style-system-reviewers,sparky,zrhoffman

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/6659230d6f56