Introduce ArcVec<T> to improve Copy performance of messages#2015
Introduce ArcVec<T> to improve Copy performance of messages#2015muhamadazmy wants to merge 1 commit intorestatedev:mainfrom
Conversation
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
Thanks for creating this PR @muhamadazmy. The wrapper makes sense to me. The one thing I would like to understand is whether there is a measurable difference between Arc<Vec<T>> and Arc<[T]> since it seems that converting a Vec<T> into a Arc<[T]> requires another allocation.
| /// Internally it keeps the data inside an [`Arc<[T]>`] | ||
| #[derive(Debug)] | ||
| pub struct ArcVec<T> { | ||
| inner: Arc<[T]>, |
There was a problem hiding this comment.
Is there a benefit over Arc<Vec<T>>? If I understand the API correctly, then converting from Vec<T> into an Arc<[T]> requires an extra allocation whereas the former adds another level of pointer indirection.
There was a problem hiding this comment.
The only reason that I used Arc<[T]> instead of Arc<Vec<T>> is because Arc<[T]> is very common type in Bifrost.
In Bifrost the Loglet::enqueue_batch method accepts an Arc<[Record]> as a param, but when we actually need to send this over the wire (in case of replicated loglet client, or to log servers), the message data types are using Vec<Record> type. This requires copying all records to build, which can be quite heavy in case we sending the same set of records to multiple log servers for example.
In other wards, the most common usage patter is to build an ArcVec from an Arc<[Record]> not from Vec.
That being said, I should actually Arc<[T]>::from(vec) instead of from_iter to build the ArcVec from a Vec (in case of deseralization) to avoid one extra allocation. as per here
There was a problem hiding this comment.
Thanks for the explanation.
I think that Arc<[T]>::from(vec) will also require one additional allocation.
Once this becomes a problem and measurable, we can revisit it.
Summary: The ArcVec<T> is just a thin wrapper around Arc<[T]> that then can be cheaply cloned. But at the same time can be seralized and most importantly deserialized when sent over the wire
tillrohrmann
left a comment
tillrohrmann
left a comment
There was a problem hiding this comment.
LGTM. +1 for merging.
| /// Internally it keeps the data inside an [`Arc<[T]>`] | ||
| #[derive(Debug)] | ||
| pub struct ArcVec<T> { | ||
| inner: Arc<[T]>, |
There was a problem hiding this comment.
Thanks for the explanation.
I think that Arc<[T]>::from(vec) will also require one additional allocation.
Once this becomes a problem and measurable, we can revisit it.
| let mut vec: Vec<T> = Vec::with_capacity(seq.size_hint().unwrap_or_default()); | ||
| while let Some(value) = seq.next_element()? { | ||
| vec.push(value); | ||
| } | ||
|
|
||
| Ok(vec.into()) |
There was a problem hiding this comment.
Once rust-lang/rust#129401 lands with Rust 1.82, we can avoid the extra allocation of a Vec here and directly produce into the Arc<[T]> slice.
|
I don't think we need this wrapper if we enable |
|
@muhamadazmy If you want to give this a try and confirm it, we can close this PR in favor of |
|
@AhmedSoliman thanks for your review. I didn't know I will close this PR now |
3 participants
Introduce ArcVec to improve Copy performance of messages
Summary:
The ArcVec is just a thin wrapper around Arc<[T]> that then
can be cheaply cloned. But at the same time can be seralized
and most importantly deserialized when sent over the wire
Stack created with Sapling. Best reviewed with ReviewStack.