Merge regions overhaul #201

Shillaker · 2021-12-20T17:26:00Z

This PR addresses the following problems:

Merge regions were not being transferred across hosts (see below).
Whitelisting merge regions for big applications became unfeasible (impossible to specify all places where the memory might change).
Merge region code only supported Raw and Int types (needed more types including Bool, Long, Float and Double).

When it comes to transferring merge regions across hosts, we have to consider three scenarios where snapshot data is being transferred:

First time executing a thread on a remote host, in which case the full snapshot is pushed.
Subsequent executions of threads on a remote host (in which case just an update to the snapshot is sent)
Remote hosts returning snapshot diffs after executing a thread.

In the first two, we are not currently including merge regions, which means, if the main thread on the main host sets up some merge regions, they are not respected by remote hosts.

Changes:

Distinguish between a snapshot update (2) and simple snapshot diffs (3).
Include snapshot merge regions in (1) and (2).
Reintroduce Ignore merge regions to blacklist regions of memory (e.g. guard pages), then automatically fill the rest of the snapshot memory with Overwrite regions.
Add double, float, bool and long support.

src/flat/faabric.fbs

Shillaker · 2021-12-21T11:06:10Z

@KubaSz I added you as a reviewer given that you looked at the previous snapshot stuff, but feel free to ignore if you don't have scope to keep checking this stuff 😄

eigenraven · 2021-12-21T11:18:41Z

I don't have much time to review this, but from a quick glance I can offer two quick comments:

const std::vector<T>& -> std::span<const T> in arguments
when pushing/emplacing back to a vector in a loop, make sure there's a reserve before the loop with the number of iterations

Shillaker · 2021-12-21T11:19:56Z

Ok nice, cheers and have a good Xmas!

csegarragonz

LGTM. Just pointed out two very minor issues, feel free to ignore them.

I have deliberately marked all occurings I have found to aid in the re-factor if you end up going for it.

csegarragonz · 2021-12-29T15:44:38Z

include/faabric/util/snapshot.h

 };

