Skip to content

Remove LazyMeta::min_size #92497

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 1 commit into from
Jan 9, 2022
Merged

Remove LazyMeta::min_size #92497

merged 1 commit into from
Jan 9, 2022

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 2, 2022

It is extremely conservative and as such barely reduces the size of encoded Lazy distances, but does increase complexity.

@bjorn3 bjorn3 added C-cleanup Category: PRs that clean code up or issues documenting cleanup. A-metadata Area: Crate metadata T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 2, 2022
@rust-highfive
Copy link
Contributor

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2022
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 2, 2022

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 2, 2022
@bors
Copy link
Collaborator

bors commented Jan 2, 2022

⌛ Trying commit c47b3836097d859942d4d9aa6fb8bd798b4efad2 with merge 66f0299604f575b12a6356529d3fb1d7e72505bb...

@bors
Copy link
Collaborator

bors commented Jan 2, 2022

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

@rust-timer
Copy link
Collaborator

Queued 66f0299604f575b12a6356529d3fb1d7e72505bb with parent 03360be, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (66f0299604f575b12a6356529d3fb1d7e72505bb): comparison url.

Summary: This change led to small relevant improvements 🎉 in compiler performance.

  • Small improvement in instruction counts (up to -0.3% on full builds of helloworld)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

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 led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jan 2, 2022
@michaelwoerister
Copy link
Member

r? @eddyb how knows this code much better than me

eddyb
eddyb approved these changes Jan 7, 2022
View reviewed changes
Copy link
Member

@eddyb eddyb 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 that comment tweaked to not say "conservative estimate"

compiler/rustc_metadata/src/rmeta/mod.rs
Comment on lines -67 to -68
// FIXME(eddyb) Give better estimates for certain types.
fn min_size(meta: Self::Meta) -> usize;
Copy link
Member

Choose a reason for hiding this comment

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

I suppose I had hoped to be able to predict certain sizes well enough to have a lot of single-byte LEB128 encodings, but in retrospect it doesn't seem doable without a bunch of custom derives. In most cases, distances under 128 wouldn't make a difference anyway, and I suspect that's most of what min_size could ever handle (outside of Lazy<[T]>, I suppose).

@bjorn3
Remove LazyMeta::min_size
717d4b3 
It is extremely conservative and as such barely reduces the size of
encoded Lazy distances, but does increase complexity.
@bjorn3 bjorn3 force-pushed the remove_lazy_meta_min_size branch from c47b383 to 717d4b3 Compare January 8, 2022 12:56
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 8, 2022

@bors r=eddyb

@bors
Copy link
Collaborator

bors commented Jan 8, 2022

📌 Commit 717d4b3 has been approved by eddyb

@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 Jan 8, 2022
@bors
Copy link
Collaborator

bors commented Jan 9, 2022

⌛ Testing commit 717d4b3 with merge 02fe61b...

@bors
Copy link
Collaborator

bors commented Jan 9, 2022

☀️ Test successful - checks-actions
Approved by: eddyb
Pushing 02fe61b to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 9, 2022
@bors bors merged commit 02fe61b into rust-lang:master Jan 9, 2022
@rustbot rustbot added this to the 1.60.0 milestone Jan 9, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (02fe61b): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@bjorn3 bjorn3 deleted the remove_lazy_meta_min_size branch January 9, 2022 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
A-metadata Area: Crate metadata C-cleanup Category: PRs that clean code up or issues documenting cleanup. 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.60.0
Development

Successfully merging this pull request may close these issues.
7 participants
@bjorn3 @rust-highfive @rust-timer @bors @michaelwoerister @eddyb @rustbot