808 Checkpoint/Restart collection

[lifflander]

Fixes #808

Has a test, working.

[codecov]

Codecov Report

Merging #809 into develop will increase coverage by 0.34%.
The diff coverage is 95.75%.

@@             Coverage Diff             @@
##           develop     #809      +/-   ##
===========================================
+ Coverage    80.07%   80.41%   +0.34%     
===========================================
  Files          343      347       +4     
  Lines        10724    10933     +209     
===========================================
+ Hits          8587     8792     +205     
- Misses        2137     2141       +4     

Impacted Files	Coverage Δ
src/vt/vrt/collection/manager.impl.h	85.24% <88.88%> (+1.22%)	⬆️
src/vt/vrt/collection/collection_directory.h	100.00% <100.00%> (ø)
src/vt/vrt/collection/staged_token/token.h	100.00% <100.00%> (ø)
tests/unit/collection/test_checkpoint.cc	100.00% <100.00%> (ø)
src/vt/vrt/collection/staged_token/token.impl.h	100.00% <0.00%> (ø)
src/vt/topos/mapping/dense/dense.impl.h	98.21% <0.00%> (+0.06%)	⬆️

[PhilMiller]

This generally looks good, and pretty straightforward.

Something we may want to consider in checkpointToFile is putting the serialized size in the first sizeof(size_t) bytes, before the payload, so that we can compare that to the stat'd size of the file that we read in restartFromFile

[PhilMiller]

This does make an assumption of the array being dense in its index space.

Looking ahead a little bit, will that be a problem if we wanted to use this in NimbleSM? Does that even care about preserving a collection instance across a hypothetical checkpoint?

[PhilMiller]

As we look toward large-scale runs, we're going to have a lot of objects. We may want to create subdirectories to hold limited numbers of objects each. I was going to suggest per-index-component, but that falls over if one of the index components is simply process rank, since even that gets too big. Instead, maybe we should hash the indices, and use a few bytes from the hash to set subdirectories?

[lifflander]

This does make an assumption of the array being dense in its index space.

Looking ahead a little bit, will that be a problem if we wanted to use this in NimbleSM? Does that even care about preserving a collection instance across a hypothetical checkpoint?

I can fix that easily by just not failing if the file is not found during restart---assume it never existed.

[PhilMiller]

This does make an assumption of the array being dense in its index space.
Looking ahead a little bit, will that be a problem if we wanted to use this in NimbleSM? Does that even care about preserving a collection instance across a hypothetical checkpoint?

I can fix that easily by just not failing if the file is not found during restart---assume it never existed.

That seems a bit risky - How about each process recording the objects it knows it's the home process for, in an 'indices.$node' file?

[lifflander]

@PhilMiller I've addressed all your concerns and written a test for it!

[lifflander]

Here is an overview of what got changed by this pull request:

Clones added
============
- tests/unit/collection/test_checkpoint.cc  2
         

See the complete overview on Codacy

[PhilMiller]

All lgtm

[lifflander]

@PhilMiller Good for beta.9?

[PhilMiller]

Yeah, good for the stable branch

…

On Fri, Jun 5, 2020, 17:00 Jonathan Lifflander ***@***.***> wrote: @PhilMiller <https://github.com/PhilMiller> Good for beta.9? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#809 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA64AD6HLYK6J7BXCPRZXDRVFMGPANCNFSM4M5R2W3A> .