cg_llvm: fewer_names in uncached_llvm_type

[davidtwco]

This PR changes uncached_llvm_type so that a named struct type (with an empty name) is always created when the fewer_names option is enabled. By skipping the generation of names, we can improve perf. Giving LLVMStructCreateNamed an empty name works because LLVM will perform random renames to avoid collisions. Needs a perf run!

cc @eddyb (whom I've discussed this with)

[rust-highfive]

r? @eddyb

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

[Mark-Simulacrum]

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 2a36ad0fba34ca0971bd0de139958cb2f2f7fa11 with merge bb00eaa22b9a71d4409837350e6428ad8cca832c...

[davidtwco]

@bors try (after fixing the use of fewer_names)

[bors]

⌛ Trying commit 3be8dca04d7021e24a236f7feb841226daa647c5 with merge d71677e4467122732f2d745104d4f694320de5cf...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: d71677e4467122732f2d745104d4f694320de5cf (d71677e4467122732f2d745104d4f694320de5cf)

[rust-timer]

Queued d71677e4467122732f2d745104d4f694320de5cf with parent 3e3c552, future comparison URL.

[rust-timer]

Finished benchmarking try commit (d71677e4467122732f2d745104d4f694320de5cf): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[wesleywiser]

The wins seem to be almost exclusively in the incremental tests (there are a few 0-2% wins in the FULL tests). Is that expected? Do we know why that is?

[eddyb]

@wesleywiser most of what incr-patched rebuilds do is generate LLVM for everything in a CGU, most of which isn't actually changed, but we have no way to partially update the LLVM module corresponding to the CGU (I don't even think we keep the pre-optimization LLVM module bitcode anyway).

So what this is showing is that codegen and/or LLVM optimizations are faster. The 0-2% wins in full/incr-full are probably the same, there's just the regular Rust compilation as well, so it's much less of the total.

---

Although looking at the absolute values, I'm starting to doubt that, maybe the dep graph has less (false?) edges thanks to not looking at the types in full detail?

Maybe we could figure out if it's that, by using a FmtWriter with a fmt::Write implementation with a noop write_str, and still passing String::new() to LLVM, so that most of the effect is making all of the queries in ty::print::pretty.
(or I guess the old ty::print::obsolete? not sure if that was more/less expensive, in terms of the dep graph)

[eddyb]

@bors try @rust-timer queue (we now allow deduplication of non-ty::Adt LLVM types)

[rust-timer]

Awaiting bors try build completion

[bors]

⌛ Trying commit 0100c83c0743ba0164edbb931ba513dedbd26c69 with merge b86ce2d9e7d96d5a001f3d8fbd61161e351c4f60...

[bors]

☀️ Try build successful - checks-actions, checks-azure
Build commit: b86ce2d9e7d96d5a001f3d8fbd61161e351c4f60 (b86ce2d9e7d96d5a001f3d8fbd61161e351c4f60)

[rust-timer]

Queued b86ce2d9e7d96d5a001f3d8fbd61161e351c4f60 with parent 7b1dd61, future comparison URL.

[rust-timer]

Finished benchmarking try commit (b86ce2d9e7d96d5a001f3d8fbd61161e351c4f60): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never

[eddyb]

I was excited about the difference between the two perf runs but we didn't do it soon enough and 3 PRs landed in between, which seem to account for all the wins except clap-rs-debug.

So overall I feel like the final version is an improvement, even if slight.

[davidtwco]

Merge failure looks spurious?

[davidtwco]

Re-approving, previous merge failure has been seen on other PRs so is likely spurious.

@bors r=eddyb

[bors]

📌 Commit fa01ce8 has been approved by eddyb

[bors]

⌛ Testing commit fa01ce8 with merge 1d22f75...

[bors]

☀️ Test successful - checks-actions, checks-azure
Approved by: eddyb
Pushing 1d22f75 to master...

[tmiasko]

Giving LLVMStructCreateNamed an empty name works because LLVM will perform random renames to avoid collisions

The renaming scheme applies only if type name is non-empty, It might be plausible to try landing this again with a fixed with non-empty string.

There is also additional subtle side effect of this changes. For LTO, the merged codegen unit contains substantially more bitcasts than before (since type names between different units are unrelated). While bitcasts, shouldn't be significant, it wouldn't be exactly surprising if they have some effect, at least until migration to opaque pointer is complete in LLVM.

[tgnottingham]

@tmiasko, I applied your suggestion of using a fixed non-empty string ("x" in particular -- does it matter?) and ran benchmarks locally. Results were still impressive:

I'll leave this to you or @davidtwco to apply, since I'm just drive-by benchmarking.

Other than the bitcasting issue tmiasko mentioned, is there any reason we shouldn't do this, and should try to optimize name generation instead?

I also don't understand why this seems to have huge effects on some benchmarks, and no effect on similar benchmarks. E.g. why should this have 0% change on cargo-debug incr-full, but give a ~60% improvement on cargo-debug incr-patched-println?

[tgnottingham]

@eddyb

I didn't go as far as what you suggested in #76030 (comment), but I did look at the CGU reuse for the cargo-debug incr-patched-println benchmark before and after this change.

Before the change, there were 16 CGUs whose artifacts couldn't be reused. After the change, there were only 2. This likely explains why this change only had a dramatic effect on incr-patched builds. It also potentially suggests that there are false edges in the dependency graph, which you alluded to. Finally, it goes to show how much profit we might derive from improving the CGU invalidation situation.

Also, this change made it so that the name is set to None for types like Closure, Foreign, etc. when fewer_names is enabled. Was that intended? Edit: Oh, just saw the code review comment that talked about this. Don't quite understand, but at least I know it was intended.