Improve cgu naming and ordering #112946
Merged
Improve cgu naming and ordering #112946
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rustbot
added
S-waiting-on-review
nnethercote
force-pushed
the
improve-cgu-naming-and-ordering
branch
from
fecb794
to
d65ca60
Compare
bjorn3
reviewed
Jun 23, 2023
bjorn3
reviewed
Jun 23, 2023
Currently there are two problems. First, the CGUS don't end up in size order. The merging loop does sort by size on each iteration, but we don't sort after the final merge, so typically there is one CGU out of place. (And sometimes we don't enter the merging loop at all, in which case they end up in random order.) Second, we then assign names that differ only by a numeric suffix, and then we sort them lexicographically by name, giving us an order like this: regex.f10ba03eb5ec7975-cgu.1 regex.f10ba03eb5ec7975-cgu.10 regex.f10ba03eb5ec7975-cgu.11 regex.f10ba03eb5ec7975-cgu.12 regex.f10ba03eb5ec7975-cgu.13 regex.f10ba03eb5ec7975-cgu.14 regex.f10ba03eb5ec7975-cgu.15 regex.f10ba03eb5ec7975-cgu.2 regex.f10ba03eb5ec7975-cgu.3 regex.f10ba03eb5ec7975-cgu.4 regex.f10ba03eb5ec7975-cgu.5 regex.f10ba03eb5ec7975-cgu.6 regex.f10ba03eb5ec7975-cgu.7 regex.f10ba03eb5ec7975-cgu.8 regex.f10ba03eb5ec7975-cgu.9 These two problems are really annoying when debugging and profiling the CGUs. This commit ensures CGUs are sorted by name *and* reverse sorted by size. This involves (a) one extra sort by size operation, and (b) padding the numeric indices with zeroes, e.g. `regex.f10ba03eb5ec7975-cgu.01`. (Note that none of this applies for incremental builds, where a different hash-based CGU naming scheme is used.)
For non-incremental builds on Unix, currently all the thread names look like `opt regex.f10ba03eb5ec7975-cgu.0`. But they are truncated by `pthread_setname` to `opt regex.f10ba`, hiding the numeric suffix that distinguishes them. This is really annoying when using a profiler like Samply. This commit changes these thread names to a form like `opt cgu.0`, which is much better.
nnethercote
force-pushed
the
improve-cgu-naming-and-ordering
branch
from
d65ca60
to
666b1b6
Compare
wesleywiser
approved these changes
Jun 28, 2023
|
@bors r+ |
bors
added
S-waiting-on-bors
|
@bors rollup |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Jun 29, 2023
Rollup of 5 pull requests Successful merges: - rust-lang#112946 (Improve cgu naming and ordering) - rust-lang#113048 (Fix build on Solaris where fd-lock cannot be used.) - rust-lang#113100 (Fix display of long items in search results) - rust-lang#113107 (add check for ConstKind::Value(_) to in_operand()) - rust-lang#113119 (rustdoc: Reduce internal function visibility.) r? `@ghost` `@rustbot` modify labels: rollup
|
For some reason the fuchsia project's windows build of rustc started failing around the time of this PR with: This PR seemed like the most likely candidate from the rollup. Any insight as to what would cause this? |
|
@ComputerDruid: yes, this PR is almost certainly the cause of that failure. Is it really Windows only? That would be pretty weird. There must be a wrinkle with the CGU naming I'm not aware of, but I looked over the code again and I can't see what it is. It seems like the kind of code where any problem should show up frequently on all platforms, not rarely and on a single platform. I see three possible paths forward.
What do you think? |
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Jul 6, 2023
An assertion failure was reported in rust-lang#112946. This extra information will help diagnose the problem.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Jul 6, 2023
…, r=lqd Diagnose unsorted CGUs. An assertion failure was reported in rust-lang#112946. This extra information will help diagnose the problem. r? `@lqd`
|
I created #113402 for the third option mentioned above. |
|
Thanks for that! https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/rust-x64-windows/b8776317651163748305/overview is the first build with that PR and the error looks like: The full output got somewhat interleaved, but here's 2 instances interleaved a bit: |
|
Re: only happening on windows; yes, I've only seen this on our windows build. We don't actually use the windows build for anything right now (there's been some attempts to get fuchsia's developer tooling to build for windows, but as far as I know that work has not completed), so it's not really load-bearing like our linux build is. But it does show up on my build dashboard so I figured I'd raise the issue. You can actually see the config.toml we use, and the environment variables we set Comparing that to the config.toml from a working linux build the only difference I see that seems like it might be relevant is |
|
And when I tried switching |
|
Thanks for the updated info. This warrants its own issue, I have filed #113425, let's move the conversation there. |
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Jul 11, 2023
PR rust-lang#112946 tweaked the naming of LLVM threads, but messed things up, resulting in threads on Windows having names like `optimize module {} regex.f10ba03eb5ec7975-cgu.0`. This commit removes the extraneous braces.
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Jul 16, 2023
PR rust-lang#112946 tweaked the naming of LLVM threads, but messed things up slightly, resulting in threads on Windows having names like `optimize module {} regex.f10ba03eb5ec7975-cgu.0`. This commit removes the extraneous `{} `.
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Jul 31, 2023
PR rust-lang#112946 tweaked the naming of LLVM threads, but messed things up slightly, resulting in threads on Windows having names like `optimize module {} regex.f10ba03eb5ec7975-cgu.0`. This commit removes the extraneous `{} `.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some quality of life improvements when debugging and profiling CGU formation.
r? @wesleywiser