timely-util: implement an accounting container builder #27811
Conversation
| let local = Vec::with_capacity((capacity + Self::CHUNK - 1) / Self::CHUNK); | ||
| let chunks = (capacity + Self::CHUNK - 1) / Self::CHUNK; | ||
| let mut local = vec![]; | ||
| local.resize_with(chunks, || Array::with_capacity(Self::CHUNK)); |
There was a problem hiding this comment.
@antiguru the code here was not actually reserving the size in the arrays so I'm doing that here. I have updated the push method to look at capacity to decide whether a new chunk is needed. This allows us to also reserve space from SizableContainer::reserve
There was a problem hiding this comment.
It was somewhat intentional to not preallocate the arrays. The cost of getting an array right now vs later should be the same, and getting it later opens up the opportunity to recycle allocations more eagerly.
There was a problem hiding this comment.
hm, that's surprising! Isn't the purpose of a with_capacity API to ensure that I can insert X items without extra allocations? I can certainly revert this and make the reserve function a no-op (i.e just add capacity to hold the number of chunks needed) but that seems like the wrong implementation.
Regarding cost, while there are no reallocations of the actual data here there is potentially temporal cost because work that can be done now is deferred for the future and now might be a more convenient time.
In general, with a with_capacity API all these considerations can be thought of at the calling site by using a capacity of 0 or the actual value depending on when they want the allocations to happen.
There was a problem hiding this comment.
I would say it's because with_capacity is a under defined API. We have to support it as it's part of the interface, and at the same time this implementation knows more about itself than the caller. At the same time, it might be the best we can offer.
I think it's very important that we do not preallocate: imagine we're forming a new largest batch in a spine, and constructing it takes a long time. We want to minimize the resource footprint during construction to avoid running out of resources. Additionally, differential chooses a capacity of the sum of input sizes, because it needs to assume that nothing will cancel. Often, we end with less than the sum, and we don't want to be in a position to carry unused memory with us, or needing to reallocate.
There was a problem hiding this comment.
We want to minimize the resource footprint during construction to avoid running out of resources
Additionally, differential chooses a capacity of the sum of input sizes
I'm still a bit confused because these two seem contradictory. If DD doesn't want preallocation to happen why call with_capacity with a large value? It sounds like DD needs to use with_capacity(0) and everything will work out fine when with_capacity does what it says on the tin. What am I missing?
There was a problem hiding this comment.
I reverted to the previous capacity behavior to unblock this PR but something doesn't feel right with how DD is using this API
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
Signed-off-by: Petros Angelatos <petrosagg@gmail.com>
petrosagg
force-pushed
the
handle-accounting
branch
from
fb5858a
to
94ae1eb
Compare
| } | ||
|
|
||
| fn preferred_capacity() -> usize { | ||
| if ENABLE_CHUNKED_STACK.load(std::sync::atomic::Ordering::Relaxed) { |
There was a problem hiding this comment.
You or I should check whether the two branches return different values. I think they should not because changing the preferred capacity at run time is a case I didn't think about and can cause undesired effects.
There was a problem hiding this comment.
They both end up in timely_container::buffer::preferred_capacity
There was a problem hiding this comment.
Sorry, I've intended to make this a stronger statement: It should just call timely::container::buffer::preferred_capacity to protect against someone changing the preferred capacity and suddenly things starting to crash/oom. preferred_capacity should probably be a const fn.
There was a problem hiding this comment.
No worries, I will put up a new PR to fix this
Motivation
This PR adds a container builder wrapper that keeps track of the heap allocated bytes produced so far. This information can be used by operators to decide when it is a good time to yield.
Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.