Improve calculation for SerializedValues::with_capacity
#809
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
piodul
approved these changes
Sep 20, 2023
There was a problem hiding this comment.
LGTM. Thanks for the contribution!
In the next release, we are planning to introduce new serialization traits (#463) which should make it possible to calculate required size exactly - however, we will keep the old traits in parallel for some time to make it possible to gradually transition to the new traits, so your contribution will stil be useful.
Please rebase on the newest main branch, the issue with failing min_rust check is fixed there.
It seems that currently the argument provided to this function is being treated as the _number_ of items that we expect to serialize. Normally that would make sense because the argument is passed along to `Vec::with_capacity` which provisions space for the inner `serialized_values` field. However, that field is a `Vec<u8>` so really the argument corresponds to the total _size_ of the serialized values in bytes. This commit tries to be smarter about reserving space by taking into account the actual size in memory of the values to be serialized. It's not perfect (e.g., `String`s will always reserve the same amount of space, regardless of how long their actual contents are) but hopefully it will still result in fewer re-allocations.
cstyles
force-pushed
the
improve-calculation-for-with-capacity
branch
from
f1c9063 to
389c772
Compare
piodul
approved these changes
Sep 20, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It seems that currently the argument provided to this function is being treated as the number of items that we expect to serialize. Normally that would make sense because the argument is passed along to
Vec::with_capacitywhich provisions space for the innerserialized_valuesfield. However, that field is aVec<u8>so really the argument corresponds to the total size of the serialized values in bytes.This commit tries to be smarter about reserving space by taking into account the actual size in memory of the values to be serialized. It's not perfect (e.g.,
Strings will always reserve the same amount of space, regardless of how long their actual contents are) but hopefully it will still result in fewer re-allocations.Pre-review checklist
./docs/source/.Fixes:annotations to PR description.