-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
impl FromIterator for BytesMut
exposes implementation details
#723
Comments
The fix would be to turn off specialization like this: let src = vec![1; 1024];
let from_vec = measure(|| BytesMut::from_iter(NoSpecialize(src)));
assert_eq!(from_vec, 1024); // this used to be 0
struct NoSpecialize<T>(T);
impl Iterator for NoSpecialize<vec::IntoIter<u8>> {
type Item = u8;
fn next(&mut self) -> Option<Self::Item> {
self.0.next()
}
fn size_hint(&self) -> (usize, Option<usize>) {
self.0.size_hint()
}
}
impl IntoIterator for NoSpecialize<Vec<u8>> {
type Item = u8;
type IntoIter = NoSpecialize<vec::IntoIter<u8>>;
fn into_iter(self) -> Self::IntoIter {
NoSpecialize(self.0.into_iter())
}
} |
I believe the section you're quoting is only talking about |
FromIterator
impls expose implementation detailsimpl FromIterator for BytesMut
exposes implementation details
Ah, my mistake! I've updated the issue and comments as appropriate :) |
If you agree that this is a bug, I'll happily open a PR |
What's the bug? That the compiler is able to make the copying faster? That seems like a thing we want to keep. It doesn't actually expose in the type system the internals. |
Perhaps "bug" is a bit strong. If the allocation strategy changes in future (which the comment in |
I don't think people expect a They could think it from something like a You see it like its free in the benchmark most likely because the compiler optimizes that a lot, since it is a hardcoded Try with something like this (note the use bytes::BytesMut; // 1.6.1
#[global_allocator]
static ALLOC: dhat::Alloc = dhat::Alloc; // 0.3.3
fn main() {
let _guard = dhat::Profiler::new_heap();
let src = vec![1; 1024];
let mut_from_vec = std::hint::black_box(measure(|| BytesMut::from_iter(src)));
assert_eq!(mut_from_vec, 0); // doesn't allocate!
let mut_from_array = std::hint::black_box(measure(|| BytesMut::from_iter([1; 1024])));
assert_eq!(mut_from_array, 1024);
}
fn measure<T>(f: impl FnOnce() -> T) -> u64 {
let before = dhat::HeapStats::get();
f();
let after = dhat::HeapStats::get();
after.total_bytes - before.total_bytes
} |
BytesMut
cares about keeping allocation implementation details private, which means noFrom<Vec<u8>>
and noInto<Vec<u8>>
:bytes/src/bytes_mut.rs
Lines 825 to 830 in 4e2c9c0
However, this information is leaked in the
FromIterator
impl:bytes/src/bytes_mut.rs
Lines 1409 to 1413 in f8c7b57
which go through through this specialization:
https://github.com/rust-lang/rust/blob/6c6b3027ef62e911142cfc55589baef4e9f38ec8/library/alloc/src/vec/spec_from_iter.rs#L37-L64
So collection is basically a
From<Vec<u8>>
with no overhead, which may not be intended by the authors.OTOH, this probably doesn't count as "an easy way", so maybe it's fine to leave it exposed
The text was updated successfully, but these errors were encountered: