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

Status: RESOLVED WONTFIX

(Firefox Build System :: Toolchains, defect)

Description

[Alexandru Ionescu (needinfo me) [:alexandrui]]

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.

Comment 1

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

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

Comment 2

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== 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

Comment 3

[Mike Hommey [:glandium]]

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

Comment 4

[Mike Hommey [:glandium]]

Same question for the bloom-basic regressions on osx.

Comment 5

[Mike Hommey [:glandium]]

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

Comment 6

[Alexandru Ionescu (needinfo me) [:alexandrui]]

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

Comment 7

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(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.

Comment 8

[Alexandru Ionescu (needinfo me) [:alexandrui]]

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

Comment 9

[Mike Hommey [:glandium]]

(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.

Comment 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Mike Hommey [:glandium]]

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).

Comment 12

[Mike Hommey [:glandium]]

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.

Comment 13

[(Away)]

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.

Comment 14

[Mike Hommey [:glandium]]

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.

Comment 15

[Mike Hommey [:glandium]]

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.

Comment 17

[Aria Beingessner [:Gankra]]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Mike Hommey [:glandium]]

(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.

Comment 20

[(Away)]

(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?

Comment 21

[Mike Hommey [:glandium]]

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

Comment 22

[Arthur K. (he/him)]

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."

Comment 23

[Mike Hommey [:glandium]]

(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.

Comment 24

[Mike Hommey [:glandium]]

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

Comment 25

[Alexandru Ionescu (needinfo me) [:alexandrui]]

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

Comment 26

[Mike Hommey [:glandium]]

(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.

Comment 27

[Alexandru Ionescu (needinfo me) [:alexandrui]]

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

Comment 28

[Mike Hommey [:glandium]]

I'll close wontfix, then.