Snapshot diff merge operations #142
Conversation
Shillaker
changed the title
Shillaker
changed the title
src/util/snapshot.cpp
Outdated
| bool isInMergeRegion = | ||
| mergeIt != mergeRegions.end() && | ||
| offset >= mergeIt->second.offset && | ||
| offset <= (mergeIt->second.offset + mergeIt->second.length); |
There was a problem hiding this comment.
Minor point here, I think this should be a < not a <=.
If we have a merge region starting at offset 0 with length 3, we want to mark bytes 0,1,2 as dirty and belonging to this merge region. Byte 3 (i.e. 0 + 3) should be out of the region if I am not mistaken.
There was a problem hiding this comment.
Actually, after having gone through the code, afaict we will only have the case in which offset == mergeIt->second.offset as then we move the offset by region.length.
Maybe we could add an assertion after line 83 like:
if (isDirtyByte && isInMergeRegion) {
assert(offset == mergeIt->second.offset);
...| diffs.emplace_back(diff); | ||
|
|
||
| // Bump the loop variable to the end of this region (note that | ||
| // the loop itself will increment onto the next) |
There was a problem hiding this comment.
Again, I think that offset + length should point to the first byte after the region, and then the for increment would skip one byte.
A test that would check this would consist of two consecutive regions with ints that need to be added. If I am not mistaken that test should now fail.
There was a problem hiding this comment.
Yes, great spot on both of these comments. There was an off-by-one error that meant that the second of two adjacent merge regions would be lost if both had changed. Have added the relevant -1s and a test.
| faabric::util::SnapshotData snap = takeSnapshot(snapKey, 5, false); | ||
|
|
||
| // Set up a couple of ints in the snapshot | ||
| int offsetA1 = 5; |
There was a problem hiding this comment.
As mentioned earlier, could we have a test where one diff is at offset x and the other one at offset x + sizeof(int)?
| TEST_CASE_METHOD(SnapshotTestFixture, | ||
| "Test edge-cases of snapshot merge regions", | ||
| "[util]") | ||
| { |
There was a problem hiding this comment.
@csegarragonz here's the test that covers the edge cases
The default merge strategy for snapshot diffs is to overwrite the master with the diffs. This works if each distributed function is writing to different offsets (for example, if each function is writing to a different index in an array), but if they are writing to the same offset, the merging will lose information by overwriting each time.
This PR adds merge operations for summation, subtraction, multiplication, min and max, but this list could be extended in future, primarily focused on in-place reductions.
Applications can set these merge operations at byte-level granularity, e.g. specifying that a 4-byte region at offset
Xshould be summed as an integer when merging snapshots.At the moment I've only included integer operations, as these are all that's needed for the current use-case, but again, this could be extended in future.