Chore: TODO #5049
Closed
Chore: TODO #5049
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
connortsui20
changed the title
extend impls for BufferMutextend impls for BufferMut
connortsui20
force-pushed
the
ct/buffer-mut-extend
branch
from
8d2f9f9 to
e04bb7f
Compare
connortsui20
commented
Oct 23, 2025
| // we know that `add` will not overflow. | ||
| unsafe { dst = dst.add(1) }; | ||
| }); | ||
| // TODO(joe): replace with ptr_sub when stable |
Contributor
Author
There was a problem hiding this comment.
It was renamed to offset_from_unsigned
Contributor
Benchmarks: Random AccessSummary
|
CodSpeed Performance ReportMerging #5049 will degrade performances by 20.36%Comparing Summary
Benchmarks breakdown
Footnotes |
Codecov Report❌ Patch coverage is
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Benchmarks: TPC-H SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb NVMeSummary
Detailed Results Table
|
Contributor
Benchmarks: FineWeb S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=1 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: TPC-DS SF=1 on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: GitHub Archive (NVMe)Summary
Detailed Results Table
|
Contributor
Benchmarks: CompressionSummary
Detailed Results Table
|
Contributor
Benchmarks: GitHub Archive (S3)Summary
Detailed Results Table
|
Contributor
Benchmarks: TPC-H SF=10 on S3Summary
Detailed Results Table
|
Contributor
Benchmarks: Clickbench on NVMESummary
Detailed Results Table
|
Contributor
Benchmarks: Statistical and Population GeneticsSummary
Detailed Results Table
|
connortsui20
force-pushed
the
ct/buffer-mut-extend
branch
from
e04bb7f to
0246ae0
Compare
Contributor
Author
|
Actually I'm realizing this is kind of stupid. We might as well just case on if we have an upper bound in the normal Edit: nvm Rust does not guarantee those hints are correct |
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/buffer-mut-extend
branch
from
0246ae0 to
052df43
Compare
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
It incorrectly assumed that the upper bound for iterators is true. Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
connortsui20
force-pushed
the
ct/buffer-mut-extend
branch
from
2d950f1 to
dbe1155
Compare
Contributor
|
We need to be careful. We build on stable compiler, which means that we don't get See #3881 for the PR that added our own TrustedLen and the |
gatesn
reviewed
Oct 23, 2025
Contributor
gatesn
left a comment
gatesn
left a comment
There was a problem hiding this comment.
In general, your unsafe functions should have more debug_asserts. e.g. set_len
| let bit_in_byte = bit_pos % 8; | ||
|
|
||
| // Ensure buffer has enough bytes | ||
| // Ensure buffer has enough bytes. |
| // Bit is already 0 if we just pushed a new byte, otherwise ensure it's unset | ||
| // Bit is already 0 if we just pushed a new byte, otherwise ensure it's unset. | ||
| if bit_in_byte != 0 { | ||
| self.buffer.as_mut_slice()[byte_pos] &= !(1 << bit_in_byte); |
Contributor
There was a problem hiding this comment.
Use crate::bit::set_bit instead
| } | ||
|
|
||
| // Set the bit. | ||
| self.buffer.as_mut_slice()[byte_pos] |= 1 << bit_in_byte; |
Contributor
There was a problem hiding this comment.
Same here, use the set function
| let byte_pos = bit_pos / 8; | ||
| let bit_in_byte = bit_pos % 8; | ||
|
|
||
| // Ensure buffer has enough bytes. |
Contributor
There was a problem hiding this comment.
I thought the caller asserted this already. So this statement should never be true, i.e. you can delete it.
Contributor
Author
There was a problem hiding this comment.
What is the semantics of with_capacity though? Tracing through I can't figure out the answer to this question: are all bytes guaranteed to be zeroed? If not then I think we might need to do this
| let bit_in_byte = bit_pos % 8; | ||
|
|
||
| // Ensure buffer has enough bytes. | ||
| if byte_pos >= self.buffer.len() { |
| unsafe { dst.write(item) }; | ||
|
|
||
| // Note: We used to have `dst.add(iteration).write(item)`, here. However this was much | ||
| // slower than just incrementing `dst`. |
Contributor
There was a problem hiding this comment.
Was it really slower? Still doing one addition per loop
Contributor
Author
There was a problem hiding this comment.
Not sure, that comment was there before.
Contributor
There was a problem hiding this comment.
it was weirdly slower, somehow it was not optimised to the same thing
Contributor
|
@a10y even with the trick we employ here? Basically reserve the lower bound of items, then consume the iterator up to the known capacity, after which, we loop over items using checked appends. Seems like if the iterator was emitting a correct bound, then we'd never hit the slower loop portion? My understanding of the min-specialization uses is that e.g. extend(slice::iter) can be implemented using a memcpy, since we can specialize and know that the iterator is just backed by a slice. |
Deploying vortex-bench with
|
| Latest commit: |
052df43
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0b5a9292.vortex-93b.pages.dev |
| Branch Preview URL: | https://ct-clean-extend.vortex-93b.pages.dev |
Contributor
Author
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.
I got tripped up when reading this and misunderstood how this worked so I just decided to clean it up
Edit: now this PR is also going to attempt to remove
TrustedLeniterator