Skip to content
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

Is std::mem::size_for_val the recommended way to calculate the total allocation for SerializedValues? #826

Closed
Jasperav opened this issue Oct 10, 2023 · 2 comments
Assignees
Labels
question Further information is requested

Comments

@Jasperav
Copy link
Contributor

I upgraded Catalytic to the latest version of this driver (0.10) because I saw there where performance improvements by using size_for_val here: #809. Catalytic now also uses them, e.g.: https://github.com/Jasperav/Catalytic/blob/4f9babb197c865819ef5f7537685873f97ac8037/catalytic_table_to_struct/example/src/generated/child.rs#L174

Is this the best way now or am I missing something? I see that there are now new traits coming but that can take awhile: #463

@mykaul mykaul added the question Further information is requested label Oct 11, 2023
@piodul
Copy link
Collaborator

piodul commented Oct 11, 2023

The PR you linked introduces a heuristic, i.e. it sums the sizes of Rust representations of all the columns in tuples/Vec/slice using std::mem::size_of_val. The std::mem::size_of_val function will sometimes correspond to the serialized CQL size of a value, but not always - for example, if you have a Vec column type it will always be counted as 3 * size_of_val::<usize>(), regardless from the number of its elements.

Calculating the exact serialized size up front sometimes requires more complicated calculations, so it would require extending the ValueList/Value trait. We'd rather add this functionality to the new traits instead of investing more time in the old ones.

To sum up - this is a heuristic which should reduce the current number of reallocations; it's possible to do better, but it requires more changes.

@Lorak-mmk Lorak-mmk self-assigned this Nov 15, 2023
@Lorak-mmk
Copy link
Collaborator

closing in favor of #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants