Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

More CGU cleanups #112369

Merged
merged 7 commits into from
Jun 10, 2023
Merged

More CGU cleanups #112369

merged 7 commits into from
Jun 10, 2023

Conversation

nnethercote
Copy link
Contributor

An assortment of improvements.

r? @wesleywiser

@nnethercote
Remove some unnecessary &muts.
0a1cd5b
@nnethercote
Improve sorting in debug_dump.
9fd6d97 
Currently it sorts by symbol name, which is a mangled name like
`_ZN1a4main17hb29587cdb6db5f42E`, which leads to non-obvious orderings.

This commit changes it to use the existing
`items_in_deterministic_order`, which iterates in source code order.
@nnethercote
Make the two loops in internalize_symbols have the same form.
392045b 
Because the next commit will merge them.
@nnethercote
Merge the two loops in internalize_symbols.
fe3b646 
Because they have a lot of overlap.
@nnethercote
Split loop in place_inlined_mono_item.
8dbb347 
This loop is doing two different things. For inlined items, it's adding
them to the CGU. For all items, it's recording them in
`mono_item_placements`.

This commit splits it into two separate loops. This avoids putting root
mono items into `reachable`, and removes the low-value check that
`roots` doesn't contain inlined mono items.
@nnethercote
Remove PlacedRootMonoItems::roots.
1defd30 
It's no longer used.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 7, 2023
@nnethercote
Copy link
Contributor Author

@bors rollup

@nnethercote
Move mono_item_placement construction.
8533456 
It's currently created in `place_inlined_mono_items` and then used in
`internalize_symbols`. This commit moves the creation to
`internalize_symbols`.
@nnethercote
Copy link
Contributor Author

Should have no perf effect, let's check:

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 7, 2023
@bors
Copy link
Contributor

bors commented Jun 7, 2023

⌛ Trying commit 8533456 with merge fcef110a8a3c14f8861bf33a4688867d2c019d81...

@bors
Copy link
Contributor

bors commented Jun 7, 2023

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

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (fcef110a8a3c14f8861bf33a4688867d2c019d81): comparison URL.

Overall result: ❌ regressions - no action needed

Benchmarking 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
@rustbot label: -S-waiting-on-perf -perf-regression

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)		 - - 0
Regressions ❌
(secondary)		 5.2% [5.2%, 5.2%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

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)		 3.3% [3.3%, 3.3%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -1.5% [-2.3%, -1.0%] 3
All ❌✅ (primary) 3.3% [3.3%, 3.3%] 1

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)		 1.5% [1.5%, 1.5%] 1
Regressions ❌
(secondary)		 2.5% [2.5%, 2.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 1.5% [1.5%, 1.5%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 647.37s -> 646.951s (-0.06%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 7, 2023
@Kobzol
Copy link
Contributor

Kobzol commented Jun 7, 2023

coercions debug incr-full has become very noisy recently, so this is just noise.

@nnethercote
Copy link
Contributor Author

No perf effects, as expected.

@bors rollup

wesleywiser
wesleywiser approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with or without nit 🙂

compiler/rustc_monomorphize/src/partitioning.rs
// without marking this one as internal.
continue;

if !single_codegen_unit {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe just leave some kind of comment here (could copy from the deleted one) about why it's ok to skip this check if there's only one CGU

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I felt like the "Found a user from another CGU" comment below covered the reasoning sufficiently here.

@nnethercote
Copy link
Contributor Author

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Jun 9, 2023

📌 Commit 8533456 has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jun 10, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 10, 2023
@bors
Auto merge of rust-lang#112485 - matthiaskrgr:rollup-ta84xje, r=matth…
4b71d79 
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#110141 (expand: Change how `#![cfg(FALSE)]` behaves on crate root)
 - rust-lang#112369 (More CGU cleanups)
 - rust-lang#112467 (Compile rustc_driver by default)
 - rust-lang#112468 (Change format of rustdoc-js tests by putting query and correction directly alongside the expected values)
 - rust-lang#112473 (Update cargo)
 - rust-lang#112481 (Ignore tests that hang in new solver)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3189ce6 into rust-lang:master Jun 10, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 10, 2023
@nnethercote nnethercote deleted the more-cgu-cleanups branch June 12, 2023 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.
6 participants
@nnethercote @rust-timer @bors @Kobzol @wesleywiser @rustbot