Closed Bug 1672610 Opened 9 months ago Closed 12 days ago

0.4% installer size (osx-cross) regression on push c061dfcf1598aece0fe343783b351d0a280bf67d (Wed October 21 2020)

Categories

(Firefox Build System :: Toolchains, defect)

Firefox 84
defect

Tracking

(firefox-esr78 unaffected, firefox82 unaffected, firefox83 unaffected, firefox84 wontfix, firefox85 fix-optional)

RESOLVED WONTFIX
Tracking Status
firefox-esr78 --- unaffected
firefox82 --- unaffected
firefox83 --- unaffected
firefox84 --- wontfix
firefox85 --- fix-optional

People

(Reporter: alexandrui, Assigned: glandium)

References

(Regression)

Details

(Keywords: perf-alert, regression)

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

Regressions:

Ratio Suite Test Platform Options Absolute values (old vs new)
0.40% installer size osx-cross 79,940,228.54 -> 80,261,126.17

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 offending patch(es) will be backed out in accordance with our regression policy.

For more information on performance sheriffing please see our FAQ.

Flags: needinfo?(mh+mozilla)
Component: Performance → Toolchains
Product: Testing → Firefox Build System

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

== Change summary for alert #27295 (as of Thu, 22 Oct 2020 04:03:39 GMT) ==

Regressions:

Ratio Suite Test Platform Options Absolute values (old vs new)
27% perf_reftest_singletons bloom-basic-2.html linux64-shippable e10s stylo 37.22 -> 47.45
27% perf_reftest_singletons bloom-basic.html linux64-shippable e10s stylo 37.36 -> 47.33
20% perf_reftest_singletons bloom-basic.html linux64-shippable e10s stylo 38.85 -> 46.62

Improvements:

Ratio Suite Test Platform Options Absolute values (old vs new)
13% perf_reftest_singletons tiny-traversal-singleton.html linux64-shippable e10s stylo 742.70 -> 647.55
8% perf_reftest_singletons coalesce-2.html linux64-shippable e10s stylo 135.48 -> 124.33
8% perf_reftest_singletons parent-basic-singleton.html linux64-shippable e10s stylo 87.90 -> 80.88
7% perf_reftest_singletons coalesce-1.html linux64-shippable e10s stylo 167.96 -> 156.59
6% perf_reftest_singletons abspos-reflow-1.html linux64-shippable e10s stylo 50.91 -> 47.98
5% perf_reftest_singletons nth-index-2.html linux64-shippable e10s stylo 3.02 -> 2.86
5% perf_reftest_singletons attr-selector-1.html linux64-shippable e10s stylo 163.62 -> 154.72

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=27295

How come the installer-size regressions on other platforms are not in the same alert?

Flags: needinfo?(mh+mozilla) → needinfo?(aionescu)

Same question for the bloom-basic regressions on osx.

Interestingly (weirdly?) there doesn't seem to be a perf regression on Windows?

Assignee: nobody → mh+mozilla

Ricky/Mike, I assume the alert below is also caused by the culprit of this regression:

== Change summary for alert #27352 (as of Sun, 25 Oct 2020 16:58:40 GMT) ==

Improvements:

Ratio Suite Test Platform Options Absolute values (old vs new)
22% build times windows2012-64-shippable nightly taskcluster-c5d.4xlarge 2,336.52 -> 1,815.05
19% build times windows2012-32-shippable nightly taskcluster-c5.4xlarge 2,290.26 -> 1,848.05
18% build times windows2012-aarch64 aarch64-no-eme nightly taskcluster-c5.4xlarge 2,305.00 -> 1,880.99
17% build times windows2012-32-shippable nightly taskcluster-m5.4xlarge 2,349.43 -> 1,941.14
6% build times linux64 plain taskcluster-m5.4xlarge 1,249.37 -> 1,179.17

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=27352

(In reply to Mike Hommey [:glandium] from comment #3)

How come the installer-size regressions on other platforms are not in the same alert?

I can manually check the graphs for other platforms indeed triggered alerts. A possible explanation is the jobs for other platforms run less often and in consequence the alerts weren't yet triggered.

Flags: needinfo?(aionescu)

Seems like the regression is on other platforms but not all graphs triggered alerts.

Flags: needinfo?(rstewart)
Flags: needinfo?(mh+mozilla)

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

Ricky/Mike, I assume the alert below is also caused by the culprit of this regression:

You mean, improvement.

Flags: needinfo?(rstewart)
Flags: needinfo?(mh+mozilla)

So far:

  • the installer size regression doesn't seem to affect shippable builds and comes from https://github.com/rust-lang/rust/pull/72227. I filed https://github.com/rust-lang/rust/issues/78471 to track that upstream, but as this doesn't affect shippable builds, I'd tend to ignore it (although it could impact downstreams, but they also don't depend on the version of rustc we use and are probably already using an affected version, since this is a regression in 1.45).
  • the bloom-basic regression has been identified to come from the upgrade of rustc to LLVM10 (which also means it hasn't been balanced by the upgrade to LLVM11). I have yet to identify what's going on.

An interesting data point about the bloom-basic regression: I can't reproduce the regression locally on my Threadripper, but I can reproduce on a i7-7500U (and the regression is even larger than on automation).

David, if you have some cycles to spare while it's night in my part of the world, can you take a look into the bloom-basic regression from LLVM10 (not 11, see comment 10)? It does affect Windows builds too, shippable builds only. https://treeherder.mozilla.org/perf.html#/graphs?highlightAlerts=1&series=autoland,1924883,1,1&timerange=1209600
The issue can be reproduced by opening testing/talos/talos/tests/perf-reftest-singletons/bloom-basic.html from a local tree via a file:/// url.

Flags: needinfo?(dmajor)

I'm seeing that get_matching_rules got a lot slower, particularly in checking the end-condition of the for loop.

Rust 1.43: https://paste.mozilla.org/rcaKRa7G

Ctrl+F for xul!smallvec::SmallVec, following that instruction is the loop exit test:

  265 00000001`84613a81 4883c320        add     rbx,20h
  265 00000001`84613a85 4883c7e0        add     rdi,0FFFFFFFFFFFFFFE0h
  265 00000001`84613a89 0f8511feffff    jne     xul!style::selector_map::SelectorMap<style::stylist::Rule>::get_matching_rules<style::gecko::wrapper::GeckoElement,mut closure-0*>+0x40 (00000001`846138a0)

