Use dirty page flags instead of dirty regions #219
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Shillaker
changed the title
Shillaker
commented
Feb 3, 2022
Shillaker
commented
Feb 3, 2022
| @@ -19,6 +19,9 @@ | |||
| #include <faabric/util/string_tools.h> | |||
There was a problem hiding this comment.
Changes in this file are a mix of fixing the race condition and updating to the pages vs. regions change. Race condition seemed to manifest in distributed tests on this PR, so fixed it.
Shillaker
commented
Feb 3, 2022
csegarragonz
approved these changes
Feb 3, 2022
src/scheduler/Executor.cpp
Outdated
| } | ||
|
|
||
| if (!exists) { | ||
| std::string snapshotKey = faabric::util::getMainThreadSnapshotKey(msg); | ||
| bool alreadyExists = reg.snapshotExists(snapshotKey); |
There was a problem hiding this comment.
Maybre refactor to snapshotAlreadyExists?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we were taking the list of dirty pages, converting this into a
std::vector<std::pair<uint32_t, uint32_t>>to express a set of regions, then passing this set of regions through the code to the snapshot diffing logic. In fact, it's more efficient and simpler to skip this computation and just directly pass around the list of dirty page flags as astd::vector<char>. This PR reworks the necessary logic and testing around this change.I've also added a couple of other small changes along the way:
std::map<uint32_t, SnapshotMergeRegion>to astd::vector<SnapshotMergeRegion>on each snapshot. This improves insertion performance, and we can sort the vector when necessary (note that sorting was the point of having themapin the first place).nonedirty tracking, which just marks all pages as dirty. This may be optimal for workloads with a small memory where most of that memory will be dirty anyway, so diffing every page outweighs the cost of the dirty tracking.Executor::executeThreads.