1974493 - 1.2% regression on Ares6-ML_steadyState on 24June2025

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Mayank Bansal]

Link: https://treeherder.mozilla.org/perfherder/graphs?highlightAlerts=1&highlightChangelogData=1&highlightCommonAlerts=0&replicates=0&selected=5258091,2059223950&series=autoland,5258091,1,13&timerange=5184000&zoom=1750546537428,1751119115564,66.17399492753727,71.25476340691198

Most probable regressor: Bug 1953595

I dont think this is a serious regression. Filing mostly for posterity. But JS folks may have other opinions.

Comment 1

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

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

For more information, please visit BugBot documentation.

Comment 2

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

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

Comment 3

[Nika Layzell [:nika] (ooo, ni? for response)]

This seems very unlikely to be the regressor, as the regression is on Windows, and the code changed in bug 1953595 is Android-only.

Comment 4

[Mayank Bansal]

After some retriggers, looks like the regressing changeset is https://hg-edge.mozilla.org/integration/autoland/pushloghtml?fromchange=d84b6c0995f02602907ec03ee051571ffb5ebf2c&tochange=29f18fab3ab5974f6426019d37a127816056c4e1
which points to bug 1973117

Comment 5

[Dianna Smith [:diannaS]]

:sdetar since iain is on pto, could you set a prioirty/severity for this?

Comment 6

[Iain Ireland [:iain]]

I don't have access to a Windows machine, but I spent some time investigating this on my Linux laptop. I used rr to identify each function that transpiled a StoreDenseElement with the flag set (there were 8). Then I compared the generated LIR for each of those functions using samply profiles. In all but one case, the LIR operations were the same: we generated a GuardElementsArePacked MIR node, but optimized it away because it was redundant with an earlier guard. (This happens whenever we have, for example, arr[idx] += ...: we can combine the GuardElementsArePacked from the pre-addition read with the post-addition store.) The only exception was maxRowIndex, which accounts for ~0.01% of the total samples.

Conversely, looking at the assembly, I saw several cases where the new version of StoreDenseElement generated better code: if we don't generate the GuardElementsArePacked node, we instead have to generate a magic value guard as part of the StoreElementT.

I tried generating a comparison report, but it reported that the biggest difference was in mmul, which is entirely unaffected by the change in bug 1973117.

The only changes I can actually see in this benchmark from this change look like they should be strict improvements (eg we generate strictly less code). If there's any real effect here, it's some sort of weird counterintuitive hardware nonsense (eg "generating more code happens to make the loop alignment better" or whatever).

I don't think digging into this any further is a good use of time.

Comment 7

[Iain Ireland [:iain]]

Oh, wait, I hadn't realized that the regression window also included bug 1971446. I was only investigating changes caused by the changes in bug 1973117. I'll take a look at the other changes tomorrow.

Comment 8

[Iain Ireland [:iain]]

Attachment: Bug 1974493: Remove GuardElementsArePacked when consumers are folded away r=jandem

If we create a GuardElementsArePacked to ensure that MLoadElement doesn't have to check for magic values, but then fold an unbox into the MLoadElement, then the unbox implicitly handles the magic check, and the guard is superfluous. This patch checks to make sure that all the consumers of the MElements are on an allow-list of ops that can handle non-packed arrays.

In testing on try, this does seem to very slightly improve the specific subtests (ares6-ML on Windows, Perf-Dashboard/SelectingPoints on OSX) that were regressed in bug 1974493 and bug 1975491. I don't completely trust platform-specific regressions, but if we can fix them I suppose we might as well.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bbd78a425343
https://hg.mozilla.org/integration/autoland/rev/3d6ae7e05567
Remove GuardElementsArePacked when consumers are folded away r=jandem

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3d6ae7e05567

Comment 12

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

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 13

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

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

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox143 to wontfix.

For more information, please visit BugBot documentation.