Rust 1.47: https://paste.mozilla.org/w7b8d6ge

  265 00000001`846105c0 498d4720        lea     rax,[r15+20h]
  265 00000001`846105c4 4d39f7          cmp     r15,r14
  265 00000001`846105c7 490f44c7        cmove   rax,r15
  265 00000001`846105cb 740c            je      xul!style::selector_map::SelectorMap<style::stylist::Rule>::get_matching_rules<style::gecko::wrapper::GeckoElement,mut closure-0*>+0x239 (00000001`846105d9)
  
  265 00000001`846105cd 4c89fb          mov     rbx,r15
  265 00000001`846105d0 4d85ff          test    r15,r15
  265 00000001`846105d3 0f8509feffff    jne     xul!style::selector_map::SelectorMap<style::stylist::Rule>::get_matching_rules<style::gecko::wrapper::GeckoElement,mut closure-0*>+0x42 (00000001`846103e2)

In WPA I'm seeing a negligible amount of samples on the exit condition in the 1.43 code, but a large number in the 1.47 code.

Flags: needinfo?(dmajor)

Things seem to start going off the rails for get_matching_rules at the first "Rotate Loops" pass. Interestingly, the loop exit test change seems not to occur when removing -C codegen-units=1, and building without the flag closes the regression gap for bloom-basic.html (although not completely), but not for bloom-basic-2.html (although it's slightly improved).

Also, worth noting the regression doesn't happen without PGO (cross-LTO only), and happens with PGO with an empty profile.

get_matching_rules is likely a red herring: I transplanted the get_matching_rules functions (all 3 of them) from a fast build into a slow build (have you tried manually doing the job of a linker? it's fun), and it didn't change a thing.

Duplicate of this bug: 1674963
Blocks: 1671545
Depends on: 1677593

I think it's worth looking into whether This patch to llvm could be rebased/landed to address this issue. It was written specifically to address a performance regression found during the original llvm 10 upgrade.

Is comment 19 something we can still look into for 85? Too late for 84 at this point...

Flags: needinfo?(mh+mozilla)

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18)

Is comment 19 something we can still look into for 85? Too late for 84 at this point...

I suppose you mean comment 17? See https://reviews.llvm.org/D72420#2419280, we can't test the patch as-is. Even if it was validated to work, we're not currently equipped to use a patched rustc more than to test things around on Linux builds, so I doubt we'll address this for 85, except if we find a workaround somehow.

Flags: needinfo?(mh+mozilla)

(In reply to Mike Hommey [:glandium] from comment #19)

See https://reviews.llvm.org/D72420#2419280, we can't test the patch as-is.

That comment mentions LLVM 10-based rustc, is it also a problem with newer rusts that are based on LLVM 11?

It is, cf. comment 10. But the patch doesn't apply cleanly on LLVM 11, so I went for the "easy" thing to try.

I know it might be premature to ask but has it been tested against Rust 1.49 and LLVM 11.0.1-RC2?

From: https://www.phoronix.com/scan.php?page=news_item&px=Rust-1.49-Released

"Rust 1.49 also promotes its 64-bit ARM macOS and 64-bit ARM Windows targets from Tier 3 to Tier 2. Under Tier 2, there are prebuilt binaries and the code is guaranteed to build but there is greater risk of bugs, etc. This is good news given the popularity of Apple Silicon with their recently launched Apple M1 devices."

(In reply to Arthur K. [He/Him/His] from comment #22)

I know it might be premature to ask but has it been tested against Rust 1.49 and LLVM 11.0.1-RC2?

This is all with rust 1.49. 1.50 will fix the size regression, but not the performance regression.

It seems we're not tracking bloom-basic anymore?

Flags: needinfo?(aionescu)

:glandium, this is a build_metrics alert. bloom_basic is talos and we are tracking it AFAIK.

Flags: needinfo?(aionescu)

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

:glandium, this is a build_metrics alert. bloom_basic is talos and we are tracking it AFAIK.

there's a bloom-basic alert in comment 2, and the corresponding graph stops in March, and I couldn't find other graphs in perfherder.

Flags: needinfo?(aionescu)

You're right, the corresponding bloom-basic alert stops in march.

Flags: needinfo?(aionescu)

I'll close wontfix, then.

Status: NEW → RESOLVED
Closed: 12 days ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.