Refactor allocator strategy #421
Conversation
|
What's the motivation for handling allocation ourselves instead of reusing what |
With these changes it has been achieved:
|
|
@seanmonstar In the benchmarks, https://github.com/botika/buf-min/tree/e62a7975ed7ea00c473902ae774af537c0374949/benches, you can see the difference. Just change the byte version in |
|
The difference is very cool! Would you be able to explain the differences proposed here, vs what |
|
|
@botika can you comment this? Should be vector be shrinked? |
| NonNull::dangling() | ||
| } else { | ||
| check_capacity_overflow(capacity); | ||
| unsafe { |
There was a problem hiding this comment.
If this function (alloc_buf) is going to be safe to call, it might be worth having a comment here explaining why this is always safe with any value of capacity.
| @@ -1476,6 +1530,7 @@ impl PartialEq<Bytes> for BytesMut { | |||
| } | |||
| } | |||
|
|
|||
| #[inline] | |||
There was a problem hiding this comment.
I'm curious to know whether or not adding inline hints to these internal functions has a noticeable result on performance — in general, I expect #[inline] (not #[inline(always)]) to only be meaningful for pub functions, since it enables cross-crate inlining, and since it's a hint, rustc is pretty free to ignore it...
| } | ||
|
|
||
| #[inline] | ||
| fn non_null(ptr: *mut u8, layout: Layout) -> NonNull<u8> { |
There was a problem hiding this comment.
It might be worth having a comment here (and on the already-present vptr function?) explaining when they're supposed to be used. It looks like vptr is called to construct a NonNull from a raw ptr which is assumed to be an already valid pointer to a Vec, while this function is called to construct a NonNull from a raw ptr which was returned by the allocator? But, I am not sure if this distinction is clear on the first read-through.
There was a problem hiding this comment.
Is this result, https://github.com/rust-lang/rust/blob/8ad7bc3f428300aee6764f6e23527e19eb235e81/src/liballoc/alloc.rs#L177.
In vptr, this result has already been checked since it comes from a Vec.
| /// No cost BytesMut to Bytes converter | ||
| /// | ||
| /// # Safety | ||
| /// Make sure `bytes.kind()` is `KIND_VEC` |
There was a problem hiding this comment.
can we add an assertion (or at least a debug assertion?) in this function before performing the conversion?
There was a problem hiding this comment.
I agree, but .kind() is private, make it pub(crate)?
https://github.com/tokio-rs/bytes/blob/master/src/bytes_mut.rs#L841-L844
src/bytes.rs
Outdated
| if ptr as usize & KIND_MASK == 0 { | ||
| let data = ptr as usize | KIND_VEC; | ||
| Bytes { | ||
| ptr: off_ptr, | ||
| len, | ||
| data: AtomicPtr::new(data as *mut _), | ||
| vtable: &PROMOTABLE_EVEN_VTABLE, | ||
| } | ||
| } else { | ||
| Bytes { | ||
| ptr: off_ptr, | ||
| len, | ||
| data: AtomicPtr::new(ptr as *mut _), | ||
| vtable: &PROMOTABLE_ODD_VTABLE, | ||
| } | ||
| } |
There was a problem hiding this comment.
style nit, take it or leave it: this might be a little more readable as
| if ptr as usize & KIND_MASK == 0 { | |
| let data = ptr as usize | KIND_VEC; | |
| Bytes { | |
| ptr: off_ptr, | |
| len, | |
| data: AtomicPtr::new(data as *mut _), | |
| vtable: &PROMOTABLE_EVEN_VTABLE, | |
| } | |
| } else { | |
| Bytes { | |
| ptr: off_ptr, | |
| len, | |
| data: AtomicPtr::new(ptr as *mut _), | |
| vtable: &PROMOTABLE_ODD_VTABLE, | |
| } | |
| } | |
| let (data, vtable) = if ptr as usize & KIND_MASK == 0 { | |
| (ptr as usize | KIND_VEC as *mut _, &PROMOTABLE_EVEN_VTABLE) | |
| } else { | |
| (ptr, &PROMOTABLE_ODD_VTABLE) | |
| }; | |
| Bytes { | |
| ptr: off_ptr, | |
| len, | |
| data: AtomicPtr::new(data), | |
| vtable, | |
| } |
|
@fanatid I don't see any capacity in the free call, I do not know what he's talking about. |
|
When running the following program with miri, it fails due to the memory being deallocated with the wrong capacity. [package]
name = "dealloc-test"
version = "0.1.0"
authors = ["Alice Ryhl <[email protected]>"]
edition = "2018"
[dependencies]
bytes = { git = "https://github.com/botika/bytes", rev = "alloc" }use bytes::BytesMut;
fn main() {
let mut b = BytesMut::with_capacity(16);
b.extend_from_slice(&[0; 8]);
let frozen = b.freeze();
drop(frozen);
} |
|
@Darksonn thank you. I created small example on playground with Vec which illustrate this problem https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=19802f08ea7fdc58b341dae3aa5efdfd |
|
@Darksonn That is, we should pass the |
|
I am not very familiar with the internals of |
|
Okay, everything should be solved. Comments? |
|
I think we might still want to make the changes to improve Another potential solution we could consider is implementing some kind of heuristic for whether or not to deallocate in |
|
I closed it because |
Closes #419
Closes #401
It is just the main idea. But before continuing I would like some comment.Hello World benchmark
Old
New