Remove sip::Hasher::short_write.#69471
Merged
bors merged 1 commit intorust-lang:masterfrom Mar 15, 2020
Merged
Conversation
`sip::Hasher::short_write` is currently unused. It is called by
`sip::Hasher::write_{u8,usize}`, but those methods are also unused,
because `DefaultHasher`, `SipHasher` and `SipHasher13` don't implement
any of the `write_xyz` methods, so all their write operations end up
calling `sip::Hasher::write`.
(I confirmed this by inserting a `panic!` in `sip::Hasher::short_write`
and running the tests -- they all passed.)
The alternative would be to add all the missing `write_xyz` methods.
This does give some significant speed-ups, but it hurts compile times a
little in some cases. See rust-lang#69152 for details. This commit does the
conservative thing and doesn't change existing behaviour.
rust-highfive
added
the
S-waiting-on-review
Contributor
Author
|
This should have zero perf impact because it just removes dead code. But I was surprised by the perf impact of #69152 so I will check. @bors try @rust-timer queue |
Collaborator
|
Awaiting bors try build completion |
Collaborator
bors
added a commit
that referenced
this pull request
Feb 26, 2020
Remove `sip::Hasher::short_write`.
`sip::Hasher::short_write` is currently unused. It is called by
`sip::Hasher::write_{u8,usize}`, but those methods are also unused,
because `DefaultHasher`, `SipHasher` and `SipHasher13` don't implement
any of the `write_xyz` methods, so all their write operations end up
calling `sip::Hasher::write`.
(I confirmed this by inserting a `panic!` in `sip::Hasher::short_write`
and running the tests -- they all passed.)
The alternative would be to add all the missing `write_xyz` methods.
This does give some significant speed-ups, but it hurts compile times a
little in some cases. See #69152 for details. This commit does the
conservative thing and doesn't change existing behaviour.
r? @rust-lang/libs
Collaborator
|
☀️ Try build successful - checks-azure |
Collaborator
|
Queued ba8f508 with parent 6fd8798, future comparison URL. |
Collaborator
|
Finished benchmarking try commit ba8f508, comparison URL. |
Contributor
Author
|
The performance effect is right on the edge of "noise" and "miniscule improvement", more or less as expected. |
Member
|
Let's try r? @dtolnay perhaps? |
dtolnay
approved these changes
Mar 12, 2020
Member
dtolnay
left a comment
dtolnay
left a comment
There was a problem hiding this comment.
Regarding #69152 (comment), I agree that this is the right tradeoff. Thanks!
Member
|
@bors r+ |
Collaborator
|
📌 Commit 54d1c50 has been approved by |
bors
added
S-waiting-on-bors
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 14, 2020
…te, r=dtolnay
Remove `sip::Hasher::short_write`.
`sip::Hasher::short_write` is currently unused. It is called by
`sip::Hasher::write_{u8,usize}`, but those methods are also unused,
because `DefaultHasher`, `SipHasher` and `SipHasher13` don't implement
any of the `write_xyz` methods, so all their write operations end up
calling `sip::Hasher::write`.
(I confirmed this by inserting a `panic!` in `sip::Hasher::short_write`
and running the tests -- they all passed.)
The alternative would be to add all the missing `write_xyz` methods.
This does give some significant speed-ups, but it hurts compile times a
little in some cases. See rust-lang#69152 for details. This commit does the
conservative thing and doesn't change existing behaviour.
r? @rust-lang/libs
Dylan-DPC-zz
pushed a commit
to Dylan-DPC-zz/rust
that referenced
this pull request
Mar 14, 2020
…te, r=dtolnay
Remove `sip::Hasher::short_write`.
`sip::Hasher::short_write` is currently unused. It is called by
`sip::Hasher::write_{u8,usize}`, but those methods are also unused,
because `DefaultHasher`, `SipHasher` and `SipHasher13` don't implement
any of the `write_xyz` methods, so all their write operations end up
calling `sip::Hasher::write`.
(I confirmed this by inserting a `panic!` in `sip::Hasher::short_write`
and running the tests -- they all passed.)
The alternative would be to add all the missing `write_xyz` methods.
This does give some significant speed-ups, but it hurts compile times a
little in some cases. See rust-lang#69152 for details. This commit does the
conservative thing and doesn't change existing behaviour.
r? @rust-lang/libs
bors
added a commit
that referenced
this pull request
Mar 15, 2020
Rollup of 7 pull requests Successful merges: - #69357 (Emit 1-based column numbers in debuginfo) - #69471 (Remove `sip::Hasher::short_write`.) - #69498 (Change "method" to "associated function") - #69967 (Remove a few `Rc`s from RegionInferenceCtxt) - #69987 (Add self to .mailmap) - #69991 (fix E0117 message out of sync) - #69993 (Add long error explanation for E0693) Failed merges: r? @ghost
bors
added a commit
that referenced
this pull request
Mar 15, 2020
Rollup of 7 pull requests Successful merges: - #69357 (Emit 1-based column numbers in debuginfo) - #69471 (Remove `sip::Hasher::short_write`.) - #69498 (Change "method" to "associated function") - #69967 (Remove a few `Rc`s from RegionInferenceCtxt) - #69987 (Add self to .mailmap) - #69991 (fix E0117 message out of sync) - #69993 (Add long error explanation for E0693) Failed merges: r? @ghost
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
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.
sip::Hasher::short_writeis currently unused. It is called bysip::Hasher::write_{u8,usize}, but those methods are also unused,because
DefaultHasher,SipHasherandSipHasher13don't implementany of the
write_xyzmethods, so all their write operations end upcalling
sip::Hasher::write.(I confirmed this by inserting a
panic!insip::Hasher::short_writeand running the tests -- they all passed.)
The alternative would be to add all the missing
write_xyzmethods.This does give some significant speed-ups, but it hurts compile times a
little in some cases. See #69152 for details. This commit does the
conservative thing and doesn't change existing behaviour.
r? @rust-lang/libs