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.

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

Replace some u64 hashes with Hash64 #137095

Merged
merged 2 commits into from
Feb 17, 2025
Merged

Conversation

saethlin
Copy link
Member

@saethlin saethlin commented Feb 15, 2025

I introduced the Hash64 and Hash128 types in #110083, essentially as a mechanism to prevent hashes from landing in our leb128 encoding paths. If you just have a u64 or u128 field in a struct then derive Encodable/Decodable, that number gets leb128 encoding. So if you need to store a hash or some other value which behaves very close to a hash, don't store it as a u64.

This reverts part of #117603, which turned an encoded Hash64 into a u64.

Based on #110083, I don't expect this to be perf-sensitive on its own, though I expect that it may help stabilize some of the small rmeta size fluctuations we currently see in perf reports.

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2025

Could not assign reviewer from: workingjubilee.
User(s) workingjubilee are either the PR author, already assigned, or on vacation. Please use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2025

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Feb 15, 2025
@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2025

rust-analyzer is developed in its own repository. If possible, consider making this change to rust-lang/rust-analyzer instead.

cc @rust-lang/rust-analyzer

Copy link
Member

@Veykril Veykril left a comment

Choose a reason for hiding this comment

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

rust-analyzer can't depend on rustc_data_structures since it doesn't compile on stable

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

time to put the wrapper in rustc_index :3

@saethlin
Copy link
Member Author

Actually I was thinking about creating rustc_nightly_data_structures but I haven't looked at how much of rustc_data_structures needs nightly yet.

@workingjubilee
Copy link
Member

Potentially silly alternative: byte arrays?

@saethlin
Copy link
Member Author

In practice, all the little helper methods that permit numeric operations while keeping the containment of "do not encode me with leb128" are quite useful in the compiler. And these types get passed around a lot so I think it would be a silly maintenance issue to have multiple of them. I'll just figure out how to put these helper types in a crate that r-a can use.

@workingjubilee
Copy link
Member

workingjubilee commented Feb 15, 2025

Sounds good.

Also, @saethlin can you add this to the docs for Hash64? And likewise for Hash128.

+/// A hashed u64 encoded with a stable size that is more compact than u64's ULEB128 encoding
 #[derive(Clone, Copy, PartialEq, Eq, Hash, PartialOrd, Ord, Default)]
 pub struct Hash64 {

That way it appears in hover-docs.

@workingjubilee
Copy link
Member

workingjubilee commented Feb 15, 2025

Any variation on that sentence you want is fine, as long as it is a single one so that it consistently renders correctly.

@rustbot rustbot added the A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) label Feb 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

@saethlin saethlin force-pushed the use-hash64-for-hashes branch from 9b2f0af to 7b99665 Compare February 16, 2025 11:33
@saethlin
Copy link
Member Author

saethlin commented Feb 16, 2025

I factored this now so that the HashN types are in a new crate called rustc_hashes. I think this works? The diff touches a lot of files now which is unfortunate, but any proper solution to code sharing with r-a like this is going to have to touch a lot of files. Happy to try out another strategy if people think it will turn out better.

Also, the Encodable/Decodable impls for these types now live in rustc_serialize, which is producing a strange diff visualization.

@workingjubilee
Copy link
Member

Thanks, that's great!

This should be fine.

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Feb 16, 2025

📌 Commit 7b99665 has been approved by workingjubilee

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 Feb 16, 2025
@workingjubilee
Copy link
Member

Wait, I just remembered, there's a test to update, lemme check...

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 16, 2025
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 16, 2025
@workingjubilee
Copy link
Member

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Feb 16, 2025
@saethlin saethlin force-pushed the use-hash64-for-hashes branch from cd11624 to 4cf2186 Compare February 16, 2025 21:18
@saethlin
Copy link
Member Author

Since you wanted to mark this for rollup, I'll wait until CI passes to re-add it to the queue

@workingjubilee
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 16, 2025

📌 Commit 4cf2186 has been approved by workingjubilee

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2025
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 16, 2025
…workingjubilee

Replace some u64 hashes with Hash64

I introduced the Hash64 and Hash128 types in rust-lang#110083, essentially as a mechanism to prevent hashes from landing in our leb128 encoding paths. If you just have a u64 or u128 field in a struct then derive Encodable/Decodable, that number gets leb128 encoding. So if you need to store a hash or some other value which behaves very close to a hash, don't store it as a u64.

This reverts part of rust-lang#117603, which turned an encoded Hash64 into a u64.

Based on rust-lang#110083, I don't expect this to be perf-sensitive on its own, though I expect that it may help stabilize some of the small rmeta size fluctuations we currently see in perf reports.
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#136671 (Overhaul `rustc_middle::limits`)
 - rust-lang#136817 (Pattern Migration 2024: clean up and comment)
 - rust-lang#136844 (Use `const_error!` when possible)
 - rust-lang#136953 (rustc_target: import TargetMetadata)
 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137140 (Fix const items not being allowed to be called `r#move` or `r#static`)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fab3837 into rust-lang:master Feb 17, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Feb 17, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
Rollup merge of rust-lang#137095 - saethlin:use-hash64-for-hashes, r=workingjubilee

Replace some u64 hashes with Hash64

I introduced the Hash64 and Hash128 types in rust-lang#110083, essentially as a mechanism to prevent hashes from landing in our leb128 encoding paths. If you just have a u64 or u128 field in a struct then derive Encodable/Decodable, that number gets leb128 encoding. So if you need to store a hash or some other value which behaves very close to a hash, don't store it as a u64.

This reverts part of rust-lang#117603, which turned an encoded Hash64 into a u64.

Based on rust-lang#110083, I don't expect this to be perf-sensitive on its own, though I expect that it may help stabilize some of the small rmeta size fluctuations we currently see in perf reports.
@saethlin saethlin deleted the use-hash64-for-hashes branch February 17, 2025 14:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-run-make Area: port run-make Makefiles to rmake.rs 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
Development

Successfully merging this pull request may close these issues.

7 participants