Update rustc-hash to version 2 #129533
Closed
Update rustc-hash to version 2 #129533
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @Mark-Simulacrum. Use |
|
@bors try @rust-timer queue |
rustbot
added
A-rustdoc-json
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. Some changes occurred in exhaustiveness checking cc @Nadrieril rustdoc-json-types is a public (although nightly-only) API. If possible, consider changing |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Aug 24, 2024
Update rustc-hash to version 2 This brings in the new algorithm. see rust-lang/rustc-hash#37 and rust-lang#125133
This comment has been minimized.
This comment has been minimized.
huh. |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
stuff relies on hashmap iteration order lol. it's sorta stable with fxhash as it has a fixed seed |
|
deep sigh |
|
Finished benchmarking commit (1823acf): comparison URL. Overall result: ❌✅ regressions and improvements - 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. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)Results (primary -1.3%)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.
CyclesResults (secondary -2.7%)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.
Binary sizeResults (secondary 0.0%)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.
Bootstrap: 750.904s -> 751.906s (0.13%) |
rustbot
added
perf-regression
|
On the bright side, we simultaneously reveal everything that depends on hashmap iteration order and get a faster compiler. |
Mark-Simulacrum
added
S-waiting-on-author
alex-semenyuk
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
|
uhh wtf?? @GuillaumeGomez do you have an idea why changing fxhash would cause rustdoc-gui tests to fail to find fonts? |
|
Wth. Someone cursed this CI run, I have no other explanation... I think it's safe to ignore this failure (or just make another CI run). |
This brings in the new algorithm.
Just because the code says it's OK does not mean that it actually is OK. Nodes with the same total size were not sorted, their order relied on hashmap iteration.
|
you know, i wanted to ignore this failure but when i re-ran it and it failed again D:. well, let's rebase and try again, maybe it works now. hopefully. but thanks for the confirmation that i'm not the only confused person about this |
|
One option if this CI run doesn't succeed is disabling the rustdoc-gui suite. Does anyone have any better ideas? |
|
The job Click to see the possible cause of the failure (guessed by this bot) |
https://github.com/rust-lang/rust/blob/master/src/librustdoc/html/static/css/rustdoc.css have hardcoded filenames with hashes. Updated hashes: |
Please don't. I'll take a look in a few days when I'm back. I think something unrelated like maybe disk space is in cause. |
No, the error is actually accurate (and it's concerning that (almost) everyone see's a rustdoc-gui failure and assumes it's something spooky). Fix/explanation is here: #131908 |
workingjubilee
added a commit
to workingjubilee/rustc
that referenced
this pull request
Oct 19, 2024
…g, r=notriddle,GuillaumeGomez rustdoc: Switch from FxHash to sha256 for static file hashing. Fixes rust-lang#129533 (comment) fxhash isn't well defined, and it's implementation is being changed in rust-lang#129533. But because rustdoc uses it for static files (and encodes that hashing in rustdoc.css), this broke our tests. Given that this isn't performace critical, I think the right fix is to used a well-defined hash that will never change its definition. I've picked (rather arbitrarily) sha256.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 19, 2024
…g, r=notriddle,GuillaumeGomez rustdoc: Switch from FxHash to sha256 for static file hashing. Fixes rust-lang#129533 (comment) fxhash isn't well defined, and it's implementation is being changed in rust-lang#129533. But because rustdoc uses it for static files (and encodes that hashing in rustdoc.css), this broke our tests. Given that this isn't performace critical, I think the right fix is to used a well-defined hash that will never change its definition. I've picked (rather arbitrarily) sha256.
matthiaskrgr
added a commit
to matthiaskrgr/rust
that referenced
this pull request
Oct 19, 2024
…g, r=notriddle,GuillaumeGomez rustdoc: Switch from FxHash to sha256 for static file hashing. Fixes rust-lang#129533 (comment) fxhash isn't well defined, and it's implementation is being changed in rust-lang#129533. But because rustdoc uses it for static files (and encodes that hashing in rustdoc.css), this broke our tests. Given that this isn't performace critical, I think the right fix is to used a well-defined hash that will never change its definition. I've picked (rather arbitrarily) sha256.
rust-timer
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 19, 2024
Rollup merge of rust-lang#131908 - aDotInTheVoid:rustdoc-gamer-hashing, r=notriddle,GuillaumeGomez rustdoc: Switch from FxHash to sha256 for static file hashing. Fixes rust-lang#129533 (comment) fxhash isn't well defined, and it's implementation is being changed in rust-lang#129533. But because rustdoc uses it for static files (and encodes that hashing in rustdoc.css), this broke our tests. Given that this isn't performace critical, I think the right fix is to used a well-defined hash that will never change its definition. I've picked (rather arbitrarily) sha256.
|
ferris forgor |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 19, 2024
…try> Update rustc-hash to version 2 but again it's like rust-lang#129533 but not closed by bors and rebased r? WaffleLapkin meow
|
Eeeeek, sorry! |
|
it's ok, you were gaming hard on your pr ❤️ |
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 20, 2024
…affleLapkin Update rustc-hash to version 2 but again it's like rust-lang#129533 but not closed by bors and rebased r? WaffleLapkin meow
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
Oct 20, 2024
…affleLapkin Update rustc-hash to version 2 but again it's like rust-lang#129533 but not closed by bors and rebased r? WaffleLapkin meow
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Dec 17, 2024
In rust-lang#129533 the main hash function changed and the order of `-Z input-stats` output changed, which showed that it is dependent on the hash function, even though it is sorted. That's because entries with the same cumulative size are ordered in a way that depends on the hash function. This commit fixes that by using the entry label as the secondary ordering key.
nnethercote
added a commit
to nnethercote/rust
that referenced
this pull request
Dec 17, 2024
In rust-lang#129533 the main hash function changed and the order of `-Z input-stats` output changed, which showed that it is dependent on the hash function, even though it is sorted. That's because entries with the same cumulative size are ordered in a way that depends on the hash function. This commit fixes that by using the entry label as the secondary ordering key.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This brings in the new algorithm.
see rust-lang/rustc-hash#37 and #125133