Skip to content

Fix condition in realloc() implementation of Vec #5

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

Merged

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Apr 5, 2023

Poolable and Realloc trait method implementations for Vec
operate on the vector itself rather than its underlying buffer.

This is why Poolable::capacity() implementation calls Vec::len()
and Realloc::realloc() implementation uses Vec::resize() rather
than Vec::reserve() or Vec::reserve_exact().

However, the condition checking whether the vector should be resized
called Vec::len() rather than Vec::capacity().
Apparently it was an attempt to call Poolable::capacity(&self),
but by default ambiguous call is resolved to Vec::capacity().

The problem is resolved by calling .len() explicitly.

Regression test and assertions failing for byte-pool 0.2.2 are added.

Poolable and Realloc trait method implementations for Vec
operate on the vector itself rather than its underlying buffer.

This is why Poolable::capacity() implementation calls Vec::len()
and Realloc::realloc() implementation uses Vec::resize() rather
than Vec::reserve() or Vec::reserve_exact().

However, the condition checking whether the vector should be resized
called Vec::len() rather than Vec::capacity().
Apparently it was an attempt to call Poolable::capacity(&self),
but by default ambiguous call is resolved to Vec::capacity().

The problem is resolved by calling .len() explicitly.

Regression test and assertions failing for byte-pool 0.2.2 are added.
@link2xt
Copy link
Collaborator Author

link2xt commented Apr 5, 2023

cc @flub @dignifiedquire

@link2xt
Copy link
Collaborator Author

link2xt commented Apr 5, 2023

This capacity calling len and implemented in a trait on top of Vec is very unfortunate:
https://github.com/dignifiedquire/byte-pool/blob/2a50848d9f7924511eda7705e493ea4c8316e23f/src/poolable.rs#L10-L13

This was noticed before (well, yesterday), but never addressed: #4

@link2xt
Copy link
Collaborator Author

link2xt commented Apr 5, 2023

Once this is merged, byte-pool 0.2.3 is needed to merge chatmail/async-imap#77 and then release a new deltachat-core-rust.

@link2xt
Copy link
Collaborator Author

link2xt commented Apr 6, 2023

@flub I pushed a second commit here as you said in chatmail/async-imap#77 (comment) that "byte-pool wanted to never have a Vec with capacity != len since that's wasteful, but at the time of writing this was not yet possible with the rust std api."
Now length is checked to be the same as capacity, which is ensured by using reserve_exact before resize.

self.reserve_exact(new_size - self.len());
debug_assert_eq!(self.capacity(), new_size);
self.resize(new_size, T::default());
debug_assert_eq!(self.len(), new_size);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this assert seems a bit redundant, but doesn't do much harm either.

@flub
Copy link
Member

flub commented Apr 6, 2023

Hum, don't you have to do the reserve_exact thing also when initially creating the block?

@link2xt
Copy link
Collaborator Author

link2xt commented Apr 6, 2023

Hum, don't you have to do the reserve_exact thing also when initially creating the block?

I have quickly checked, vec![T::default(); size] allocates exact size buffer. But even if not and it changes later, it is not a big deal, we look at len() everywhere and matching capacity to vector size is only a memory usage optimization.

@link2xt link2xt requested review from dignifiedquire and flub April 6, 2023 16:55
@link2xt link2xt merged commit 11ef606 into async-email:master Apr 12, 2023
@link2xt link2xt deleted the link2xt/vec-capacity-fully-qualified branch April 12, 2023 10:41
@link2xt link2xt mentioned this pull request Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants