-
Notifications
You must be signed in to change notification settings - Fork 562
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
chore(SharedMemory): small internal refactoring #806
Conversation
b4eb3a4
to
af62b57
Compare
af62b57
to
3f841b8
Compare
|
||
self.current_len = new_len; | ||
pub fn resize(&mut self, new_size: usize) { | ||
self.buffer.reserve(usize::max(new_size, 4 * 1024)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resize
already allocates this, actually for new_size > 4*1024
it would allocate twice.
Do you mean usize::min
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea was to reserve a minimum of 4096 bytes to avoid many mini-allocations, but I agree that if new_size > 4 * 1024
then this line is useless
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, then it should be min
not max
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not sure: suppose new_size
equals 32, if we use min
then:
assert_eq!(usize::min(32, 4*1024), 32);
and we reserve 32 more bytes instead of 4096, which is not what we want.
I don't understand if I'm missing something here from what you said
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah you're right. This should be removed because Vec
already does exponential allocations so this is redundant since we're starting already with 4*1024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be precise it should be something like okay ignore this. As @DaniPopes said .reserve(self.last_checkpoint+max(new_size,4*1024))
to allocate at least 4096
resize
doubles capacity every time so it is good to rely on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! Now it should be good.
3f841b8
to
4dc67eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
current_len
is redundant -> replaced withlast_checkpoint
with minor performance boost