Remove the ThinLTO CU hack

[cuviper]

This reverts #46722, commit e0ab5d5.

Since #111167, commit 10b69dd, we are
generating DWARF subprograms in a way that is meant to be more compatible
with LLVM's expectations, so hopefully we don't need this workaround
rewriting CUs anymore.

[rustbot]

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

[cuviper]

This was mentioned in #66118, but the original issue #46346 was specific to OSX, so let's get a try build there...

@bors try

[bors]

⌛ Trying commit 3227be0c051505d233febaf420c0c6116dacf372 with merge 67919e910983b07ebb5e26c5def1817a9079db4c...

[bors]

☀️ Try build successful - checks-actions
Build commit: 67919e910983b07ebb5e26c5def1817a9079db4c (67919e910983b07ebb5e26c5def1817a9079db4c)

[cuviper]

If any macOS person could try that build and look for the issues that were in #46346, that would be appreciated!

$ rustup-toolchain-install-master 67919e910983b07ebb5e26c5def1817a9079db4c

(https://crates.io/crates/rustup-toolchain-install-master)

[thomcc]

I've tried it out a bit in a number of configurations (probably not everything possible) and can't see any issues (certainly not the ones I see in the old nightlies that have the issue from #46346), so it's at least not obviously broken as far as I can tell.

(This stuff can be subtle tho, so hopefully history doesn't make me eat those words)

[nikic]

@bors r+

[bors]

📌 Commit c836c24 has been approved by nikic

It is now in the queue for this repository.

[bors]

⌛ Testing commit c836c24 with merge 77fb0cd...

[bors]

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 77fb0cd to master...

[rust-timer]

Finished benchmarking commit (77fb0cd): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[1.1%, 2.2%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[1.1%, 2.2%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-3.0%, -2.0%]	2
All ❌✅ (primary)	-	-	0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.1%	[2.1%, 2.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.1%	[2.1%, 2.1%]	1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	6.6%	[2.9%, 8.2%]	18
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	6.6%	[2.9%, 8.2%]	18

Bootstrap: 645.339s -> 642.167s (-0.49%)

[nnethercote]

@cuviper: there are some significant increases in binary size, was that expected? They are mostly in optimized incremental builds, which is a configuration that doesn't get much real-world use, but we also have some opt-full ones that are a concern:

Benchmark	Profile	Scenario	% Change	Significance Threshold ?	Significance Factor ?
webrender-2022	opt	full	7.60%	0.20%	37.51x
regex-1.5.5	opt	full	6.50%	0.41%	15.74x
ripgrep-13.0.0	opt	full	2.91%	0.22%	13.18x

[cuviper]

No, that's not expected -- this should only affect debuginfo, and even then I'm not sure that it would affect the size all that much, just no longer shuffling the CUs around. I'll see if I can reproduce that locally and see what changed.

[nnethercote]

The number of codegen units can have a sizeable effect on binary size -- fewer CGUs resulting in smaller binaries. Not sure if that's relevant.

[cuviper]

Sure, but I'm only talking about DWARF CUs here.

[michaelwoerister]

I could imagine that LLVM is less able to merge debuginfo with the hack removed, which results in more duplicated debuginfo?

[pnkfelix]

Do the opt-full size:linked-artifact benchmarks include debuginfo? Sounds counter-intuitive to me, if so.

I guess we'll see if @cuviper is able to reproduce the problem locally before we investigate more.

[cuviper]

@pnkfelix

Do the opt-full size:linked-artifact benchmarks include debuginfo? Sounds counter-intuitive to me, if so.

Normally they should not, apart from what debuginfo they get from the pre-compiled std. However, these 3 crates are notable for having [profile.release] debug overrides: debug = true in webrender and regex, and lesser debug = 1 in ripgrep (which also saw less change).

Taking the ripgrep example, I can see it go from 29.34MiB to 30.19MiB. The runtime code/data size changed slightly:

$ size -G target?/release/rg
      text       data        bss      total filename
   2549388    1628965        688    4179041 target1/release/rg
   2549612    1629101        688    4179401 target2/release/rg

But the debug sections show the main difference, especially pubnames, info, and line:

File: target1/release/rg
There are 43 section headers, starting at offset 0x1d58e40:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
[...]
  [32] .debug_aranges    PROGBITS        0000000000000000 3fe8c0 037f00 00      0   0  1
  [33] .debug_pubnames   PROGBITS        0000000000000000 4367c0 5355ba 00      0   0  1
  [34] .debug_info       PROGBITS        0000000000000000 96bd7a 5f2ef1 00      0   0  1
  [35] .debug_abbrev     PROGBITS        0000000000000000 f5ec6b 016ea3 00      0   0  1
  [36] .debug_line       PROGBITS        0000000000000000 f75b0e 2d3e42 00      0   0  1
  [37] .debug_str        PROGBITS        0000000000000000 1249950 645609 01  MS  0   0  1
  [38] .debug_pubtypes   PROGBITS        0000000000000000 188ef59 00193e 00      0   0  1
  [39] .debug_ranges     PROGBITS        0000000000000000 1890897 39f280 00      0   0  1
  [40] .symtab           SYMTAB          0000000000000000 1c2fb18 04cd40 18     41 7203  8
  [41] .strtab           STRTAB          0000000000000000 1c7c858 0dc411 00      0   0  1
  [42] .shstrtab         STRTAB          0000000000000000 1d58c69 0001d0 00      0   0  1

File: target2/release/rg
There are 43 section headers, starting at offset 0x1e30738:

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
[...]
  [32] .debug_aranges    PROGBITS        0000000000000000 3fe8c0 037f50 00      0   0  1
  [33] .debug_pubnames   PROGBITS        0000000000000000 436810 589469 00      0   0  1
  [34] .debug_info       PROGBITS        0000000000000000 9bfc79 60c3e5 00      0   0  1
  [35] .debug_abbrev     PROGBITS        0000000000000000 fcc05e 01d408 00      0   0  1
  [36] .debug_line       PROGBITS        0000000000000000 fe9466 331459 00      0   0  1
  [37] .debug_str        PROGBITS        0000000000000000 131a8bf 645c62 01  MS  0   0  1
  [38] .debug_pubtypes   PROGBITS        0000000000000000 1960521 00659a 00      0   0  1
  [39] .debug_ranges     PROGBITS        0000000000000000 1966abb 39fbb0 00      0   0  1
  [40] .symtab           SYMTAB          0000000000000000 1d06670 04cdd0 18     41 7205  8
  [41] .strtab           STRTAB          0000000000000000 1d53440 0dd123 00      0   0  1
  [42] .shstrtab         STRTAB          0000000000000000 1e30563 0001d0 00      0   0  1

@michaelwoerister

I could imagine that LLVM is less able to merge debuginfo with the hack removed, which results in more duplicated debuginfo?

Yeah, that sounds plausible. I could also imagine that the former hack might have caused us to lose data, so the new change is possibly better, more complete -- but I don't know good tools for really teasing out the difference.

In any case, I think increases in debuginfo size are acceptable, as long as it's not absurd.

[michaelwoerister]

FWIW, I'm glad the hack has been removed 😉 I was always surprised that it apparently didn't cause trouble.