Skip to content

Streamline size estimates (take 2) #113772

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
Merged
merged 4 commits into from
Jul 17, 2023
Merged

Streamline size estimates (take 2) #113772

merged 4 commits into from
Jul 17, 2023

Conversation

nnethercote
Copy link
Contributor

This was merged in #113684 but then something happened:

There has been a bors issue that lead to the merge commit of this PR getting purged from master.
You'll have to make a new PR to reapply it.

So this is exactly the same changes.

@bors r=wesleywiser

@nnethercote
Introduce MonoItemData.
b52f9eb 
It replaces `(Linkage, Visibility)`, making the code nicer. Plus the
next commit will add another field.
@nnethercote
Store item size estimate in MonoItemData.
edd1f38 
This means we call `MonoItem::size_estimate` (which involves a query)
less often: just once per mono item, and then once more per inline item
placement. After that we can reuse the stored value as necessary. This
means `CodegenUnit::compute_size_estimate` is cheaper.
@nnethercote
Ignore unreachable inlined items in debug_dump.
87c509d 
They're quite rare, and ignoring them simplifies things quite a bit, and
further reduces the number of calls to `MonoItem::size_estimate` to the
number of placed items (one per root item, and one or more per reachable
inlined item).
@nnethercote
Remove instance_def_size_estimate query.
005a70e 
It doesn't seem worthwhile now that `MonoItem::size_estimate` is called
much less often.
@rustbot
Copy link
Collaborator

rustbot commented Jul 16, 2023

r? @WaffleLapkin

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

@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 Jul 16, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jul 16, 2023

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@nnethercote
Copy link
Contributor Author

@bors r=wesleywiser

@bors
Copy link
Collaborator

bors commented Jul 16, 2023

📌 Commit 005a70e 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 Jul 16, 2023
@bors
Copy link
Collaborator

bors commented Jul 17, 2023

⌛ Testing commit 005a70e with merge 299179e...

@bors
Copy link
Collaborator

bors commented Jul 17, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 299179e to master...

1 similar comment
@bors
Copy link
Collaborator

bors commented Jul 17, 2023

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 299179e to master...

@bors bors added merged-by-bors This PR was explicitly merged by bors. labels Jul 17, 2023
@bors bors merged commit 299179e into rust-lang:master Jul 17, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 17, 2023
@nnethercote nnethercote deleted the streamline-size-estimates-2 branch July 17, 2023 04:52
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (299179e): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -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)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -0.8% [-1.1%, -0.5%] 2
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)		 - - 0
Regressions ❌
(secondary)		 2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

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

Binary size

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

Bootstrap: 656.194s -> 657.644s (0.22%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@WaffleLapkin WaffleLapkin

Labels
merged-by-bors This PR was explicitly merged by bors. 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.73.0
Development

Successfully merging this pull request may close these issues.
5 participants
@nnethercote @rustbot @bors @rust-timer @WaffleLapkin