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

handle trivially-copyable types correctly #769

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

h-2
Copy link
Contributor

@h-2 h-2 commented Nov 10, 2022

Cereal has two implementations for contiguous standard library containers:

  1. serialise element-wise (default)
  2. serialise en-bloc

The second implementation is only chosen for arithmetic element types when, in fact, it should be chosen for all trivially-copyable types; this is exactly what the trivially-copyable trait was made for 🙂

This PR changes the behaviour. It reduces the deserialisation time in one of our applications by more than 5x!

Note that the on-disk representation is not guaranteed to stay the same, although in many cases it will.

@h-2
Copy link
Contributor Author

h-2 commented Nov 11, 2022

CI failures on Ubuntu 16.04 seems unrelated.

@royjacobson
Copy link

I don't think that's a good idea, actually - trivially copyable is not strong enough. For example, types with raw pointer members and types with padding bytes are both trivially copyable.

@h-2
Copy link
Contributor Author

h-2 commented Dec 19, 2022

For example, types with raw pointer members

trivially_copyable implies for class types that they have a non-deleted trivial destructor. By any sane definition, this implies that the class is not managing the memory behind the raw pointer member. Thus, the user needs to handle this in some way, anyway.

types with padding bytes

This is true, but I don't think it's very relevant. If the total size of serialised data is very small, the impact of padding is also small (in absolute numbers). If the total size of serialised data is big, then the size effect of padding bits/bytes might be noticeable, but in that case, the performance overhead of not doing this optimisation is going to be even more noticeable––like I said, it was 5x slower in my application.

Of course, there are applications where size will be more important than speed, and that's why users can overload this for their types. But I do think that the current defaults are not well-chosen.

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.

2 participants