#161 use reconstruction logic for std::vector

[jstrzebonski]

Fixes #161

[jstrzebonski]

In general, I took reconstruct code almost 1:1 to be able to pass reconstruct tag directly to emplace_back(). Without that I would have to do emplace_back(std::move(reconstructed_obj)) for all reconstruct cases, including tagged ctor, but if that's not a problem I think I could call dispatch::Standard::construct() directly, something like this:

template <typename T, typename VectorAllocator>
void reconstructVectorData(
  SerialSizeType const vec_size, std::vector<T, VectorAllocator>& vec) {
  auto* buf = dispatch::Standard::allocate<T>();
  for (SerialSizeType i = 0; i < vec_size; ++i) {
    auto* t = dispatch::Standard::construct<T>(buf);
    vec.emplace_back(std::move(*t));
  }
}

[jstrzebonski]

@lifflander Do you think that's (more or less) the direction we wanted to head?

[PhilMiller]

I think vec.resize(vec_size, SERIALIZE_RECONSTRUCT_TAG) is a feasible and preferable form for the case where we have the tagged reconstruction, since it avoids the intermediate allocation and move, and potentially even allows complete elision of all moves/copies if it gets inlined.

[codecov]

Codecov Report

Merging #214 (c7d41a0) into develop (50889a5) will increase coverage by 0.17%.
The diff coverage is 99.14%.

@@             Coverage Diff             @@
##           develop     #214      +/-   ##
===========================================
+ Coverage    94.14%   94.32%   +0.17%     
===========================================
  Files          100      102       +2     
  Lines         3093     3187      +94     
===========================================
+ Hits          2912     3006      +94     
  Misses         181      181              

Impacted Files	Coverage Δ
src/checkpoint/container/list_serialize.h	88.23% <85.71%> (+0.73%)	⬆️
src/checkpoint/container/map_serialize.h	93.10% <100.00%> (+0.24%)	⬆️
src/checkpoint/container/vector_serialize.h	100.00% <100.00%> (ø)
src/checkpoint/dispatch/allocator.h	100.00% <100.00%> (ø)
src/checkpoint/dispatch/reconstructor.h	79.31% <100.00%> (ø)
tests/unit/test_vector_serializer.cc	100.00% <100.00%> (ø)
src/checkpoint/serializers/base_serializer.h	100.00% <0.00%> (ø)

[lifflander]

@lifflander Do you think that's (more or less) the direction we wanted to head?

Sorry I was slow to reply. I agree with @PhilMiller that doing the resize is probably preferable for optimization.

[lifflander]

I think the approach makes sense. The only overload that might be improved by another issue is the vector case that seems very un-optimized.

[PhilMiller]

A potential refactoring, based on comments I had pointed out to me elsewhere: this case can avoid using enable_if, and just be an overload taking checkpoint::Footprinter rather than templating over Serializer at all.

[PhilMiller]

The same applies for related cases in the other files, too.

[PhilMiller]

This could/should also s.addBytes(vec.capacity()*sizeof(bool)) or s.addBytes(vec.capacity()*sizeof(bool)/CHAR_BIT).

I'm unconcerned which exactly, but I'd like something whose resulting value is O(N) so that if we or an application have code with an overgrown vector<bool>, it shows up.