Skip to content

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