From c04750f4f5e5ecc8aa8178a4759c19f80ff3a0a6 Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Mon, 18 Oct 2021 10:37:08 +0000 Subject: [PATCH 1/7] Add cross-page regions and update test --- src/util/snapshot.cpp | 23 ++++++++++++++----- tests/test/util/test_snapshot.cpp | 37 +++++++++++++++++++++++-------- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/src/util/snapshot.cpp b/src/util/snapshot.cpp index c39dfb129..2f9a38ed8 100644 --- a/src/util/snapshot.cpp +++ b/src/util/snapshot.cpp @@ -147,6 +147,10 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, isIgnore = true; break; } + case (SnapshotMergeOperation::Overwrite): { + // Default behaviour + break; + } default: { SPDLOG_ERROR( "Unhandled raw merge operation: {}", @@ -170,12 +174,21 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, diffs.emplace_back(diff); } - // Bump the loop variable to the end of this region (note that - // the loop itself will increment onto the next) - b = (region.offset - pageOffset) + (region.length - 1); + // Work out the offset to where this region ends + int regionEndOffset = + (region.offset - pageOffset) + region.length; - // Move onto the next merge region - ++mergeIt; + if (regionEndOffset <= HOST_PAGE_SIZE) { + // Bump the loop variable to the end of this region (note + // that the loop itself will increment onto the next). + b = regionEndOffset - 1; + + // Move onto the next merge region + ++mergeIt; + } else { + // End work on this page + break; + } } else if (isDirtyByte && !diffInProgress) { // Diff starts here if it's different and diff not in progress diffInProgress = true; diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index db8b88089..30a117e9b 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -311,23 +311,40 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot merge regions", "[util]") SECTION("Raw") { - dataLength = 2 * sizeof(int32_t); + // Make the raw data span several pages + dataLength = 4 * HOST_PAGE_SIZE; originalData = std::vector(dataLength, 3); updatedData = originalData; expectedData = originalData; dataType = faabric::util::SnapshotDataType::Raw; - operation = faabric::util::SnapshotMergeOperation::Ignore; + operation = faabric::util::SnapshotMergeOperation::Overwrite; SECTION("Ignore") { - // Scatter some modifications through the updated data, to make sure - // none are picked up - updatedData[0] = 1; - updatedData[sizeof(int32_t) - 2] = 1; - updatedData[sizeof(int32_t) + 10] = 1; - - expectedNumDiffs = 0; + // Add an ignore region within the snapshot which still spans + // multiple pages + int ignoreOffset = offset + 200; + int ignoreLength = dataLength - 300; + + snap.addMergeRegion(ignoreOffset, + ignoreLength, + dataType, + faabric::util::SnapshotMergeOperation::Ignore); + + // Make a modification that will _not_ be ignored + updatedData[10] = 3; + expectedData[10] = 3; + + // Scatter some modifications through the ignore region, spanning + // multiple pages + updatedData[ignoreOffset + 5] = 1; + updatedData[ignoreOffset + HOST_PAGE_SIZE + 2] = 1; + updatedData[ignoreOffset + (2 * HOST_PAGE_SIZE) + 5] = 1; + + // Make changes right at the start and end of the ignore region + updatedData[ignoreOffset] = 1; + updatedData[ignoreOffset + ignoreLength] = 1; } } @@ -395,6 +412,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test invalid snapshot merges", "[util]") SECTION("Integer overwrite") { dataType = faabric::util::SnapshotDataType::Int; + operation = faabric::util::SnapshotMergeOperation::Overwrite; dataLength = sizeof(int32_t); expectedMsg = "Unhandled integer merge operation"; } @@ -402,6 +420,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test invalid snapshot merges", "[util]") SECTION("Raw sum") { dataType = faabric::util::SnapshotDataType::Raw; + operation = faabric::util::SnapshotMergeOperation::Sum; dataLength = 123; expectedMsg = "Unhandled raw merge operation"; } From e12d5c705dbf4f54ee0d9e4de28af04118535ae9 Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Mon, 18 Oct 2021 10:48:08 +0000 Subject: [PATCH 2/7] Change to stand-alone test --- tests/test/util/test_snapshot.cpp | 85 ++++++++++++++++++++++--------- 1 file changed, 60 insertions(+), 25 deletions(-) diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index 30a117e9b..4140c0570 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -311,8 +311,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot merge regions", "[util]") SECTION("Raw") { - // Make the raw data span several pages - dataLength = 4 * HOST_PAGE_SIZE; + dataLength = 2 * sizeof(int32_t); originalData = std::vector(dataLength, 3); updatedData = originalData; expectedData = originalData; @@ -322,29 +321,15 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot merge regions", "[util]") SECTION("Ignore") { - // Add an ignore region within the snapshot which still spans - // multiple pages - int ignoreOffset = offset + 200; - int ignoreLength = dataLength - 300; - - snap.addMergeRegion(ignoreOffset, - ignoreLength, - dataType, - faabric::util::SnapshotMergeOperation::Ignore); - - // Make a modification that will _not_ be ignored - updatedData[10] = 3; - expectedData[10] = 3; - - // Scatter some modifications through the ignore region, spanning - // multiple pages - updatedData[ignoreOffset + 5] = 1; - updatedData[ignoreOffset + HOST_PAGE_SIZE + 2] = 1; - updatedData[ignoreOffset + (2 * HOST_PAGE_SIZE) + 5] = 1; - - // Make changes right at the start and end of the ignore region - updatedData[ignoreOffset] = 1; - updatedData[ignoreOffset + ignoreLength] = 1; + operation = faabric::util::SnapshotMergeOperation::Ignore; + + // Scatter some modifications through the updated data, to make sure + // none are picked up + updatedData[0] = 1; + updatedData[sizeof(int32_t) - 2] = 1; + updatedData[sizeof(int32_t) + 10] = 1; + + expectedNumDiffs = 0; } } @@ -455,4 +440,54 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test invalid snapshot merges", "[util]") REQUIRE(failed); deallocatePages(snap.data, snapPages); } + +TEST_CASE_METHOD(SnapshotTestFixture, "Test cross-page ignores", "[util]") +{ + std::string snapKey = "foobar123"; + int snapPages = 5; + + faabric::util::SnapshotData snap; + snap.size = snapPages * faabric::util::HOST_PAGE_SIZE; + snap.data = allocatePages(snapPages); + + // Take the snapshot + reg.takeSnapshot(snapKey, snap, true); + + // Map the snapshot + size_t sharedMemSize = snapPages * HOST_PAGE_SIZE; + uint8_t* sharedMem = allocatePages(snapPages); + reg.mapSnapshot(snapKey, sharedMem); + + // Reset dirty tracking + faabric::util::resetDirtyTracking(); + + // Single ignore region to cover multiple pages + int ignoreOffset = HOST_PAGE_SIZE + 100; + int ignoreLength = 2 * HOST_PAGE_SIZE; + snap.addMergeRegion(ignoreOffset, + ignoreLength, + faabric::util::SnapshotDataType::Raw, + faabric::util::SnapshotMergeOperation::Ignore); + + // Add modifications that *will* cause diffs + // Make sure these are both just before, and just after the ignore region + sharedMem[0] = 1; + sharedMem[ignoreOffset - 1] = 1; + sharedMem[ignoreOffset + ignoreLength + 1] = 1; + + // Add modifications that should be ignored, including just inside both ends + // of the region + sharedMem[ignoreOffset] = 1; + sharedMem[ignoreOffset + HOST_PAGE_SIZE - 1] = 1; + sharedMem[ignoreOffset + HOST_PAGE_SIZE] = 1; + sharedMem[ignoreOffset + HOST_PAGE_SIZE + 1] = 1; + sharedMem[ignoreOffset + ignoreLength] = 1; + + // Check number of diffs + std::vector actualDiffs = + snap.getChangeDiffs(sharedMem, sharedMemSize); + + // Check number of diffs + REQUIRE(actualDiffs.size() == 3); +} } From 453b3a65573c2e9e649b94935a405d15eed5b21d Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Mon, 18 Oct 2021 11:57:23 +0000 Subject: [PATCH 3/7] Logging for diffs --- include/faabric/util/snapshot.h | 6 +++ src/util/snapshot.cpp | 88 ++++++++++++++++++++++++++++--- tests/test/util/test_snapshot.cpp | 45 +++++++++++----- 3 files changed, 120 insertions(+), 19 deletions(-) diff --git a/include/faabric/util/snapshot.h b/include/faabric/util/snapshot.h index bad839899..e9ac6873a 100644 --- a/include/faabric/util/snapshot.h +++ b/include/faabric/util/snapshot.h @@ -6,6 +6,8 @@ #include #include +#include + namespace faabric::util { enum SnapshotDataType @@ -76,4 +78,8 @@ class SnapshotData // order of offsets std::map mergeRegions; }; + +std::string snapshotDataTypeStr(SnapshotDataType dt); + +std::string snapshotMergeOpStr(SnapshotMergeOperation op); } diff --git a/src/util/snapshot.cpp b/src/util/snapshot.cpp index 2f9a38ed8..880341601 100644 --- a/src/util/snapshot.cpp +++ b/src/util/snapshot.cpp @@ -91,8 +91,6 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, diff.dataType = region.dataType; diff.operation = region.operation; - bool isIgnore = false; - // Modify diff data for certain operations switch (region.dataType) { case (SnapshotDataType::Int): { @@ -101,6 +99,7 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, int updatedInt = *(reinterpret_cast(updatedValue)); + std::string label; switch (region.operation) { case (SnapshotMergeOperation::Sum): { // Sums must send the value to be _added_, and @@ -144,7 +143,6 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, case (SnapshotDataType::Raw): { switch (region.operation) { case (SnapshotMergeOperation::Ignore): { - isIgnore = true; break; } case (SnapshotMergeOperation::Overwrite): { @@ -159,6 +157,7 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, "Unhandled raw merge operation"); } } + break; } default: { @@ -169,8 +168,15 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, } } + SPDLOG_TRACE("Diff at {} falls in {} {} merge region {}-{}", + pageOffset + b, + snapshotDataTypeStr(region.dataType), + snapshotMergeOpStr(region.operation), + region.offset, + region.offset + region.length); + // Add the diff to the list - if (!isIgnore) { + if (diff.operation != SnapshotMergeOperation::Ignore) { diffs.emplace_back(diff); } @@ -178,7 +184,7 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, int regionEndOffset = (region.offset - pageOffset) + region.length; - if (regionEndOffset <= HOST_PAGE_SIZE) { + if (regionEndOffset < HOST_PAGE_SIZE) { // Bump the loop variable to the end of this region (note // that the loop itself will increment onto the next). b = regionEndOffset - 1; @@ -186,7 +192,17 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, // Move onto the next merge region ++mergeIt; } else { - // End work on this page + // End work on this page, region continues into next + SPDLOG_TRACE( + "{} {} merge region {}-{} crosses page {} ({}-{})", + snapshotDataTypeStr(region.dataType), + snapshotMergeOpStr(region.operation), + region.offset, + region.offset + region.length, + i, + pageOffset, + pageOffset + HOST_PAGE_SIZE); + break; } } else if (isDirtyByte && !diffInProgress) { @@ -198,6 +214,12 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, diffInProgress = false; diffs.emplace_back( diffStart, updated + diffStart, offset - diffStart); + + SPDLOG_TRACE("Found {} {} diff between {}-{}", + snapshotDataTypeStr(diffs.back().dataType), + snapshotMergeOpStr(diffs.back().operation), + diffs.back().offset, + diffs.back().offset + diffs.back().size); } } @@ -205,8 +227,15 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, // off if (diffInProgress) { offset++; + diffs.emplace_back( diffStart, updated + diffStart, offset - diffStart); + + SPDLOG_TRACE("Found {} {} diff between {}-{} at end of page", + snapshotDataTypeStr(diffs.back().dataType), + snapshotMergeOpStr(diffs.back().operation), + diffs.back().offset, + diffs.back().offset + diffs.back().size); } } @@ -232,4 +261,51 @@ void SnapshotData::addMergeRegion(uint32_t offset, faabric::util::UniqueLock lock(snapMx); mergeRegions[offset] = region; } + +std::string snapshotDataTypeStr(SnapshotDataType dt) +{ + switch (dt) { + case (SnapshotDataType::Raw): { + return "Raw"; + } + case (SnapshotDataType::Int): { + return "Int"; + } + default: { + SPDLOG_ERROR("Cannot convert snapshot data type to string: {}", dt); + throw std::runtime_error("Cannot convert data type to string"); + } + } +} + +std::string snapshotMergeOpStr(SnapshotMergeOperation op) +{ + switch (op) { + case (SnapshotMergeOperation::Ignore): { + return "Ignore"; + } + case (SnapshotMergeOperation::Max): { + return "Max"; + } + case (SnapshotMergeOperation::Min): { + return "Min"; + } + case (SnapshotMergeOperation::Overwrite): { + return "Overwrite"; + } + case (SnapshotMergeOperation::Product): { + return "Product"; + } + case (SnapshotMergeOperation::Subtract): { + return "Subtract"; + } + case (SnapshotMergeOperation::Sum): { + return "Sum"; + } + default: { + SPDLOG_ERROR("Cannot convert snapshot merge op to string: {}", op); + throw std::runtime_error("Cannot convert merge op to string"); + } + } +} } diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index 4140c0570..970f51400 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -469,19 +469,18 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test cross-page ignores", "[util]") faabric::util::SnapshotDataType::Raw, faabric::util::SnapshotMergeOperation::Ignore); - // Add modifications that *will* cause diffs - // Make sure these are both just before, and just after the ignore region - sharedMem[0] = 1; - sharedMem[ignoreOffset - 1] = 1; - sharedMem[ignoreOffset + ignoreLength + 1] = 1; - - // Add modifications that should be ignored, including just inside both ends - // of the region - sharedMem[ignoreOffset] = 1; - sharedMem[ignoreOffset + HOST_PAGE_SIZE - 1] = 1; - sharedMem[ignoreOffset + HOST_PAGE_SIZE] = 1; - sharedMem[ignoreOffset + HOST_PAGE_SIZE + 1] = 1; - sharedMem[ignoreOffset + ignoreLength] = 1; + // Add modifications that *will* cause diffs and some should be ignored, + // including just inside and outside both ends of the ignore region + std::vector dataA(10, 1); + + std::memcpy(sharedMem, dataA.data(), dataA.size()); // Not ignored + sharedMem[ignoreOffset - 1] = 3; // Not ignored + sharedMem[ignoreOffset] = 1; // Ignored + sharedMem[ignoreOffset + HOST_PAGE_SIZE - 1] = 1; // Ignored + sharedMem[ignoreOffset + HOST_PAGE_SIZE] = 1; // Ignored + sharedMem[ignoreOffset + HOST_PAGE_SIZE + 1] = 1; // Ignored + sharedMem[ignoreOffset + ignoreLength] = 1; // Ignored + sharedMem[ignoreOffset + ignoreLength + 1] = 1; // Not ignored // Check number of diffs std::vector actualDiffs = @@ -489,5 +488,25 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test cross-page ignores", "[util]") // Check number of diffs REQUIRE(actualDiffs.size() == 3); + + SnapshotDiff diffA = actualDiffs.at(0); + SnapshotDiff diffB = actualDiffs.at(1); + SnapshotDiff diffC = actualDiffs.at(2); + + REQUIRE(diffA.offset == 0); + REQUIRE(diffB.offset == ignoreOffset - 1); + REQUIRE(diffC.offset == ignoreOffset + ignoreLength + 1); + + REQUIRE(diffA.operation == SnapshotMergeOperation::Overwrite); + REQUIRE(diffB.operation == SnapshotMergeOperation::Overwrite); + REQUIRE(diffC.operation == SnapshotMergeOperation::Overwrite); + + REQUIRE(diffA.dataType == SnapshotDataType::Raw); + REQUIRE(diffB.dataType == SnapshotDataType::Raw); + REQUIRE(diffC.dataType == SnapshotDataType::Raw); + + REQUIRE(diffA.size == dataA.size()); + REQUIRE(diffB.size == 1); + REQUIRE(diffC.size == 1); } } From 8f14e17914addccae4b0b9e7ab04d74d1ac1fb4f Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Mon, 18 Oct 2021 16:37:08 +0000 Subject: [PATCH 4/7] Fix up tests --- include/faabric/util/snapshot.h | 17 ++- src/util/snapshot.cpp | 42 +++++- tests/test/util/test_snapshot.cpp | 238 ++++++++++++++++++++---------- 3 files changed, 211 insertions(+), 86 deletions(-) diff --git a/include/faabric/util/snapshot.h b/include/faabric/util/snapshot.h index e9ac6873a..e0d195f6f 100644 --- a/include/faabric/util/snapshot.h +++ b/include/faabric/util/snapshot.h @@ -38,14 +38,27 @@ struct SnapshotMergeRegion class SnapshotDiff { public: + SnapshotDataType dataType = SnapshotDataType::Raw; + SnapshotMergeOperation operation = SnapshotMergeOperation::Overwrite; uint32_t offset = 0; size_t size = 0; const uint8_t* data = nullptr; - SnapshotDataType dataType = SnapshotDataType::Raw; - SnapshotMergeOperation operation = SnapshotMergeOperation::Overwrite; SnapshotDiff() = default; + SnapshotDiff(SnapshotDataType dataTypeIn, + SnapshotMergeOperation operationIn, + uint32_t offsetIn, + const uint8_t* dataIn, + size_t sizeIn) + { + dataType = dataTypeIn; + operation = operationIn; + offset = offsetIn; + data = dataIn; + size = sizeIn; + } + SnapshotDiff(uint32_t offsetIn, const uint8_t* dataIn, size_t sizeIn) { offset = offsetIn; diff --git a/src/util/snapshot.cpp b/src/util/snapshot.cpp index 880341601..5c345f30c 100644 --- a/src/util/snapshot.cpp +++ b/src/util/snapshot.cpp @@ -48,9 +48,12 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, // Get byte-wise diffs _within_ the dirty pages // - // NOTE - this will cause diffs to be split across pages if they hit a page - // boundary, but we can be relatively confident that variables will be - // page-aligned so this shouldn't be a problem + // NOTE - if raw diffs cover page boundaries, they will be split into + // multiple diffs, each of which is page-aligned. + // We can be relatively confident that variables will be page-aligned so + // this shouldn't be a problem. + // + // Merge regions support crossing page boundaries. // // For each byte we encounter have the following possible scenarios: // @@ -81,6 +84,22 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, offset < (mergeIt->second.offset + mergeIt->second.length); if (isDirtyByte && isInMergeRegion) { + // If we've entered a merge region with a diff in progress, we + // need to close it off + if (diffInProgress) { + diffs.emplace_back( + diffStart, updated + diffStart, offset - diffStart); + + SPDLOG_TRACE( + "Finished {} {} diff between {}-{} before merge region", + snapshotDataTypeStr(diffs.back().dataType), + snapshotMergeOpStr(diffs.back().operation), + diffs.back().offset, + diffs.back().offset + diffs.back().size); + + diffInProgress = false; + } + SnapshotMergeRegion region = mergeIt->second; // Set up the diff @@ -99,7 +118,6 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, int updatedInt = *(reinterpret_cast(updatedValue)); - std::string label; switch (region.operation) { case (SnapshotMergeOperation::Sum): { // Sums must send the value to be _added_, and @@ -185,16 +203,26 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, (region.offset - pageOffset) + region.length; if (regionEndOffset < HOST_PAGE_SIZE) { + // Finish this region, move onto the next + SPDLOG_TRACE( + "{} {} merge region {}-{} finished. Skipping to {}", + snapshotDataTypeStr(region.dataType), + snapshotMergeOpStr(region.operation), + region.offset, + region.offset + region.length, + regionEndOffset); + // Bump the loop variable to the end of this region (note // that the loop itself will increment onto the next). b = regionEndOffset - 1; // Move onto the next merge region ++mergeIt; + } else { // End work on this page, region continues into next SPDLOG_TRACE( - "{} {} merge region {}-{} crosses page {} ({}-{})", + "{} {} merge region {}-{} over page boundary {} ({}-{})", snapshotDataTypeStr(region.dataType), snapshotMergeOpStr(region.operation), region.offset, @@ -209,13 +237,15 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, // Diff starts here if it's different and diff not in progress diffInProgress = true; diffStart = offset; + + SPDLOG_TRACE("Started Raw Overwrite diff at {}", diffStart); } else if (!isDirtyByte && diffInProgress) { // Diff ends if it's not different and diff is in progress diffInProgress = false; diffs.emplace_back( diffStart, updated + diffStart, offset - diffStart); - SPDLOG_TRACE("Found {} {} diff between {}-{}", + SPDLOG_TRACE("Finished {} {} diff between {}-{}", snapshotDataTypeStr(diffs.back().dataType), snapshotMergeOpStr(diffs.back().operation), diffs.back().offset, diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index 970f51400..fb02e3921 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -12,7 +12,59 @@ using namespace faabric::util; namespace tests { -TEST_CASE_METHOD(SnapshotTestFixture, +class SnapshotMergeTestFixture : public SnapshotTestFixture +{ + public: + SnapshotMergeTestFixture() {} + ~SnapshotMergeTestFixture() {} + + protected: + std::string snapKey; + int snapPages; + faabric::util::SnapshotData snap; + + uint8_t* setUpSnapshot(int snapPages) + { + snapKey = "foobar123"; + snap.size = snapPages * faabric::util::HOST_PAGE_SIZE; + snap.data = allocatePages(snapPages); + + // Take the snapshot + reg.takeSnapshot(snapKey, snap, true); + + // Map the snapshot + uint8_t* sharedMem = allocatePages(snapPages); + reg.mapSnapshot(snapKey, sharedMem); + + // Reset dirty tracking + faabric::util::resetDirtyTracking(); + + return sharedMem; + } + + void checkDiffs(std::vector& actualDiffs, + std::vector& expectedDiffs) + { + REQUIRE(actualDiffs.size() == expectedDiffs.size()); + + for (int i = 0; i < actualDiffs.size(); i++) { + SnapshotDiff actualDiff = actualDiffs.at(i); + SnapshotDiff expectedDiff = expectedDiffs.at(i); + + REQUIRE(actualDiff.operation == expectedDiff.operation); + REQUIRE(actualDiff.dataType == expectedDiff.dataType); + REQUIRE(actualDiff.offset == expectedDiff.offset); + + std::vector actualData(actualDiff.data, + actualDiff.data + actualDiff.size); + std::vector expectedData( + expectedDiff.data, expectedDiff.data + expectedDiff.size); + REQUIRE(actualData == expectedData); + } + } +}; + +TEST_CASE_METHOD(SnapshotMergeTestFixture, "Detailed test snapshot merge regions with ints", "[util]") { @@ -123,7 +175,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, deallocatePages(snap.data, snapPages); } -TEST_CASE_METHOD(SnapshotTestFixture, +TEST_CASE_METHOD(SnapshotMergeTestFixture, "Test edge-cases of snapshot merge regions", "[util]") { @@ -139,22 +191,22 @@ TEST_CASE_METHOD(SnapshotTestFixture, int originalA = 50; int finalA = 25; int subA = 25; - int offsetA = 0; + uint32_t offsetA = 0; int originalB = 100; int finalB = 200; int sumB = 100; - int offsetB = HOST_PAGE_SIZE + (2 * sizeof(int32_t)); + uint32_t offsetB = HOST_PAGE_SIZE + (2 * sizeof(int32_t)); int originalC = 200; int finalC = 150; int subC = 50; - int offsetC = offsetB + sizeof(int32_t); + uint32_t offsetC = offsetB + sizeof(int32_t); int originalD = 100; int finalD = 150; int sumD = 50; - int offsetD = snapSize - sizeof(int32_t); + uint32_t offsetD = snapSize - sizeof(int32_t); faabric::util::SnapshotData snap; snap.size = snapSize; @@ -202,37 +254,44 @@ TEST_CASE_METHOD(SnapshotTestFixture, *(int*)(sharedMem + offsetD) = finalD; // Check the diffs + std::vector expectedDiffs = { + { SnapshotDataType::Int, + SnapshotMergeOperation::Subtract, + offsetA, + BYTES(&subA), + sizeof(int32_t) }, + { SnapshotDataType::Int, + SnapshotMergeOperation::Sum, + offsetB, + BYTES(&sumB), + sizeof(int32_t) }, + { SnapshotDataType::Int, + SnapshotMergeOperation::Subtract, + offsetC, + BYTES(&subC), + sizeof(int32_t) }, + { SnapshotDataType::Int, + SnapshotMergeOperation::Sum, + offsetD, + BYTES(&sumD), + sizeof(int32_t) }, + }; + std::vector actualDiffs = snap.getChangeDiffs(sharedMem, sharedMemSize); REQUIRE(actualDiffs.size() == 4); - SnapshotDiff diffA = actualDiffs.at(0); - SnapshotDiff diffB = actualDiffs.at(1); - SnapshotDiff diffC = actualDiffs.at(2); - SnapshotDiff diffD = actualDiffs.at(3); - - REQUIRE(diffA.offset == offsetA); - REQUIRE(diffB.offset == offsetB); - REQUIRE(diffC.offset == offsetC); - REQUIRE(diffD.offset == offsetD); - - REQUIRE(diffA.operation == SnapshotMergeOperation::Subtract); - REQUIRE(diffB.operation == SnapshotMergeOperation::Sum); - REQUIRE(diffC.operation == SnapshotMergeOperation::Subtract); - REQUIRE(diffD.operation == SnapshotMergeOperation::Sum); - - REQUIRE(*(int*)diffA.data == subA); - REQUIRE(*(int*)diffB.data == sumB); - REQUIRE(*(int*)diffC.data == subC); - REQUIRE(*(int*)diffD.data == sumD); + checkDiffs(actualDiffs, expectedDiffs); } -TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot merge regions", "[util]") +TEST_CASE_METHOD(SnapshotMergeTestFixture, + "Test snapshot merge regions", + "[util]") { std::string snapKey = "foobar123"; int snapPages = 5; - int offset = HOST_PAGE_SIZE + (10 * sizeof(int32_t)); + uint32_t offset = HOST_PAGE_SIZE + (10 * sizeof(int32_t)); faabric::util::SnapshotData snap; snap.size = snapPages * faabric::util::HOST_PAGE_SIZE; @@ -362,21 +421,21 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot merge regions", "[util]") REQUIRE(actualDiffs.size() == expectedNumDiffs); if (expectedNumDiffs == 1) { - SnapshotDiff diff = actualDiffs.at(0); - REQUIRE(diff.offset == offset); - REQUIRE(diff.operation == operation); - REQUIRE(diff.dataType == dataType); - REQUIRE(diff.size == dataLength); - - // Check actual and expected - std::vector actualData(diff.data, diff.data + dataLength); - REQUIRE(actualData == expectedData); + std::vector expectedDiffs = { { dataType, + operation, + offset, + expectedData.data(), + expectedData.size() } }; + + checkDiffs(actualDiffs, expectedDiffs); } deallocatePages(snap.data, snapPages); } -TEST_CASE_METHOD(SnapshotTestFixture, "Test invalid snapshot merges", "[util]") +TEST_CASE_METHOD(SnapshotMergeTestFixture, + "Test invalid snapshot merges", + "[util]") { std::string snapKey = "foobar123"; int snapPages = 3; @@ -441,25 +500,11 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test invalid snapshot merges", "[util]") deallocatePages(snap.data, snapPages); } -TEST_CASE_METHOD(SnapshotTestFixture, "Test cross-page ignores", "[util]") +TEST_CASE_METHOD(SnapshotMergeTestFixture, "Test cross-page ignores", "[util]") { - std::string snapKey = "foobar123"; int snapPages = 5; - - faabric::util::SnapshotData snap; - snap.size = snapPages * faabric::util::HOST_PAGE_SIZE; - snap.data = allocatePages(snapPages); - - // Take the snapshot - reg.takeSnapshot(snapKey, snap, true); - - // Map the snapshot size_t sharedMemSize = snapPages * HOST_PAGE_SIZE; - uint8_t* sharedMem = allocatePages(snapPages); - reg.mapSnapshot(snapKey, sharedMem); - - // Reset dirty tracking - faabric::util::resetDirtyTracking(); + uint8_t* sharedMem = setUpSnapshot(snapPages); // Single ignore region to cover multiple pages int ignoreOffset = HOST_PAGE_SIZE + 100; @@ -472,41 +517,78 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test cross-page ignores", "[util]") // Add modifications that *will* cause diffs and some should be ignored, // including just inside and outside both ends of the ignore region std::vector dataA(10, 1); + std::vector dataB(1, 1); + std::vector dataC(1, 1); + + uint32_t offsetA = 0; + std::memcpy(sharedMem + offsetA, dataA.data(), dataA.size()); // Not ignored + + uint32_t offsetB = ignoreOffset - 1; + std::memcpy(sharedMem + offsetB, dataB.data(), dataB.size()); // Not ignored + + sharedMem[ignoreOffset] = (uint8_t)1; // Ignored + + sharedMem[ignoreOffset + HOST_PAGE_SIZE - 1] = (uint8_t)1; // Ignored + + sharedMem[ignoreOffset + HOST_PAGE_SIZE] = (uint8_t)1; // Ignored + + sharedMem[ignoreOffset + HOST_PAGE_SIZE + 1] = (uint8_t)1; // Ignored + + sharedMem[ignoreOffset + ignoreLength - 1] = (uint8_t)1; // Ignored + + uint32_t offsetC = ignoreOffset + ignoreLength; + std::memcpy(sharedMem + offsetC, dataC.data(), + dataC.size()); // Not ignored - std::memcpy(sharedMem, dataA.data(), dataA.size()); // Not ignored - sharedMem[ignoreOffset - 1] = 3; // Not ignored - sharedMem[ignoreOffset] = 1; // Ignored - sharedMem[ignoreOffset + HOST_PAGE_SIZE - 1] = 1; // Ignored - sharedMem[ignoreOffset + HOST_PAGE_SIZE] = 1; // Ignored - sharedMem[ignoreOffset + HOST_PAGE_SIZE + 1] = 1; // Ignored - sharedMem[ignoreOffset + ignoreLength] = 1; // Ignored - sharedMem[ignoreOffset + ignoreLength + 1] = 1; // Not ignored + std::vector expectedDiffs = { + { offsetA, dataA.data(), dataA.size() }, + { offsetB, dataB.data(), dataB.size() }, + { offsetC, dataC.data(), dataC.size() }, + }; // Check number of diffs std::vector actualDiffs = snap.getChangeDiffs(sharedMem, sharedMemSize); - // Check number of diffs - REQUIRE(actualDiffs.size() == 3); + checkDiffs(actualDiffs, expectedDiffs); +} - SnapshotDiff diffA = actualDiffs.at(0); - SnapshotDiff diffB = actualDiffs.at(1); - SnapshotDiff diffC = actualDiffs.at(2); +TEST_CASE_METHOD(SnapshotMergeTestFixture, + "Test fine-grained byte-wise diffs", + "[util]") +{ + int snapPages = 3; + size_t sharedMemSize = snapPages * HOST_PAGE_SIZE; + uint8_t* sharedMem = setUpSnapshot(snapPages); + + // Add some tightly-packed changes + uint32_t offsetA = 0; + std::vector dataA(10, 1); + std::memcpy(sharedMem + offsetA, dataA.data(), dataA.size()); + + uint32_t offsetB = dataA.size() + 1; + std::vector dataB(2, 1); + std::memcpy(sharedMem + offsetB, dataB.data(), dataB.size()); - REQUIRE(diffA.offset == 0); - REQUIRE(diffB.offset == ignoreOffset - 1); - REQUIRE(diffC.offset == ignoreOffset + ignoreLength + 1); + uint32_t offsetC = offsetB + 3; + std::vector dataC(1, 1); + std::memcpy(sharedMem + offsetC, dataC.data(), dataC.size()); - REQUIRE(diffA.operation == SnapshotMergeOperation::Overwrite); - REQUIRE(diffB.operation == SnapshotMergeOperation::Overwrite); - REQUIRE(diffC.operation == SnapshotMergeOperation::Overwrite); + uint32_t offsetD = offsetC + 2; + std::vector dataD(1, 1); + std::memcpy(sharedMem + offsetD, dataD.data(), dataD.size()); - REQUIRE(diffA.dataType == SnapshotDataType::Raw); - REQUIRE(diffB.dataType == SnapshotDataType::Raw); - REQUIRE(diffC.dataType == SnapshotDataType::Raw); + std::vector expectedDiffs = { + { offsetA, dataA.data(), dataA.size() }, + { offsetB, dataB.data(), dataB.size() }, + { offsetC, dataC.data(), dataC.size() }, + { offsetD, dataD.data(), dataD.size() }, + }; + + // Check number of diffs + std::vector actualDiffs = + snap.getChangeDiffs(sharedMem, sharedMemSize); - REQUIRE(diffA.size == dataA.size()); - REQUIRE(diffB.size == 1); - REQUIRE(diffC.size == 1); + checkDiffs(actualDiffs, expectedDiffs); } } From 5fdd7b42f53c9b598573e1d308ee0836a52a431f Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Tue, 19 Oct 2021 11:08:54 +0000 Subject: [PATCH 5/7] Fixed snapshot tests --- src/util/snapshot.cpp | 44 ++++++++++++++++++----- tests/test/util/test_snapshot.cpp | 60 +++++++++++++++++++++---------- 2 files changed, 76 insertions(+), 28 deletions(-) diff --git a/src/util/snapshot.cpp b/src/util/snapshot.cpp index 5c345f30c..09592b1fd 100644 --- a/src/util/snapshot.cpp +++ b/src/util/snapshot.cpp @@ -42,6 +42,20 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, std::vector dirtyPageNumbers = getDirtyPageNumbers(updated, nThisPages); + SPDLOG_TRACE("Diffing {} pages with {} changed pages and {} merge regions", + nThisPages, + dirtyPageNumbers.size(), + mergeRegions.size()); + + for (auto& m : mergeRegions) { + SPDLOG_TRACE("{} {} merge region at {} {}-{}", + snapshotDataTypeStr(m.second.dataType), + snapshotMergeOpStr(m.second.operation), + m.first, + m.second.offset, + m.second.offset + m.second.length); + } + // Get iterator over merge regions std::map::iterator mergeIt = mergeRegions.begin(); @@ -78,6 +92,21 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, offset = pageOffset + b; bool isDirtyByte = *(data + offset) != *(updated + offset); + // Skip any merge regions we've passed + while (mergeIt != mergeRegions.end() && + offset > (mergeIt->second.offset + mergeIt->second.length)) { + SnapshotMergeRegion region = mergeIt->second; + SPDLOG_TRACE("At offset {}, skipping region {} {} {}-{}", + offset, + snapshotDataTypeStr(region.dataType), + snapshotMergeOpStr(region.operation), + region.offset, + region.offset + region.length); + + ++mergeIt; + } + + // Check if we're in a merge region bool isInMergeRegion = mergeIt != mergeRegions.end() && offset >= mergeIt->second.offset && @@ -198,12 +227,13 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, diffs.emplace_back(diff); } - // Work out the offset to where this region ends + // Work out the offset where this region ends int regionEndOffset = (region.offset - pageOffset) + region.length; if (regionEndOffset < HOST_PAGE_SIZE) { - // Finish this region, move onto the next + // Skip over this region, still more offsets left in this + // page SPDLOG_TRACE( "{} {} merge region {}-{} finished. Skipping to {}", snapshotDataTypeStr(region.dataType), @@ -215,12 +245,8 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, // Bump the loop variable to the end of this region (note // that the loop itself will increment onto the next). b = regionEndOffset - 1; - - // Move onto the next merge region - ++mergeIt; - } else { - // End work on this page, region continues into next + // Merge region extends over this page, move onto next SPDLOG_TRACE( "{} {} merge region {}-{} over page boundary {} ({}-{})", snapshotDataTypeStr(region.dataType), @@ -253,8 +279,8 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, } } - // If we've reached the end with a diff in progress, we need to close it - // off + // If we've reached the end of this page with a diff in progress, we + // need to close it off if (diffInProgress) { offset++; diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index fb02e3921..f62858fe1 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -502,48 +502,70 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, TEST_CASE_METHOD(SnapshotMergeTestFixture, "Test cross-page ignores", "[util]") { - int snapPages = 5; + int snapPages = 6; size_t sharedMemSize = snapPages * HOST_PAGE_SIZE; uint8_t* sharedMem = setUpSnapshot(snapPages); - // Single ignore region to cover multiple pages - int ignoreOffset = HOST_PAGE_SIZE + 100; - int ignoreLength = 2 * HOST_PAGE_SIZE; - snap.addMergeRegion(ignoreOffset, - ignoreLength, + // Add ignore regions that cover multiple pages + int ignoreOffsetA = HOST_PAGE_SIZE + 100; + int ignoreLengthA = 2 * HOST_PAGE_SIZE; + snap.addMergeRegion(ignoreOffsetA, + ignoreLengthA, + faabric::util::SnapshotDataType::Raw, + faabric::util::SnapshotMergeOperation::Ignore); + + int ignoreOffsetB = sharedMemSize - (HOST_PAGE_SIZE + 10); + int ignoreLengthB = 30; + snap.addMergeRegion(ignoreOffsetB, + ignoreLengthB, faabric::util::SnapshotDataType::Raw, faabric::util::SnapshotMergeOperation::Ignore); - // Add modifications that *will* cause diffs and some should be ignored, - // including just inside and outside both ends of the ignore region + // Add some modifications that will cause diffs, and some should be ignored std::vector dataA(10, 1); std::vector dataB(1, 1); std::vector dataC(1, 1); + std::vector dataD(3, 1); + // Not ignored, start of memory uint32_t offsetA = 0; - std::memcpy(sharedMem + offsetA, dataA.data(), dataA.size()); // Not ignored + std::memcpy(sharedMem + offsetA, dataA.data(), dataA.size()); - uint32_t offsetB = ignoreOffset - 1; - std::memcpy(sharedMem + offsetB, dataB.data(), dataB.size()); // Not ignored + // Not ignored, just before first ignore region + uint32_t offsetB = ignoreOffsetA - 1; + std::memcpy(sharedMem + offsetB, dataB.data(), dataB.size()); - sharedMem[ignoreOffset] = (uint8_t)1; // Ignored + // Not ignored, just after first ignore region + uint32_t offsetC = ignoreOffsetA + ignoreLengthA; + std::memcpy(sharedMem + offsetC, dataC.data(), dataC.size()); + + // Not ignored, just before second ignore region + uint32_t offsetD = ignoreOffsetB - 4; + std::memcpy(sharedMem + offsetD, dataD.data(), dataD.size()); - sharedMem[ignoreOffset + HOST_PAGE_SIZE - 1] = (uint8_t)1; // Ignored + // Ignored, start of first ignore region + sharedMem[ignoreOffsetA] = (uint8_t)1; - sharedMem[ignoreOffset + HOST_PAGE_SIZE] = (uint8_t)1; // Ignored + // Ignored, just before page boundary in ignore region + sharedMem[(2 * HOST_PAGE_SIZE) - 1] = (uint8_t)1; - sharedMem[ignoreOffset + HOST_PAGE_SIZE + 1] = (uint8_t)1; // Ignored + // Ignored, just after page boundary in ignore region + sharedMem[(2 * HOST_PAGE_SIZE)] = (uint8_t)1; - sharedMem[ignoreOffset + ignoreLength - 1] = (uint8_t)1; // Ignored + // Deliberately don't put any changes after the next page boundary to check + // that it rolls over properly - uint32_t offsetC = ignoreOffset + ignoreLength; - std::memcpy(sharedMem + offsetC, dataC.data(), - dataC.size()); // Not ignored + // Ignored, just inside second region + sharedMem[ignoreOffsetB + 2] = (uint8_t)1; + + // Ignored, end of second region + sharedMem[ignoreOffsetB + ignoreLengthB - 1] = (uint8_t)1; std::vector expectedDiffs = { { offsetA, dataA.data(), dataA.size() }, { offsetB, dataB.data(), dataB.size() }, { offsetC, dataC.data(), dataC.size() }, + { offsetD, dataD.data(), dataD.size() }, }; // Check number of diffs From 23f14c1f1a037b70304c2d8a6c685362612ed1f1 Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Tue, 19 Oct 2021 12:02:30 +0000 Subject: [PATCH 6/7] Add test for skipping diffs --- src/util/snapshot.cpp | 7 ++-- tests/test/util/test_snapshot.cpp | 63 +++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 3 deletions(-) diff --git a/src/util/snapshot.cpp b/src/util/snapshot.cpp index 09592b1fd..f49e99a55 100644 --- a/src/util/snapshot.cpp +++ b/src/util/snapshot.cpp @@ -94,9 +94,10 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, // Skip any merge regions we've passed while (mergeIt != mergeRegions.end() && - offset > (mergeIt->second.offset + mergeIt->second.length)) { + offset >= + (mergeIt->second.offset + mergeIt->second.length)) { SnapshotMergeRegion region = mergeIt->second; - SPDLOG_TRACE("At offset {}, skipping region {} {} {}-{}", + SPDLOG_TRACE("At offset {}, past region {} {} {}-{}", offset, snapshotDataTypeStr(region.dataType), snapshotMergeOpStr(region.operation), @@ -240,7 +241,7 @@ std::vector SnapshotData::getChangeDiffs(const uint8_t* updated, snapshotMergeOpStr(region.operation), region.offset, region.offset + region.length, - regionEndOffset); + pageOffset + regionEndOffset); // Bump the loop variable to the end of this region (note // that the loop itself will increment onto the next). diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index f62858fe1..78c4b6936 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -613,4 +613,67 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, checkDiffs(actualDiffs, expectedDiffs); } + +TEST_CASE_METHOD(SnapshotMergeTestFixture, + "Test mix of applicable and non-applicable merge regions", + "[util]") +{ + int snapPages = 6; + size_t sharedMemSize = snapPages * HOST_PAGE_SIZE; + uint8_t* sharedMem = setUpSnapshot(snapPages); + + // Add a couple of merge regions on each page, which should be skipped as + // they won't cover any changes + for (int i = 0; i < snapPages; i++) { + // Ignore + int iOff = i * HOST_PAGE_SIZE; + snap.addMergeRegion(iOff, + 10, + faabric::util::SnapshotDataType::Raw, + faabric::util::SnapshotMergeOperation::Ignore); + + // Sum + int sOff = ((i + 1) * HOST_PAGE_SIZE) - (2 * sizeof(int32_t)); + snap.addMergeRegion(sOff, + sizeof(int32_t), + faabric::util::SnapshotDataType::Int, + faabric::util::SnapshotMergeOperation::Sum); + } + + // Add an ignore region that should take effect, along with a corresponding + // change to be ignored + uint32_t ignoreA = (2 * HOST_PAGE_SIZE) + 2; + snap.addMergeRegion(ignoreA, + 20, + faabric::util::SnapshotDataType::Raw, + faabric::util::SnapshotMergeOperation::Ignore); + std::vector dataA(10, 1); + std::memcpy(sharedMem + ignoreA, dataA.data(), dataA.size()); + + // Add a sum region and data that should also take effect + uint32_t sumOffset = (4 * HOST_PAGE_SIZE) + 100; + int sumValue = 111; + int sumOriginal = 222; + int sumExpected = 333; + snap.addMergeRegion(sumOffset, + sizeof(int32_t), + faabric::util::SnapshotDataType::Int, + faabric::util::SnapshotMergeOperation::Sum); + *(int*)(snap.data + sumOffset) = sumOriginal; + *(int*)(sharedMem + sumOffset) = sumValue; + + // Check diffs + std::vector expectedDiffs = { + { faabric::util::SnapshotDataType::Int, + faabric::util::SnapshotMergeOperation::Sum, + sumOffset, + BYTES(&sumExpected), + sizeof(int32_t) }, + }; + + std::vector actualDiffs = + snap.getChangeDiffs(sharedMem, sharedMemSize); + + checkDiffs(actualDiffs, expectedDiffs); +} } From 590f8e0db4d72814cfe8896d63a825282779ffaf Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Tue, 19 Oct 2021 12:26:54 +0000 Subject: [PATCH 7/7] Correct sum values in test --- tests/test/util/test_snapshot.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index 78c4b6936..4ae49966c 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -652,9 +652,9 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, // Add a sum region and data that should also take effect uint32_t sumOffset = (4 * HOST_PAGE_SIZE) + 100; - int sumValue = 111; - int sumOriginal = 222; - int sumExpected = 333; + int sumValue = 333; + int sumOriginal = 111; + int sumExpected = 222; snap.addMergeRegion(sumOffset, sizeof(int32_t), faabric::util::SnapshotDataType::Int,