No StableHasherResult everywhere#64824
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
Mark-Simulacrum
force-pushed
the
no-stable-hasher-result-everywhere
branch
from
38fd34b to
64ef0f1
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
…here, r=<try>
No StableHasherResult everywhere
This removes the generic parameter on `StableHasher`, instead moving it to the call to `finish`. This has the side-effect of making all `HashStable` impls nicer, since we no longer need the verbose `<W: StableHasherResult>` that previously existed -- often forcing line wrapping.
This is done for two reasons:
* we should avoid false "generic" dependency on the result of StableHasher
* we don't need to codegen two/three copies of all the HashStable impls when they're transitively used to produce a fingerprint, u64, or u128. I haven't measured, but this might actually make our artifacts somewhat smaller too.
* Easier to understand/read/write code -- the result of the stable hasher is irrelevant when writing a hash impl.
cc @eddyb @nikomatsakis I guess? Not sure whose purview this falls under
r? @michaelwoerister due to touching incremental
|
☀️ Try build successful - checks-azure |
|
Queued 66d3e1a with parent 0b1521f, future comparison URL. |
|
Finished benchmarking try commit 66d3e1a, comparison URL. |
|
Thank you, @Mark-Simulacrum! Great find! That parameter has been basically obsolete for quite a while. r=me once benchmarking finishes and shows nothing suspicious. |
|
Looks like a slight regression for unicode-normalization (2%) - we recently landed or will land a 50% improvement there, so that's not too worrying in my eyes; I suspect that this is probably noise anyway; if hashing had gotten any slower I'd expect losses in all the other benchmarks too, but none have shown up. In any case this patch seems worth it imo. @bors r=michaelwoerister |
|
📌 Commit 64ef0f1 has been approved by |
bors
added
S-waiting-on-bors
|
That's weird. The changes should really make no difference at all... |
|
I suspect there might be slight differences in object layout and such -- e.g., previously LLVM merged two versions of some function and now doesn't need to, we only have one in the first place -- I can try and investigate if you'd like, though I can't say I expect anything too news-worthy. |
|
I don't think this is worth investigating. Thanks for the PR! |
|
Shaved ~3 MB of the uncompressed size on x86_64-unknown-linux-gnu :) |
|
Really nice! |
Rollup of 14 pull requests Successful merges: - #63492 (Remove redundancy from the implementation of C variadics.) - #64703 (Docs: slice elements are equidistant) - #64745 (Include message on tests that should panic but do not) - #64781 (Remove stray references to the old global tcx) - #64794 (Remove unused DepTrackingMap) - #64802 (Account for tail expressions when pointing at return type) - #64809 (hir: Disallow `target_feature` on constants) - #64815 (Fix div_duration() marked as stable by mistake) - #64818 (update rtpSpawn's parameters type(It's prototype has been updated in libc)) - #64830 (Thou shallt not `.abort_if_errors()`) - #64836 (Stabilize map_get_key_value feature) - #64845 (pin.rs: fix links to primitives in documentation) - #64847 (Upgrade env_logger to 0.7) - #64851 (Add mailmap entry for Dustin Bensing by request) Failed merges: - #64824 (No StableHasherResult everywhere) r? @ghost
|
☔ The latest upstream changes (presumably #64790) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
S-waiting-on-author
Mark-Simulacrum
force-pushed
the
no-stable-hasher-result-everywhere
branch
from
64ef0f1 to
14a5aef
Compare
|
@bors r=michaelwoerister |
|
📌 Commit 14a5aef has been approved by |
bors
added
S-waiting-on-bors
…sult-everywhere, r=michaelwoerister
No StableHasherResult everywhere
This removes the generic parameter on `StableHasher`, instead moving it to the call to `finish`. This has the side-effect of making all `HashStable` impls nicer, since we no longer need the verbose `<W: StableHasherResult>` that previously existed -- often forcing line wrapping.
This is done for two reasons:
* we should avoid false "generic" dependency on the result of StableHasher
* we don't need to codegen two/three copies of all the HashStable impls when they're transitively used to produce a fingerprint, u64, or u128. I haven't measured, but this might actually make our artifacts somewhat smaller too.
* Easier to understand/read/write code -- the result of the stable hasher is irrelevant when writing a hash impl.
Rollup of 5 pull requests Successful merges: - #63492 (Remove redundancy from the implementation of C variadics.) - #64589 (Differentiate AArch64 bare-metal targets between hf and non-hf.) - #64799 (Fix double panic when printing query stack during an ICE) - #64824 (No StableHasherResult everywhere) - #64884 (Add pkg-config to dependency list if building for Linux on Linux) Failed merges: r? @ghost
5 participants
This removes the generic parameter on
StableHasher, instead moving it to the call tofinish. This has the side-effect of making allHashStableimpls nicer, since we no longer need the verbose<W: StableHasherResult>that previously existed -- often forcing line wrapping.This is done for two reasons: