rbx_binary: Eliminate Buffer From Interleaved Iterators #561
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
the split_at can panic, there is a checked variant if it should be handled gracefully
krakow10
changed the title
This reverts commit 030f68a.
Contributor
Author
|
I've made the executive decision to check the slice length and return an io::Error if it's too short. It's completely a possibility, and it doesn't really make sense to panic on a decode error. I also made this change to #530. |
Contributor
Author
|
I've changed to a trait-based implementation. Even though it's a bit more code, I think it's nicer, and it reduces the diff since it looks the same as the original RbxReadExt functions that it's replacing at the call site. |
Contributor
Author
|
Closing in favour of combining with #530 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
I missed a pretty obvious copy optimization here. By constructing the iterator using a slice, we can use the original buffer and avoid copying the data into an intermediate buffer. I was inspired by looking over #530. I'm not actually seeing performance improvements in my benchmarking, even when omitting the interleaved index bounds checks with unsafe. Do interleaved byte reads actually only occupy a very minor portion of the decode process?
Note: The function signature is infallible, but in fact it is possible for the provided slice to be too short, and split_at will panic, similarly to #530. This could be changed to use split_at_checked and generate an EOF io error.