CGU cleanups #111899
CGU cleanups #111899
Conversation
It is small and has a single call site, and this change will facilitate the next commit.
Three of the four methods in `DefaultPartitioning` are defined in `default.rs`. But `merge_codegen_units` is defined in a separate module, `merging`, even though it's less than 100 lines of code and roughly the same size as the other three methods. (Also, the `merging` module currently sits alongside `default`, when it should be a submodule of `default`, adding to the confusion.) In rust-lang#74275 this explanation was given: > I pulled this out into a separate module since it seemed like we might > want a few different merge algorithms to choose from. But in the three years since there have been no additional merging algorithms, and there is no mechanism for choosing between different merging algorithms. (There is a mechanism, `-Zcgu-partitioning-strategy`, for choosing between different partitioning strategies, but the merging algorithm is just one piece of a partitioning strategy.) This commit merges `merging` into `default`, making the code easier to navigate and read.
I find that these structs obfuscate the code. Removing them and just passing the individual fields around makes the `Partition` method signatures a little longer, but makes the data flow much clearer. E.g. - `codegen_units` is mutable all the way through. - `codegen_units`'s length is changed by `merge_codegen_units`, but only the individual elements are changed by `place_inlined_mono_items` and `internalize_symbols`. - `roots`, `internalization_candidates`, and `mono_item_placements` are all immutable after creation, and all used by just one of the four methods.
This is something that took me some time to figure out.
rustbot
added
S-waiting-on-review
|
This shouldn't affect perf, but just in case: @bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit 59c5259 with merge cd79982c3a8def178ac0fa0fe29534d85b88af99... |
|
☀️ Try build successful - checks-actions |
1 similar comment
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (cd79982c3a8def178ac0fa0fe29534d85b88af99): comparison URL. Overall result: no relevant changes - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 647.009s -> 648.77s (0.27%) |
rustbot
removed
the
S-waiting-on-perf
|
@bors rollup |
| @@ -26,7 +24,7 @@ impl<'tcx> Partition<'tcx> for DefaultPartitioning { | |||
| &mut self, | |||
| cx: &PartitioningCx<'_, 'tcx>, | |||
| mono_items: &mut I, | |||
| ) -> PreInliningPartitioning<'tcx> | |||
| ) -> (Vec<CodegenUnit<'tcx>>, FxHashSet<MonoItem<'tcx>>, FxHashSet<MonoItem<'tcx>>) | |||
There was a problem hiding this comment.
Returning a tuple of two values with the same type is a little confusing, I'd prefer seeing the struct here.
| } | ||
|
|
||
| fn merge_codegen_units( | ||
| &mut self, | ||
| cx: &PartitioningCx<'_, 'tcx>, | ||
| initial_partitioning: &mut PreInliningPartitioning<'tcx>, | ||
| codegen_units: &mut Vec<CodegenUnit<'tcx>>, |
There was a problem hiding this comment.
removing the structs in argument position is nice because there are still names attached to it
There was a problem hiding this comment.
I agree with @Nilstrieb's comments about using a struct for the return value of place_root_mono_items. r=me with that change 🙂
As per review request.
|
I added the struct as requested. @bors r=wesleywiser |
bors
added
S-waiting-on-bors
…wiser CGU cleanups Some code clarity improvements I found when reading this code closely. r? `@wesleywiser`
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#111384 (Fix linking Mac Catalyst by including LC_BUILD_VERSION in object files) - rust-lang#111899 (CGU cleanups) - rust-lang#111940 (Clarify safety concern of `io::Read::read` is only relevant in unsafe code) - rust-lang#111947 (Add test for RPIT defined with different hidden types with different substs) - rust-lang#111951 (Correct comment on privately uninhabited pattern.) Failed merges: - rust-lang#111954 (improve error message for calling a method on a raw pointer with an unknown pointee) r? `@ghost` `@rustbot` modify labels: rollup
Some code clarity improvements I found when reading this code closely.
r? @wesleywiser