+template<typename T>
+inline bool calculateDiffValue(const uint8_t* original,


It's not immediate to tell the difference between calculateDiffValue and applyDiffValue, could we add a comment explaining what each one does, or how they differ?

Also, just to double check, can the uint8_t pointers here can't be converted to std::spans?

csegarragonz · 2021-12-29T15:47:14Z

src/runner/FaabricMain.cpp

@@ -25,6 +25,8 @@ void FaabricMain::startBackground()
    // Crash handler
    faabric::util::setUpCrashHandler();

+    PROF_BEGIN


Same point made in the Faasm PR re. comitting profiling macros. I would personally remove them, but feel free to ignore.

csegarragonz · 2021-12-29T15:50:29Z

src/snapshot/SnapshotRegistry.cpp


 #include <sys/mman.h>

 namespace faabric::snapshot {
 std::shared_ptr<faabric::util::SnapshotData> SnapshotRegistry::getSnapshot(
  const std::string& key)
 {
+    PROF_START(GetSnapshot)


Same point re. profiling (feel free to ignore).

csegarragonz · 2021-12-29T15:53:01Z

src/util/memory.cpp

+{
+    static ClearRefsWrapper wrap;
+
+    PROF_START(ResetDirty)


Same here re. profiling (feel free to ignore).

csegarragonz · 2021-12-29T15:58:33Z

src/util/snapshot.cpp

+{
+    // Note we only need a shared lock here as we are not modifying data and the
+    // OS will handle synchronisation of the mapping itself
+    PROF_START(MapSnapshot)


Same point re. profiling

csegarragonz · 2021-12-29T15:59:53Z

src/util/snapshot.cpp

@@ -337,6 +400,7 @@ MemoryView::MemoryView(std::span<const uint8_t> dataIn)

 std::vector<SnapshotDiff> MemoryView::getDirtyRegions()
 {
+    PROF_START(GetDirtyRegions)


csegarragonz · 2021-12-29T16:04:06Z

tests/test/snapshot/test_snapshot_diffs.cpp

@@ -36,9 +36,13 @@ TEST_CASE_METHOD(SnapshotTestFixture,

    int sharedMemPages = 8;
    size_t sharedMemSize = sharedMemPages * HOST_PAGE_SIZE;
-    MemoryRegion sharedMem = allocateSharedMemory(sharedMemSize);
+    MemoryRegion sharedMem = allocatePrivateMemory(sharedMemSize);


Just to double check, the variable is called sharedMem and is defined by calling allocatePrivateMemory which feels odd.

csegarragonz · 2021-12-29T16:04:38Z

tests/test/util/test_memory.cpp

@@ -433,7 +433,7 @@ TEST_CASE("Test mapping memory", "[util]")
 TEST_CASE("Test mapping memory fails with invalid fd", "[util]")
 {
    size_t memSize = 10 * HOST_PAGE_SIZE;
-    MemoryRegion sharedMem = allocateSharedMemory(memSize);
+    MemoryRegion sharedMem = allocatePrivateMemory(memSize);


Same point re. variable wording.

csegarragonz · 2021-12-29T16:05:38Z

tests/test/util/test_snapshot.cpp

@@ -179,12 +179,12 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture,
    std::vector<uint8_t> expectedSnapMem = snap->getDataCopy();

    // Set up two shared mem regions
-    MemoryRegion sharedMemA = allocateSharedMemory(snapSize);
-    MemoryRegion sharedMemB = allocateSharedMemory(snapSize);
+    MemoryRegion sharedMemA = allocatePrivateMemory(snapSize);


Same point re. variable naming throughout this test.

csegarragonz · 2021-12-29T16:06:51Z

tests/test/util/test_snapshot.cpp

@@ -590,6 +592,214 @@ TEST_CASE_METHOD(SnapshotMergeTestFixture,
        expectedData = faabric::util::valueToBytes<int>(diffValue);
    }

+    SECTION("Long")


Nice nesting of tests here 👍

Shillaker · 2021-12-29T16:16:17Z

I'll merge this in the interest of moving things along and fixing downstream, then I'll put the refactoring into #210

Client/ server for pushing snapshot merge regions

55d8ddf

Shillaker commented Dec 20, 2021

View reviewed changes

src/flat/faabric.fbs Show resolved Hide resolved

Shillaker added 2 commits December 20, 2021 17:36

Reinstate clearing of merge regions

ff0aaaf

Dist tests and test

02f7daa

Shillaker marked this pull request as ready for review December 21, 2021 10:54

Shillaker mentioned this pull request Dec 21, 2021

Snapshot and shared memory updates faasm/faasm#548

Merged

Shillaker requested review from csegarragonz and eigenraven December 21, 2021 11:05

Shillaker removed the request for review from eigenraven December 21, 2021 11:20

Add reserves where possible

5355bf9

Shillaker marked this pull request as draft December 21, 2021 11:44

Shillaker removed the request for review from csegarragonz December 21, 2021 11:44

Support more merge ops

2066535

Shillaker changed the title ~~Push snapshot merge regions with snapshots and snapshot diffs~~ Merge regions overhaul Dec 21, 2021

Shillaker added 12 commits December 21, 2021 14:47

Tests for more datatypes

2fe3cc0

Added long type

832982a

Add trace logging to difsf

19cc7d3

Remove register snapshot if exists

95d32b8

Formatting

1903f42

More snapshot logging

94ed106

Add filling in gaps capacity

f1e5bf8

Default to shared

1c2ed89

Use spans where possible

69094d7

Logging and reinstate MR clearing

6dcf8ce

Add ignore diffs

0406a3f

Fix bug in overflowing original snap data

8e21f28

Shillaker marked this pull request as ready for review December 27, 2021 14:03

Remove unnecessary merge regions in dist tests

4ec33ce

Shillaker self-assigned this Dec 27, 2021

Shillaker added 4 commits December 28, 2021 10:00

More snapshot tests

5d6b7bb

Merge branch 'master' into mrs-with-snaps

e501035

Fix uint mismatch

3753ae2

Self review

6f2ed5b

Shillaker requested review from csegarragonz and removed request for csegarragonz December 28, 2021 13:32

Shillaker added 3 commits December 28, 2021 17:29

Profiling

086359d

Use spans

54b4403

Use static initialised for resetting dirty pages

58ca527

Shillaker requested a review from csegarragonz December 29, 2021 11:46

csegarragonz approved these changes Dec 29, 2021

View reviewed changes

Shillaker merged commit 0e7a32c into master Dec 29, 2021

Shillaker deleted the mrs-with-snaps branch December 29, 2021 16:16

csegarragonz mentioned this pull request Feb 23, 2022

Add task to generate release body #233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge regions overhaul #201

Merge regions overhaul #201

Shillaker commented Dec 20, 2021 •

edited

Loading

Shillaker commented Dec 21, 2021

eigenraven commented Dec 21, 2021 •

edited

Loading

Shillaker commented Dec 21, 2021

csegarragonz left a comment

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

csegarragonz Dec 29, 2021

Shillaker commented Dec 29, 2021

Merge regions overhaul #201

Merge regions overhaul #201

Conversation

Shillaker commented Dec 20, 2021 • edited Loading

Shillaker commented Dec 21, 2021

eigenraven commented Dec 21, 2021 • edited Loading

Shillaker commented Dec 21, 2021

csegarragonz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shillaker commented Dec 29, 2021

Shillaker commented Dec 20, 2021 •

edited

Loading

eigenraven commented Dec 21, 2021 •

edited

Loading