splitstream: Drop an unsafe #96
Merged
Conversation
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
This one didn't have a `SAFETY` comment; looking through git log here I do see: > It no longer does internal buffer allocations when reading tar headers, saving > another ~10% off the total time to print a merged dumpfile. However, it looks like that was going from allocation-per-read to "read into a global buffer", which we're still doing here. I didn't measure performance here, but I think we have other performance worries and it's not worth carrying an `unsafe` for this. In very hot I/O paths one *can* measure the impact of zero-initializing, but I'm doubtful of that here. Just looking at the bigger picture here I think instead of a generic `R: Read` we should take a `BufReader` and then use `fill_buf` on it, which is intended for this use case. Anyways for now just drop the `unsafe`. Also add tests. Signed-off-by: Colin Walters <[email protected]>
allisonkarlitskaya
approved these changes
Apr 7, 2025
There was a problem hiding this comment.
I agree that I'd prefer to drop the unsafe here, even if it costs us a bit.
Rustix has a really cool new buffer approach that I'd love to use here, but unfortunately it's not for the Read trait. It seems like Rust is still having trouble making up its mind about what to do here: rust-lang/rust#117693
Thanks for the tests!
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 one didn't have a
SAFETYcomment; looking through git log here I do see:However, it looks like that was going from allocation-per-read to "read into a global buffer", which we're still doing here. I didn't measure performance here, but I think
we have other performance worries and it's not worth carrying an
unsafefor this.In very hot I/O paths one can measure the impact of zero-initializing, but I'm doubtful of that here.
Just looking at the bigger picture here I think
instead of a generic
R: Readwe should takea
BufReaderand then usefill_bufon it,which is intended for this use case.
Anyways for now just drop the
unsafe.Also add tests.