From 593e98b4ec08f103fbde6111e871666a757858cb Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Tue, 8 Feb 2022 18:27:06 +0000 Subject: [PATCH 1/2] Adding xor diffing --- include/faabric/util/config.h | 1 + include/faabric/util/snapshot.h | 3 + src/util/config.cpp | 1 + src/util/snapshot.cpp | 82 ++++++++++++++--- tests/test/util/test_snapshot.cpp | 148 +++++++++++++++++++++++++----- 5 files changed, 197 insertions(+), 38 deletions(-) diff --git a/include/faabric/util/config.h b/include/faabric/util/config.h index 25106cd1b..13cd9643c 100644 --- a/include/faabric/util/config.h +++ b/include/faabric/util/config.h @@ -52,6 +52,7 @@ class SystemConfig // Dirty tracking std::string dirtyTrackingMode; + std::string diffingMode; SystemConfig(); diff --git a/include/faabric/util/snapshot.h b/include/faabric/util/snapshot.h index 0264a51d5..3cff2ce3f 100644 --- a/include/faabric/util/snapshot.h +++ b/include/faabric/util/snapshot.h @@ -28,6 +28,7 @@ enum SnapshotDataType enum SnapshotMergeOperation { Overwrite, + XOR, Sum, Product, Subtract, @@ -285,6 +286,8 @@ class SnapshotData void writeData(std::span buffer, uint32_t offset = 0); + void xorData(std::span buffer, uint32_t offset = 0); + void checkWriteExtension(std::span buffer, uint32_t offset); }; diff --git a/src/util/config.cpp b/src/util/config.cpp index 56ac5a831..dcd45403e 100644 --- a/src/util/config.cpp +++ b/src/util/config.cpp @@ -71,6 +71,7 @@ void SystemConfig::initialise() // Dirty tracking dirtyTrackingMode = getEnvVar("DIRTY_TRACKING_MODE", "segfault"); + diffingMode = getEnvVar("DIFFING_MODE", "xor"); } int SystemConfig::getSystemConfIntParam(const char* name, diff --git a/src/util/snapshot.cpp b/src/util/snapshot.cpp index f44715465..2acb1696e 100644 --- a/src/util/snapshot.cpp +++ b/src/util/snapshot.cpp @@ -153,6 +153,22 @@ void SnapshotData::writeData(std::span buffer, uint32_t offset) trackedChanges.emplace_back(offset, regionEnd); } +void SnapshotData::xorData(std::span buffer, uint32_t offset) +{ + size_t regionEnd = offset + buffer.size(); + if (regionEnd > size) { + SPDLOG_ERROR( + "XORing snapshot data exceeding size: {} > {}", regionEnd, size); + throw std::runtime_error("XORing data exceeding size"); + } + + uint8_t* copyTarget = validatedOffsetPtr(offset); + std::transform( + buffer.begin(), buffer.end(), copyTarget, copyTarget, std::bit_xor()); + + trackedChanges.emplace_back(offset, regionEnd); +} + const uint8_t* SnapshotData::getDataPtr(uint32_t offset) { faabric::util::SharedLock lock(snapMx); @@ -210,12 +226,22 @@ void SnapshotData::fillGapsWithOverwriteRegions() { faabric::util::FullLock lock(snapMx); + SnapshotMergeOperation fillType; + SystemConfig& conf = faabric::util::getSystemConfig(); + if (conf.diffingMode == "overwrite") { + fillType = SnapshotMergeOperation::Overwrite; + } else if (conf.diffingMode == "xor") { + fillType = SnapshotMergeOperation::XOR; + } else { + SPDLOG_ERROR("Unsupported diffing mode: {}", conf.diffingMode); + throw std::runtime_error("Unsupported diffing mode"); + } + // If there's no merge regions, just do one big one (note, zero length means // fill all space if (mergeRegions.empty()) { SPDLOG_TRACE("Filling gap with single overwrite merge region"); - mergeRegions.emplace_back( - 0, 0, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); + mergeRegions.emplace_back(0, 0, SnapshotDataType::Raw, fillType); return; } @@ -240,10 +266,8 @@ void SnapshotData::fillGapsWithOverwriteRegions() lastRegionEnd, lastRegionEnd + regionLen); - mergeRegions.emplace_back(lastRegionEnd, - regionLen, - SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + mergeRegions.emplace_back( + lastRegionEnd, regionLen, SnapshotDataType::Raw, fillType); lastRegionEnd = r.offset + r.length; } @@ -253,10 +277,8 @@ void SnapshotData::fillGapsWithOverwriteRegions() lastRegionEnd); // Add a final region at the end of the snapshot - mergeRegions.emplace_back(lastRegionEnd, - 0, - SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + mergeRegions.emplace_back( + lastRegionEnd, 0, SnapshotDataType::Raw, fillType); } } @@ -324,11 +346,15 @@ int SnapshotData::writeQueuedDiffs() continue; } + if (diff.getOperation() == faabric::util::SnapshotMergeOperation::Overwrite) { - writeData(diff.getData(), diff.getOffset()); + continue; + } + if (diff.getOperation() == faabric::util::SnapshotMergeOperation::XOR) { + xorData(diff.getData(), diff.getOffset()); continue; } @@ -550,6 +576,9 @@ std::string snapshotMergeOpStr(SnapshotMergeOperation op) case (SnapshotMergeOperation::Sum): { return "Sum"; } + case (SnapshotMergeOperation::XOR): { + return "XOR"; + } default: { SPDLOG_ERROR("Cannot convert snapshot merge op to string: {}", op); throw std::runtime_error("Cannot convert merge op to string"); @@ -622,7 +651,11 @@ void SnapshotMergeRegion::addDiffs(std::vector& diffs, } startPage += std::distance(startIt, foundIt); - if (operation == SnapshotMergeOperation::Overwrite) { + // Overwrites and XORs both deal with overwriting bytes without any + // other logic. Overwrites will filter in only the modified bytes, + // whereas XOR will transmit the XOR of the whole page and the original + if (operation == SnapshotMergeOperation::Overwrite || + operation == SnapshotMergeOperation::XOR) { // Iterate through pages for (int p = startPage; p < endPage; p++) { // Skip if page not dirty @@ -639,11 +672,30 @@ void SnapshotMergeRegion::addDiffs(std::vector& diffs, SPDLOG_TRACE("Checking page {} {}-{}", p, startByte, endByte); - diffArrayRegions( - diffs, startByte, endByte, originalData, updatedData); + if (operation == SnapshotMergeOperation::Overwrite) { + diffArrayRegions( + diffs, startByte, endByte, originalData, updatedData); + } else { + uint32_t rangeSize = endByte - startByte; + std::transform(originalData.begin() + startByte, + originalData.begin() + startByte + rangeSize, + updatedData.begin() + startByte, + updatedData.begin() + startByte, + std::bit_xor()); + + SPDLOG_TRACE("Adding {} XOR merge: {}-{}", + snapshotDataTypeStr(dataType), + startByte, + startByte + rangeSize); + + diffs.emplace_back(dataType, + operation, + startByte, + updatedData.subspan(startByte, rangeSize)); + } } - // This is the end of the overwrite diff + // This is the end of the XOR/overwrite diff return; } diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index 54aa92223..328c0899f 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -1064,9 +1064,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, checkDiffs(actualDiffs, expectedDiffs); } -TEST_CASE_METHOD(SnapshotMergeTestFixture, - "Test filling gaps in regions with overwrite", - "[snapshot][util]") +void doFillGapsChecks(SnapshotMergeOperation op) { int snapPages = 3; size_t snapSize = snapPages * HOST_PAGE_SIZE; @@ -1076,8 +1074,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, SECTION("No existing regions") { - expectedRegions.emplace_back( - 0, 0, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); + expectedRegions.emplace_back(0, 0, SnapshotDataType::Raw, op); } SECTION("One region at start") @@ -1088,8 +1085,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, expectedRegions.emplace_back( 0, 100, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); - expectedRegions.emplace_back( - 100, 0, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); + expectedRegions.emplace_back(100, 0, SnapshotDataType::Raw, op); } SECTION("One region at end") @@ -1099,10 +1095,8 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); - expectedRegions.emplace_back(0, - snapSize - 100, - SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + expectedRegions.emplace_back( + 0, snapSize - 100, SnapshotDataType::Raw, op); expectedRegions.emplace_back((uint32_t)snapSize - 100, 100, @@ -1126,8 +1120,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); - expectedRegions.emplace_back( - 0, 100, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); + expectedRegions.emplace_back(0, 100, SnapshotDataType::Raw, op); expectedRegions.emplace_back( 100, sizeof(int), SnapshotDataType::Int, SnapshotMergeOperation::Sum); @@ -1135,7 +1128,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, expectedRegions.emplace_back(100 + sizeof(int), HOST_PAGE_SIZE - (100 + sizeof(int)), SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + op); expectedRegions.emplace_back((uint32_t)HOST_PAGE_SIZE, sizeof(double), @@ -1146,17 +1139,15 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, (uint32_t)(HOST_PAGE_SIZE + sizeof(double)), 200 - sizeof(double), SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + op); expectedRegions.emplace_back((uint32_t)HOST_PAGE_SIZE + 200, (uint32_t)HOST_PAGE_SIZE, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); - expectedRegions.emplace_back((uint32_t)(2 * HOST_PAGE_SIZE) + 200, - 0, - SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + expectedRegions.emplace_back( + (uint32_t)(2 * HOST_PAGE_SIZE) + 200, 0, SnapshotDataType::Raw, op); } snap->fillGapsWithOverwriteRegions(); @@ -1170,11 +1161,45 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, for (int i = 0; i < actualRegions.size(); i++) { SnapshotMergeRegion expectedRegion = expectedRegions[i]; SnapshotMergeRegion actualRegion = actualRegions[i]; + REQUIRE(actualRegion == expectedRegion); + } +} - REQUIRE(actualRegion.offset == expectedRegion.offset); - REQUIRE(actualRegion.dataType == expectedRegion.dataType); - REQUIRE(actualRegion.length == expectedRegion.length); - REQUIRE(actualRegion.operation == expectedRegion.operation); +TEST_CASE_METHOD(SnapshotMergeTestFixture, + "Test filling gaps in regions", + "[snapshot][util]") +{ + SystemConfig& conf = getSystemConfig(); + SECTION("Overwrite") + { + conf.diffingMode = "overwrite"; + doFillGapsChecks(SnapshotMergeOperation::Overwrite); + } + + SECTION("XOR") + { + conf.diffingMode = "xor"; + doFillGapsChecks(SnapshotMergeOperation::XOR); + } + + SECTION("Unsupported") + { + conf.diffingMode = "foobar"; + + int snapPages = 3; + size_t snapSize = snapPages * HOST_PAGE_SIZE; + auto snap = std::make_shared(snapSize); + + bool failed = false; + try { + snap->fillGapsWithOverwriteRegions(); + } catch (std::runtime_error& ex) { + std::string expected = "Unsupported diffing mode"; + REQUIRE(ex.what() == expected); + failed = true; + } + + REQUIRE(failed); } } @@ -1835,4 +1860,81 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, REQUIRE(diffA.getDataCopy() == dataA); REQUIRE(diffB.getDataCopy() == dataB); } + +TEST_CASE_METHOD(DirtyTrackingTestFixture, "Test XOR diffs", "[util][snapshot]") +{ + int nSnapPages = 5; + size_t snapSize = nSnapPages * HOST_PAGE_SIZE; + + auto snap = std::make_shared(snapSize); + + std::vector expectedDirtyPages(nSnapPages, 0); + std::vector expectedSnapData(snapSize, 0); + + std::vector dataA(150, 3); + std::vector dataB(200, 4); + + uint32_t offsetA = 0; + uint32_t offsetB = 2 * HOST_PAGE_SIZE + 100; + expectedDirtyPages[0] = 1; + expectedDirtyPages[2] = 1; + + // Add merge regions + snap->addMergeRegion(offsetA, + dataA.size(), + SnapshotDataType::Raw, + SnapshotMergeOperation::XOR); + + snap->addMergeRegion(offsetB, + dataB.size(), + SnapshotDataType::Raw, + SnapshotMergeOperation::XOR); + + // Map some memory + MemoryRegion mem = allocatePrivateMemory(snapSize); + std::span memView(mem.get(), snapSize); + snap->mapToMemory(memView); + + // Clear tracking + DirtyTracker& tracker = getDirtyTracker(); + tracker.clearAll(); + snap->clearTrackedChanges(); + + // Start tracking + tracker.startTracking(memView); + tracker.startThreadLocalTracking(memView); + + // Copy in data + std::memcpy(mem.get() + offsetA, dataA.data(), dataA.size()); + std::memcpy(mem.get() + offsetB, dataB.data(), dataB.size()); + + std::memcpy(expectedSnapData.data() + offsetA, dataA.data(), dataA.size()); + std::memcpy(expectedSnapData.data() + offsetB, dataB.data(), dataB.size()); + + // Stop tracking + tracker.stopTracking(memView); + tracker.stopThreadLocalTracking(memView); + + // Get diffs + std::vector dirtyPages = tracker.getBothDirtyPages(memView); + REQUIRE(dirtyPages == expectedDirtyPages); + + // Diff with snapshot + std::vector actual = + snap->diffWithDirtyRegions(memView, dirtyPages); + + REQUIRE(actual.size() == 2); + SnapshotDiff diffA = actual.at(0); + SnapshotDiff diffB = actual.at(1); + + REQUIRE(diffA.getOffset() == offsetA); + REQUIRE(diffB.getOffset() == offsetB); + + // Apply the diffs to the snapshot + snap->queueDiffs(actual); + snap->writeQueuedDiffs(); + + // Check snapshot data is now as expected + REQUIRE(snap->getDataCopy() == expectedSnapData); +} } From 5e0a9d89d7474cb0a21d207147b424e143d1a627 Mon Sep 17 00:00:00 2001 From: Simon Shillaker Date: Wed, 9 Feb 2022 14:11:02 +0000 Subject: [PATCH 2/2] Fixing up broken tests --- include/faabric/util/snapshot.h | 8 +- src/scheduler/Executor.cpp | 4 +- src/util/snapshot.cpp | 36 ++-- tests/test/scheduler/test_executor.cpp | 18 +- .../snapshot/test_snapshot_client_server.cpp | 16 +- tests/test/snapshot/test_snapshot_diffs.cpp | 8 +- tests/test/util/test_snapshot.cpp | 165 +++++++++++------- 7 files changed, 151 insertions(+), 104 deletions(-) diff --git a/include/faabric/util/snapshot.h b/include/faabric/util/snapshot.h index 3cff2ce3f..82f71acb5 100644 --- a/include/faabric/util/snapshot.h +++ b/include/faabric/util/snapshot.h @@ -27,7 +27,7 @@ enum SnapshotDataType enum SnapshotMergeOperation { - Overwrite, + Bytewise, XOR, Sum, Product, @@ -59,7 +59,7 @@ class SnapshotDiff private: SnapshotDataType dataType = SnapshotDataType::Raw; - SnapshotMergeOperation operation = SnapshotMergeOperation::Overwrite; + SnapshotMergeOperation operation = SnapshotMergeOperation::Bytewise; uint32_t offset = 0; std::vector data; }; @@ -80,7 +80,7 @@ class SnapshotMergeRegion uint32_t offset = 0; size_t length = 0; SnapshotDataType dataType = SnapshotDataType::Raw; - SnapshotMergeOperation operation = SnapshotMergeOperation::Overwrite; + SnapshotMergeOperation operation = SnapshotMergeOperation::Bytewise; SnapshotMergeRegion() = default; @@ -234,7 +234,7 @@ class SnapshotData SnapshotDataType dataType, SnapshotMergeOperation operation); - void fillGapsWithOverwriteRegions(); + void fillGapsWithBytewiseRegions(); void clearMergeRegions(); diff --git a/src/scheduler/Executor.cpp b/src/scheduler/Executor.cpp index a6b044cfc..a421f0f50 100644 --- a/src/scheduler/Executor.cpp +++ b/src/scheduler/Executor.cpp @@ -184,7 +184,7 @@ std::vector> Executor::executeThreads( std::vector dirtyRegions = tracker.getBothDirtyPages(memView); // Apply changes to snapshot - snap->fillGapsWithOverwriteRegions(); + snap->fillGapsWithBytewiseRegions(); std::vector updates = snap->diffWithDirtyRegions(memView, dirtyRegions); @@ -622,7 +622,7 @@ void Executor::threadPoolThread(int threadPoolIdx) std::string mainThreadSnapKey = faabric::util::getMainThreadSnapshotKey(msg); auto snap = reg.getSnapshot(mainThreadSnapKey); - snap->fillGapsWithOverwriteRegions(); + snap->fillGapsWithBytewiseRegions(); // Compare snapshot with all dirty regions for this executor std::vector diffs; diff --git a/src/util/snapshot.cpp b/src/util/snapshot.cpp index 2acb1696e..d036880c4 100644 --- a/src/util/snapshot.cpp +++ b/src/util/snapshot.cpp @@ -47,7 +47,7 @@ void diffArrayRegions(std::vector& snapshotDiffs, // Finished on byte before diffInProgress = false; snapshotDiffs.emplace_back(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, diffStart, b.subspan(diffStart, i - diffStart)); } @@ -56,7 +56,7 @@ void diffArrayRegions(std::vector& snapshotDiffs, // If we finish with a diff in progress, add it if (diffInProgress) { snapshotDiffs.emplace_back(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, diffStart, b.subspan(diffStart, endOffset - diffStart)); } @@ -222,14 +222,14 @@ void SnapshotData::addMergeRegion(uint32_t offset, mergeRegions.emplace_back(offset, length, dataType, operation); } -void SnapshotData::fillGapsWithOverwriteRegions() +void SnapshotData::fillGapsWithBytewiseRegions() { faabric::util::FullLock lock(snapMx); SnapshotMergeOperation fillType; SystemConfig& conf = faabric::util::getSystemConfig(); - if (conf.diffingMode == "overwrite") { - fillType = SnapshotMergeOperation::Overwrite; + if (conf.diffingMode == "bytewise") { + fillType = SnapshotMergeOperation::Bytewise; } else if (conf.diffingMode == "xor") { fillType = SnapshotMergeOperation::XOR; } else { @@ -240,7 +240,7 @@ void SnapshotData::fillGapsWithOverwriteRegions() // If there's no merge regions, just do one big one (note, zero length means // fill all space if (mergeRegions.empty()) { - SPDLOG_TRACE("Filling gap with single overwrite merge region"); + SPDLOG_TRACE("Filling gap with single bytewise merge region"); mergeRegions.emplace_back(0, 0, SnapshotDataType::Raw, fillType); return; @@ -262,7 +262,7 @@ void SnapshotData::fillGapsWithOverwriteRegions() uint32_t regionLen = r.offset - lastRegionEnd; - SPDLOG_TRACE("Filling gap with overwrite merge region {}-{}", + SPDLOG_TRACE("Filling gap with bytewise merge region {}-{}", lastRegionEnd, lastRegionEnd + regionLen); @@ -348,7 +348,7 @@ int SnapshotData::writeQueuedDiffs() } if (diff.getOperation() == - faabric::util::SnapshotMergeOperation::Overwrite) { + faabric::util::SnapshotMergeOperation::Bytewise) { writeData(diff.getData(), diff.getOffset()); continue; } @@ -460,7 +460,7 @@ std::vector SnapshotData::getTrackedChanges() for (auto [regionBegin, regionEnd] : trackedChanges) { SPDLOG_TRACE("Snapshot dirty {}-{}", regionBegin, regionEnd); diffs.emplace_back(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, regionBegin, d.subspan(regionBegin, regionEnd - regionBegin)); } @@ -477,7 +477,7 @@ std::vector SnapshotData::diffWithDirtyRegions( PROF_START(DiffWithSnapshot) std::vector diffs; - // Always add an overwrite region that covers any extension of the + // Always add a bytewise region that covers any extension of the // updated data if (updated.size() > size) { PROF_START(ExtensionDiff) @@ -485,7 +485,7 @@ std::vector SnapshotData::diffWithDirtyRegions( "Adding diff to extend snapshot from {} to {}", size, updated.size()); size_t extensionLen = updated.size() - size; diffs.emplace_back(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, size, updated.subspan(size, extensionLen)); PROF_END(ExtensionDiff) @@ -564,8 +564,8 @@ std::string snapshotMergeOpStr(SnapshotMergeOperation op) case (SnapshotMergeOperation::Min): { return "Min"; } - case (SnapshotMergeOperation::Overwrite): { - return "Overwrite"; + case (SnapshotMergeOperation::Bytewise): { + return "Bytewise"; } case (SnapshotMergeOperation::Product): { return "Product"; @@ -651,10 +651,10 @@ void SnapshotMergeRegion::addDiffs(std::vector& diffs, } startPage += std::distance(startIt, foundIt); - // Overwrites and XORs both deal with overwriting bytes without any - // other logic. Overwrites will filter in only the modified bytes, + // Bytewise and XOR both deal with overwriting bytes without any + // other logic. Bytewise will filter in only the modified bytes, // whereas XOR will transmit the XOR of the whole page and the original - if (operation == SnapshotMergeOperation::Overwrite || + if (operation == SnapshotMergeOperation::Bytewise || operation == SnapshotMergeOperation::XOR) { // Iterate through pages for (int p = startPage; p < endPage; p++) { @@ -672,7 +672,7 @@ void SnapshotMergeRegion::addDiffs(std::vector& diffs, SPDLOG_TRACE("Checking page {} {}-{}", p, startByte, endByte); - if (operation == SnapshotMergeOperation::Overwrite) { + if (operation == SnapshotMergeOperation::Bytewise) { diffArrayRegions( diffs, startByte, endByte, originalData, updatedData); } else { @@ -695,7 +695,7 @@ void SnapshotMergeRegion::addDiffs(std::vector& diffs, } } - // This is the end of the XOR/overwrite diff + // This is the end of the XOR/bytewise diff return; } diff --git a/tests/test/scheduler/test_executor.cpp b/tests/test/scheduler/test_executor.cpp index 47e431875..d35007f10 100644 --- a/tests/test/scheduler/test_executor.cpp +++ b/tests/test/scheduler/test_executor.cpp @@ -644,6 +644,10 @@ TEST_CASE_METHOD(TestExecutorFixture, { int nThreads = 4; + SECTION("XOR diffs") { conf.diffingMode = "xor"; } + + SECTION("Bytewise diffs") { conf.diffingMode = "bytewise"; } + // Sanity check memory size REQUIRE(TEST_EXECUTOR_DEFAULT_MEMORY_SIZE > nThreads * HOST_PAGE_SIZE); @@ -691,9 +695,17 @@ TEST_CASE_METHOD(TestExecutorFixture, REQUIRE(diffList.at(i).getOffset() == i * faabric::util::HOST_PAGE_SIZE); - std::vector expected = { (uint8_t)(i + 1), - (uint8_t)(i + 2), - (uint8_t)(i + 3) }; + std::vector expected; + if (conf.diffingMode == "xor") { + // In XOR mode we'll get the whole page back with a modification at + // the start + expected = std::vector(HOST_PAGE_SIZE, 0); + expected[0] = i + 1; + expected[1] = i + 2; + expected[2] = i + 3; + } else { + expected = { (uint8_t)(i + 1), (uint8_t)(i + 2), (uint8_t)(i + 3) }; + } std::vector actual = diffList.at(i).getDataCopy(); diff --git a/tests/test/snapshot/test_snapshot_client_server.cpp b/tests/test/snapshot/test_snapshot_client_server.cpp index 2d712ef67..7bd183271 100644 --- a/tests/test/snapshot/test_snapshot_client_server.cpp +++ b/tests/test/snapshot/test_snapshot_client_server.cpp @@ -87,7 +87,7 @@ TEST_CASE_METHOD(SnapshotClientServerFixture, // Add merge regions to one std::vector mergeRegions = { { 123, 1234, SnapshotDataType::Int, SnapshotMergeOperation::Sum }, - { 345, 3456, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite } + { 345, 3456, SnapshotDataType::Raw, SnapshotMergeOperation::Bytewise } }; for (const auto& m : mergeRegions) { @@ -167,7 +167,7 @@ TEST_CASE_METHOD(SnapshotClientServerFixture, std::vector mergeRegions = { { 123, 1234, SnapshotDataType::Int, SnapshotMergeOperation::Sum }, - { 345, 3456, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite } + { 345, 3456, SnapshotDataType::Raw, SnapshotMergeOperation::Bytewise } }; for (const auto& m : mergeRegions) { @@ -183,12 +183,12 @@ TEST_CASE_METHOD(SnapshotClientServerFixture, REQUIRE(snap->getQueuedDiffsCount() == 0); SnapshotDiff diffA1(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetA1, diffDataA1); SnapshotDiff diffA2(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetA2, diffDataA2); @@ -207,17 +207,17 @@ TEST_CASE_METHOD(SnapshotClientServerFixture, std::vector diffDataB3 = { 1, 1, 2, 2, 3, 3, 4, 4 }; SnapshotDiff diffB1(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetB1, diffDataB1); SnapshotDiff diffB2(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetB2, diffDataB2); SnapshotDiff diffB3(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetB3, diffDataB3); @@ -328,7 +328,7 @@ TEST_CASE_METHOD(SnapshotClientServerFixture, std::vector diffData; std::vector expectedData; - SnapshotMergeOperation operation = SnapshotMergeOperation::Overwrite; + SnapshotMergeOperation operation = SnapshotMergeOperation::Bytewise; SnapshotDataType dataType = SnapshotDataType::Raw; SECTION("Integer") diff --git a/tests/test/snapshot/test_snapshot_diffs.cpp b/tests/test/snapshot/test_snapshot_diffs.cpp index 251cde6f9..bb7c34fb2 100644 --- a/tests/test/snapshot/test_snapshot_diffs.cpp +++ b/tests/test/snapshot/test_snapshot_diffs.cpp @@ -117,7 +117,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot diffs", "[snapshot]") snap->addMergeRegion(offsetA, dataA.size(), SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); // Deliberately add merge regions out of order @@ -131,7 +131,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot diffs", "[snapshot]") snap->addMergeRegion(regionOffsetC, dataC.size(), SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); // Two changes in single merge region std::vector dataB1 = { 4, 5, 6 }; @@ -144,7 +144,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot diffs", "[snapshot]") snap->addMergeRegion(offsetB1, (offsetB2 - offsetB1) + dataB2.size() + 10, SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); // Merge region within a change std::vector dataD = { 1, 1, 2, 2, 3, 3, 4 }; @@ -157,7 +157,7 @@ TEST_CASE_METHOD(SnapshotTestFixture, "Test snapshot diffs", "[snapshot]") snap->addMergeRegion(regionOffsetD, regionSizeD, SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); // Write some data to the region that exceeds the size of the original. // Anything outside the original snapshot should be marked as changed. diff --git a/tests/test/util/test_snapshot.cpp b/tests/test/util/test_snapshot.cpp index 328c0899f..9247e2b63 100644 --- a/tests/test/util/test_snapshot.cpp +++ b/tests/test/util/test_snapshot.cpp @@ -75,10 +75,10 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, uint32_t offsetC = 10 * HOST_PAGE_SIZE; SnapshotDiff diffA( - SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite, offsetA, dataA); + SnapshotDataType::Raw, SnapshotMergeOperation::Bytewise, offsetA, dataA); SnapshotDiff diffB(SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetB, std::span(memB.get(), dataB.size())); @@ -93,8 +93,8 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, REQUIRE(diffB.getDataType() == SnapshotDataType::Raw); REQUIRE(diffC.getDataType() == SnapshotDataType::Int); - REQUIRE(diffA.getOperation() == SnapshotMergeOperation::Overwrite); - REQUIRE(diffB.getOperation() == SnapshotMergeOperation::Overwrite); + REQUIRE(diffA.getOperation() == SnapshotMergeOperation::Bytewise); + REQUIRE(diffB.getOperation() == SnapshotMergeOperation::Bytewise); REQUIRE(diffC.getOperation() == SnapshotMergeOperation::Sum); REQUIRE(diffA.getDataCopy() == dataA); @@ -507,7 +507,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, faabric::util::SnapshotDataType expectedDataType = faabric::util::SnapshotDataType::Raw; faabric::util::SnapshotMergeOperation operation = - faabric::util::SnapshotMergeOperation::Overwrite; + faabric::util::SnapshotMergeOperation::Bytewise; size_t dataLength = 0; size_t regionLength = 0; @@ -773,13 +773,13 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, dataLength = sizeof(bool); regionLength = sizeof(bool); - SECTION("Bool overwrite") + SECTION("Bool bytewise") { originalValue = true; finalValue = false; diffValue = false; - operation = faabric::util::SnapshotMergeOperation::Overwrite; + operation = faabric::util::SnapshotMergeOperation::Bytewise; } originalData = faabric::util::valueToBytes(originalValue); @@ -796,16 +796,16 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, dataType = faabric::util::SnapshotDataType::Raw; - SECTION("Overwrite") + SECTION("Bytewise") { regionLength = dataLength; - operation = faabric::util::SnapshotMergeOperation::Overwrite; + operation = faabric::util::SnapshotMergeOperation::Bytewise; } - SECTION("Overwrite unspecified length") + SECTION("Bytewise unspecified length") { regionLength = 0; - operation = faabric::util::SnapshotMergeOperation::Overwrite; + operation = faabric::util::SnapshotMergeOperation::Bytewise; } SECTION("Ignore") @@ -879,7 +879,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, faabric::util::SnapshotDataType dataType = faabric::util::SnapshotDataType::Raw; faabric::util::SnapshotMergeOperation operation = - faabric::util::SnapshotMergeOperation::Overwrite; + faabric::util::SnapshotMergeOperation::Bytewise; size_t dataLength = 0; std::string expectedMsg; @@ -1028,19 +1028,19 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, std::vector expectedDiffs = { { SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetA, { dataA.data(), dataA.size() } }, { SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetB, { dataB.data(), dataB.size() } }, { SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetC, { dataC.data(), dataC.size() } }, { SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, offsetD, { dataD.data(), dataD.size() } }, }; @@ -1049,7 +1049,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, snap->addMergeRegion(0, offsetD + dataD.size() + 20, SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); // Check number of diffs tracker.stopTracking(memView); @@ -1080,10 +1080,10 @@ void doFillGapsChecks(SnapshotMergeOperation op) SECTION("One region at start") { snap->addMergeRegion( - 0, 100, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); + 0, 100, SnapshotDataType::Raw, SnapshotMergeOperation::Bytewise); expectedRegions.emplace_back( - 0, 100, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); + 0, 100, SnapshotDataType::Raw, SnapshotMergeOperation::Bytewise); expectedRegions.emplace_back(100, 0, SnapshotDataType::Raw, op); } @@ -1093,7 +1093,7 @@ void doFillGapsChecks(SnapshotMergeOperation op) snap->addMergeRegion(snapSize - 100, 100, SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); expectedRegions.emplace_back( 0, snapSize - 100, SnapshotDataType::Raw, op); @@ -1101,7 +1101,7 @@ void doFillGapsChecks(SnapshotMergeOperation op) expectedRegions.emplace_back((uint32_t)snapSize - 100, 100, SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); } SECTION("Multiple regions") @@ -1118,7 +1118,7 @@ void doFillGapsChecks(SnapshotMergeOperation op) snap->addMergeRegion(HOST_PAGE_SIZE + 200, HOST_PAGE_SIZE, SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); expectedRegions.emplace_back(0, 100, SnapshotDataType::Raw, op); @@ -1144,13 +1144,13 @@ void doFillGapsChecks(SnapshotMergeOperation op) expectedRegions.emplace_back((uint32_t)HOST_PAGE_SIZE + 200, (uint32_t)HOST_PAGE_SIZE, SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); expectedRegions.emplace_back( (uint32_t)(2 * HOST_PAGE_SIZE) + 200, 0, SnapshotDataType::Raw, op); } - snap->fillGapsWithOverwriteRegions(); + snap->fillGapsWithBytewiseRegions(); std::vector actualRegions = snap->getMergeRegions(); @@ -1170,10 +1170,10 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, "[snapshot][util]") { SystemConfig& conf = getSystemConfig(); - SECTION("Overwrite") + SECTION("Bytewise") { - conf.diffingMode = "overwrite"; - doFillGapsChecks(SnapshotMergeOperation::Overwrite); + conf.diffingMode = "bytewise"; + doFillGapsChecks(SnapshotMergeOperation::Bytewise); } SECTION("XOR") @@ -1192,7 +1192,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, bool failed = false; try { - snap->fillGapsWithOverwriteRegions(); + snap->fillGapsWithBytewiseRegions(); } catch (std::runtime_error& ex) { std::string expected = "Unsupported diffing mode"; REQUIRE(ex.what() == expected); @@ -1212,14 +1212,14 @@ TEST_CASE("Test sorting snapshot merge regions", "[snapshot][util]") regions.emplace_back( 50, 20, SnapshotDataType::Double, SnapshotMergeOperation::Sum); regions.emplace_back( - 5, 20, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite); + 5, 20, SnapshotDataType::Raw, SnapshotMergeOperation::Bytewise); regions.emplace_back( 30, 20, SnapshotDataType::Float, SnapshotMergeOperation::Max); std::sort(regions.begin(), regions.end()); std::vector expected = { - { 5, 20, SnapshotDataType::Raw, SnapshotMergeOperation::Overwrite }, + { 5, 20, SnapshotDataType::Raw, SnapshotMergeOperation::Bytewise }, { 10, 20, SnapshotDataType::Double, SnapshotMergeOperation::Sum }, { 30, 20, SnapshotDataType::Float, SnapshotMergeOperation::Max }, { 50, 20, SnapshotDataType::Double, SnapshotMergeOperation::Sum }, @@ -1255,12 +1255,12 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, // Add a couple of merge regions on each page, which should be skipped as // they won't overlap any changes for (int i = 0; i < snapPages; i++) { - // Overwrite region at bottom of page - int skippedOverwriteOffset = i * HOST_PAGE_SIZE; - snap->addMergeRegion(skippedOverwriteOffset, + // Bytewise region at bottom of page + int skippedBytewiseOffset = i * HOST_PAGE_SIZE; + snap->addMergeRegion(skippedBytewiseOffset, 10, faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite); + faabric::util::SnapshotMergeOperation::Bytewise); // Sum region near top of page int skippedSumOffset = @@ -1271,20 +1271,19 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, faabric::util::SnapshotMergeOperation::Sum); } - // Add an overwrite region above the one at the very bottom, and some + // Add a bytewise region above the one at the very bottom, and some // modified data that should be caught by it - uint32_t overwriteAOffset = (2 * HOST_PAGE_SIZE) + 20; - snap->addMergeRegion(overwriteAOffset, + uint32_t bytewiseAOffset = (2 * HOST_PAGE_SIZE) + 20; + snap->addMergeRegion(bytewiseAOffset, 20, faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite); + faabric::util::SnapshotMergeOperation::Bytewise); - std::vector overwriteData(10, 1); - SPDLOG_DEBUG("Mix test writing {} bytes at {}", - overwriteData.size(), - overwriteAOffset); + std::vector bytewiseData(10, 1); + SPDLOG_DEBUG( + "Mix test writing {} bytes at {}", bytewiseData.size(), bytewiseAOffset); std::memcpy( - mem.get() + overwriteAOffset, overwriteData.data(), overwriteData.size()); + mem.get() + bytewiseAOffset, bytewiseData.data(), bytewiseData.size()); expectedDirtyPages[2] = 1; // Add a sum region and modified data that should also cause a diff @@ -1306,9 +1305,9 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, // Check diffs std::vector expectedDiffs = { { faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite, - overwriteAOffset, - { BYTES(overwriteData.data()), overwriteData.size() } }, + faabric::util::SnapshotMergeOperation::Bytewise, + bytewiseAOffset, + { BYTES(bytewiseData.data()), bytewiseData.size() } }, { faabric::util::SnapshotDataType::Int, faabric::util::SnapshotMergeOperation::Sum, sumOffset, @@ -1383,7 +1382,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, // Diff should just be the whole of the updated memory expectedDiffs = { { faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite, + faabric::util::SnapshotMergeOperation::Bytewise, (uint32_t)snapSize, expectedData } }; } @@ -1399,7 +1398,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, // Diff should just be the whole of the updated memory expectedDiffs = { { faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite, + faabric::util::SnapshotMergeOperation::Bytewise, (uint32_t)snapSize, expectedData } }; } @@ -1416,7 +1415,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, // Diff should be the whole region of updated memory expectedDiffs = { { faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite, + faabric::util::SnapshotMergeOperation::Bytewise, (uint32_t)snapSize, expectedData } }; } @@ -1445,11 +1444,11 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, std::memset(expectedDataTwo.data(), 2, overshootSize); expectedDiffs = { { faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite, + faabric::util::SnapshotMergeOperation::Bytewise, (uint32_t)snapSize, expectedDataTwo }, { faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite, + faabric::util::SnapshotMergeOperation::Bytewise, changeOffset, expectedDataOne } }; } @@ -1461,7 +1460,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, snap->addMergeRegion(mergeRegionStart, 0, faabric::util::SnapshotDataType::Raw, - faabric::util::SnapshotMergeOperation::Overwrite); + faabric::util::SnapshotMergeOperation::Bytewise); tracker.stopTracking(memView); tracker.stopThreadLocalTracking(memView); @@ -1550,17 +1549,17 @@ TEST_CASE_METHOD(DirtyTrackingTestFixture, snap->addMergeRegion(offsetA, dataA.size(), SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); snap->addMergeRegion(offsetB, dataB.size(), SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); snap->addMergeRegion(offsetC, dataC.size(), SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite); + SnapshotMergeOperation::Bytewise); // Write some initial data to snapshot snap->copyInData(dataA); @@ -1722,7 +1721,7 @@ TEST_CASE("Test diffing byte array regions", "[util][snapshot]") for (auto p : expected) { expectedDiffs.emplace_back( SnapshotDataType::Raw, - SnapshotMergeOperation::Overwrite, + SnapshotMergeOperation::Bytewise, p.first, std::span(b.data() + p.first, b.data() + p.first + p.second)); @@ -1798,7 +1797,11 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, "Test diffing snapshot memory with none tracking", "[snapshot][util]") { - faabric::util::getSystemConfig().dirtyTrackingMode = "none"; + conf.dirtyTrackingMode = "none"; + + SECTION("XOR diffs") { conf.diffingMode = "xor"; } + + SECTION("Bytewise diffs") { conf.diffingMode = "bytewise"; } // Create snapshot int snapPages = 4; @@ -1841,7 +1844,7 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, REQUIRE(dirtyPages == expectedDirtyPages); // Diff with snapshot - snap->fillGapsWithOverwriteRegions(); + snap->fillGapsWithBytewiseRegions(); std::vector actualRegions = snap->getMergeRegions(); REQUIRE(actualRegions.size() == 1); REQUIRE(actualRegions.at(0).offset == 0); @@ -1850,15 +1853,47 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture, std::vector actual = snap->diffWithDirtyRegions(memView, dirtyPages); - REQUIRE(actual.size() == 2); - SnapshotDiff diffA = actual.at(0); - SnapshotDiff diffB = actual.at(1); + if (conf.diffingMode == "bytewise") { + REQUIRE(actual.size() == 2); + SnapshotDiff diffA = actual.at(0); + SnapshotDiff diffB = actual.at(1); - REQUIRE(diffA.getOffset() == offsetA); - REQUIRE(diffB.getOffset() == offsetB); + REQUIRE(diffA.getOffset() == offsetA); + REQUIRE(diffB.getOffset() == offsetB); - REQUIRE(diffA.getDataCopy() == dataA); - REQUIRE(diffB.getDataCopy() == dataB); + REQUIRE(diffA.getDataCopy() == dataA); + REQUIRE(diffB.getDataCopy() == dataB); + } else if (conf.diffingMode == "xor") { + REQUIRE(actual.size() == snapPages); + + std::vector blankPage(HOST_PAGE_SIZE, 0); + + SnapshotDiff diffPage1 = actual.at(0); + SnapshotDiff diffPage2 = actual.at(1); + SnapshotDiff diffPage3 = actual.at(2); + SnapshotDiff diffPage4 = actual.at(3); + + REQUIRE(diffPage1.getOffset() == 0); + REQUIRE(diffPage2.getOffset() == HOST_PAGE_SIZE); + REQUIRE(diffPage3.getOffset() == HOST_PAGE_SIZE * 2); + REQUIRE(diffPage4.getOffset() == HOST_PAGE_SIZE * 3); + + // Pages with no diffs will just be zeroes + REQUIRE(diffPage2.getDataCopy() == blankPage); + REQUIRE(diffPage4.getDataCopy() == blankPage); + + // Other pages will have diffs in place + std::vector expectedA = blankPage; + std::vector expectedB = blankPage; + std::copy(dataA.begin(), dataA.end(), expectedA.begin()); + std::copy(dataB.begin(), dataB.end(), expectedB.begin() + 1); + + REQUIRE(diffPage1.getDataCopy() == expectedA); + REQUIRE(diffPage3.getDataCopy() == expectedB); + + } else { + FAIL(); + } } TEST_CASE_METHOD(DirtyTrackingTestFixture, "Test XOR diffs", "[util][snapshot]")