compiler: give ExternAbi truly stable Hash and Ord
#136901
Conversation
The last public reexport of rustc_abi in rustc_target is finally gone.
rustbot
added
A-query-system
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
|
As one might imagine, the first two commits are the cleanup I was doing before I noticed we could do this. |
This comment has been minimized.
This comment has been minimized.
workingjubilee
force-pushed
the
stabilize-externabi-hashing-forever
branch
from
e7103b9 to
2150c83
Compare
Directly map each ExternAbi variant to its string and back again. This has a few advantages: - By making the ABIs compare equal to their strings, we can easily lexicographically sort them and use that sorted slice at runtime. - We no longer need a workaround to make sure the hashes remain stable, as they already naturally are (by being the hashes of unique strings). - The compiler can carry around less &str wide pointers
These were a way to ensure hashes were stable over time for ExternAbi, but simply hashing the strings is more stable in the face of changes. As a result, we can do away with them.
These can be entirely replaced by the FromStr implementation.
|
Also I'm fine with just yeeting the complexity, the thing is just that I only really noticed it was just two tests after I fixed up everything. |
This comment has been minimized.
This comment has been minimized.
workingjubilee
force-pushed
the
stabilize-externabi-hashing-forever
branch
from
2150c83 to
f8570e8
Compare
workingjubilee
removed
the
A-query-system
|
@bors r+ rollup |
bors
added
S-waiting-on-bors
…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#136758 (tests: `-Copt-level=3` instead of `-O` in assembly tests) - rust-lang#136761 (tests: `-Copt-level=3` instead of `-O` in codegen tests) - rust-lang#136784 (Nuke `Buffer` abstraction from `librustdoc`, take 2 💣) - rust-lang#136838 (Check whole `Unsize` predicate for escaping bound vars) - rust-lang#136848 (add docs and ut for bootstrap util cache) - rust-lang#136871 (dev-guide: Link to `t-lang` procedures for new features) - rust-lang#136890 (Change swap_nonoverlapping from lang to library UB) - rust-lang#136901 (compiler: give `ExternAbi` truly stable `Hash` and `Ord`) - rust-lang#136907 (compiler: Make middle errors `pub(crate)` and bury the dead code) - rust-lang#136916 (use cc archiver as default in `cc2ar`) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#136901 - workingjubilee:stabilize-externabi-hashing-forever, r=compiler-errors compiler: give `ExternAbi` truly stable `Hash` and `Ord` Currently, `ExternAbi` has a bunch of code to handle the reality that, as an enum, adding more variants to it will risk it hashing differently. It forces all of those variants to be added in a fixed order, except this means that the order of the variants doesn't correspond to any logical order except "historical accident". This is all to avoid having to rebless two tests. Perhaps there were more, once upon a time? But then we invented normalization in our test suite to handle exactly this sort of issue in a more general way. There are two options here: - Get rid of all the logical overhead and shrug, embracing blessing a couple of tests sometimes - Change `ExternAbi` to have an ordering and hash that doesn't depend on the number of variants As `ExternAbi` is essentially a strongly-typed string, and thus no two strings can be identical, this implements the second of the two by hand-implementing `Ord` and `Hash` to make the hashing and comparison based on the string! This will diff the current hashes, but they will diff no more after this.
|
@rust-timer build fe90d7e |
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (fe90d7e): comparison URL. Overall result: ❌ regressions - please read the text belowBenchmarking 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 the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 4.0%, secondary 5.2%)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 (primary 1.2%, secondary 2.6%)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 sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 788.874s -> 788.126s (-0.09%) |
|
FYI, this PR caused a tiny performance regression on a bunch of secondary benchmarks in #136943. But I don't think that it warrants further action. |
|
@rustbot label: +perf-regression-triaged |
rustbot
added
the
perf-regression-triaged
Currently,
ExternAbihas a bunch of code to handle the reality that, as an enum, adding more variants to it will risk it hashing differently. It forces all of those variants to be added in a fixed order, except this means that the order of the variants doesn't correspond to any logical order except "historical accident". This is all to avoid having to rebless two tests. Perhaps there were more, once upon a time? But then we invented normalization in our test suite to handle exactly this sort of issue in a more general way.There are two options here:
ExternAbito have an ordering and hash that doesn't depend on the number of variantsAs
ExternAbiis essentially a strongly-typed string, and thus no two strings can be identical, this implements the second of the two by hand-implementingOrdandHashto make the hashing and comparison based on the string! This will diff the current hashes, but they will diff no more after this.