Remove custom doubling strategy + add examples to VecAllocEx
#9058
Conversation
| /// // push more data into the vec | ||
| /// for _ in 0..10 { vec.push_accounted(1, &mut allocated); } | ||
| /// assert_eq!(allocated, 64); // underlying vec has space for 10 u32s | ||
| /// assert_eq!(vec.allocated_size(), 64); |
There was a problem hiding this comment.
The allocated size was different than what was obtained by push_accounted prior to this change
|
|
||
| // growth factor: 2, but at least 2 elements | ||
| let bump_elements = (self.capacity() * 2).max(2); | ||
| let bump_size = std::mem::size_of::<u32>() * bump_elements; |
There was a problem hiding this comment.
I think this should be sizeof<T> rather than sizeof<u32> but perhaps I am missing something
There was a problem hiding this comment.
looks correct. I suspect this was the cause of the discrepancy in the original code
alamb
changed the title
VecAllocEx
| let new_capacity = self.capacity(); | ||
| if new_capacity > prev_capacty { | ||
| // capacity changed, so we allocated more | ||
| let bump_size = (new_capacity - prev_capacty) * std::mem::size_of::<T>(); |
There was a problem hiding this comment.
does it make sense to do a checked_mul here similar to the checked_add below?
There was a problem hiding this comment.
either that or remove the checked_add because push would panic first now that it's called first instead of at the end
There was a problem hiding this comment.
thinking about it more: if accounting is tracking multiple vecs, then this makes sense. multiplication should never overflow because push would panic first in that case, but the checked_add could potentially overflow since accounting could be some value greater than capacity.
There was a problem hiding this comment.
I think that is a good point. I will add a comment to clarify
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you for the review @kallisti-dev
| let new_capacity = self.capacity(); | ||
| if new_capacity > prev_capacty { | ||
| // capacity changed, so we allocated more | ||
| let bump_size = (new_capacity - prev_capacty) * std::mem::size_of::<T>(); |
There was a problem hiding this comment.
I think that is a good point. I will add a comment to clarify
3 participants
Which issue does this PR close?
Closes #9057
Rationale for this change
While writing documentation as suggested by @crepererum on #9025 (comment) I found that the accounting with
push_accountedandallocated_sizewere different and inconsistentWhat changes are included in this PR?
Are these changes tested?
Yes, with doc tests
Are there any user-facing changes?
Allocation accounting is now different