Three-way diffing Synchronizer #1707
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
…isk state and game state
…d an error with the sync code exposed by the test
erri120
requested changes
Jul 2, 2024
src/Abstractions/NexusMods.Abstractions.Loadouts.Synchronizers/Rules/Signature.cs
Outdated
Show resolved
Hide resolved
src/Abstractions/NexusMods.Abstractions.Loadouts.Synchronizers/Rules/Signature.cs
Show resolved
Hide resolved
Comment on lines
+6
to
+37
| /// <summary> | ||
| /// A grouping of actions that can be performed on a file. The SyncTree has a `Actions` | ||
| /// field and that field is used to put the items in the tree into these specific groups. | ||
| /// </summary> | ||
| public class SyncActionGroupings | ||
| { | ||
| private ConcurrentDictionary<Actions, ConcurrentBag<SyncTreeNode>> _groupings = new(); | ||
|
|
||
| /// <summary> | ||
| /// Gets the group of nodes that have the specified action. | ||
| /// </summary> | ||
| /// <param name="action"></param> | ||
| public IReadOnlyCollection<SyncTreeNode> this[Actions action] => _groupings.GetOrAdd(action, static _ => []); | ||
|
|
||
| /// <summary> | ||
| /// Adds a node to the groupings based on the actions it has. | ||
| /// </summary> | ||
| /// <param name="node"></param> | ||
| public void Add(SyncTreeNode node) | ||
| { | ||
| foreach (var flag in Enum.GetValues<Actions>()) | ||
| { | ||
| if (node.Actions.HasFlag(flag)) | ||
| AddOne(flag, node); | ||
| } | ||
| } | ||
|
|
||
| private void AddOne(Actions flag, SyncTreeNode node) | ||
| { | ||
| _groupings.GetOrAdd(flag, static _ => []).Add(node); | ||
| } | ||
| } |
There was a problem hiding this comment.
I think there's a ton of performance potential in this class. Without getting into the weeds, the biggest performance wins for the least amount of effort would be the following:
- Using normal
DictionaryandList. I might be mistaken, but it doesn't look like we're using instances of this class in a multithreaded context. - Using a static array instead of calling
Enum.GetValues<Actions>. All theEnummethods actually use reflection, so this is extra slow. - Prepopulating the groupings' dictionary. Following number 2, we can create a dictionary will all keys. This would also mean we're never writing to the dictionary, and can replace
ConcurrentDictionarywith a normalDictionary. - Similar to number 2,
HasFlagcan be replaced withHasFlagFast.
There's more to be done here, but those 4 points together have the best performance/effort ratio.
There was a problem hiding this comment.
Good point, I haven't yet added any parallel code around this but it's fairly trivial to do so. My goal was to try and get this code done so it would un-block people while I'm out on holiday. I plan on using the next few days to do performance improvements and other small feature additions separate PRs
src/Abstractions/NexusMods.Abstractions.Loadouts.Synchronizers/Rules/ActionMapping.cs
Outdated
Show resolved
Hide resolved
erri120
approved these changes
Jul 2, 2024
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.
This switches our synchronizer over to a much simpler, 3-way merge approach. For a in-depth overview of the changes here, see the included ADR 0014.
Due to the change in how this code operates, I had to delete most of the associated benchmarks. It won't be hard to add them back in when I do a performance pass on this code, but it's plenty fast for now. The various tests we have in place for managing and un-managing games all seem to work.
Added a test in SDV that exercises the code that moves files into the matching mods when ingesting.