Skip to content

[Performance] Optimize backupFilesDict to use ConcurrentDictionary and eliminate reallocation overhead #16826

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Copilot wants to merge 5 commits into
base: master
Choose a base branch
from
+21 −10
Open

[Performance] Optimize backupFilesDict to use ConcurrentDictionary and eliminate reallocation overhead #16826

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0035a2f
Initial plan
Copilot Jan 12, 2026
8bdbb0f
Optimize backupFilesDict to use ConcurrentDictionary and avoid freque…
Copilot Jan 12, 2026
60b2819
Fix backup file list population and use ToArray for better performance
Copilot Jan 12, 2026
0a1ba3b
Add comment explaining ToArray() usage for safety
Copilot Jan 12, 2026
8999be7
Potential fix for pull request finding 'Missed 'readonly' opportunity'
QilongTang Jan 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 21 additions & 10 deletions src/DynamoCore/Models/DynamoModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.ComponentModel;
Expand Down Expand Up @@ -135,7 +136,7 @@ public partial class DynamoModel : IDynamoModel, IDisposable, IEngineControllerM
private readonly PathManager pathManager;
private WorkspaceModel currentWorkspace;
private Timer backupFilesTimer;
private Dictionary<Guid, string> backupFilesDict = new Dictionary<Guid, string>();
private readonly ConcurrentDictionary<Guid, string> backupFilesDict = new ConcurrentDictionary<Guid, string>();
internal readonly Stopwatch stopwatch = Stopwatch.StartNew();

/// <summary>
Expand Down Expand Up @@ -2685,33 +2686,43 @@ protected void SaveBackupFiles(object state)

OnRequestDispatcherBeginInvoke(() =>
{
// tempDict stores the list of backup files and their corresponding workspaces IDs
// when the last auto-save operation happens. Now the IDs will be used to know
// whether some workspaces have already been backed up. If so, those workspaces won't be
// backed up again.
var tempDict = new Dictionary<Guid, string>(backupFilesDict);
backupFilesDict.Clear();
Copy link
Copy Markdown
Contributor

@QilongTang QilongTang Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the dictionary should be fairly small size here containing only entries about opened workspaces. Given currently Dynamo does not allow multiple workspaces opened, seems the performance/memory impact here would be minimal.

PreferenceSettings.BackupFiles.Clear();
// Get the current set of workspace GUIDs that need to be tracked
var currentWorkspaceGuids = new HashSet<Guid>(Workspaces.Select(w => w.Guid));

// Remove entries for workspaces that no longer exist.
// ToArray() is used to materialize the collection before removal to avoid
// potential issues with collection modification during enumeration.
var guidsToRemove = backupFilesDict.Keys.Where(guid => !currentWorkspaceGuids.Contains(guid)).ToArray();
Comment on lines +2689 to +2695
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a new HashSet<Guid> on every backup operation introduces unnecessary allocation overhead. Consider caching this collection if Workspaces doesn't change frequently between backups, or evaluate whether the Contains check could be replaced with a direct dictionary key lookup pattern to avoid the extra collection.

Suggested change
// Get the current set of workspace GUIDs that need to be tracked
var currentWorkspaceGuids = new HashSet<Guid>(Workspaces.Select(w => w.Guid));
// Remove entries for workspaces that no longer exist.
// ToArray() is used to materialize the collection before removal to avoid
// potential issues with collection modification during enumeration.
var guidsToRemove = backupFilesDict.Keys.Where(guid => !currentWorkspaceGuids.Contains(guid)).ToArray();
// Remove entries for workspaces that no longer exist.
// ToArray() is used to materialize the collection before removal to avoid
// potential issues with collection modification during enumeration.
var guidsToRemove = backupFilesDict.Keys
.Where(guid => !Workspaces.Any(w => w.Guid == guid))
.ToArray();

Copilot uses AI. Check for mistakes.
foreach (var guid in guidsToRemove)
{
backupFilesDict.TryRemove(guid, out _);
Comment on lines +2693 to +2698
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ToArray() materializes the entire filtered collection before any removal. For large workspace sets, consider using a foreach loop directly over the filtered keys with a defensive copy (e.g., backupFilesDict.Keys.ToArray()) to avoid the intermediate Where allocation.

Suggested change
// ToArray() is used to materialize the collection before removal to avoid
// potential issues with collection modification during enumeration.
var guidsToRemove = backupFilesDict.Keys.Where(guid => !currentWorkspaceGuids.Contains(guid)).ToArray();
foreach (var guid in guidsToRemove)
{
backupFilesDict.TryRemove(guid, out _);
// Take a defensive snapshot of the keys to avoid issues with
// collection modification during enumeration, and filter in the loop
// to avoid an extra intermediate allocation.
var existingGuids = backupFilesDict.Keys.ToArray();
foreach (var guid in existingGuids)
{
if (!currentWorkspaceGuids.Contains(guid))
{
backupFilesDict.TryRemove(guid, out _);
}

Copilot uses AI. Check for mistakes.
}

foreach (var workspace in Workspaces)
{
// Skip workspaces that don't need backup
if (!workspace.HasUnsavedChanges)
{
if (workspace.Nodes.Any() &&
!workspace.Notes.Any())
continue;

if (tempDict.ContainsKey(workspace.Guid))
if (backupFilesDict.ContainsKey(workspace.Guid))
{
backupFilesDict.Add(workspace.Guid, tempDict[workspace.Guid]);
// Workspace hasn't changed and already has a backup, skip saving
continue;
}
Comment on lines +2710 to 2714
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a potential race condition between ContainsKey and the subsequent dictionary access. In a concurrent context with ConcurrentDictionary, another thread could remove the key between the check and use. Consider using TryGetValue instead to perform an atomic check-and-retrieve operation.

Copilot uses AI. Check for mistakes.
}

// Create new backup for this workspace
var savePath = pathManager.GetBackupFilePath(workspace);
OnRequestWorkspaceBackUpSave(savePath, true);
backupFilesDict[workspace.Guid] = savePath;
Copy link

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the indexer for insertion can overwrite existing entries without indication. For clarity and to match the pattern used elsewhere in this method, consider using backupFilesDict.AddOrUpdate(workspace.Guid, savePath, (key, oldValue) => savePath) or TryAdd followed by an update if needed, to make the intent explicit.

Suggested change
backupFilesDict[workspace.Guid] = savePath;
backupFilesDict.AddOrUpdate(workspace.Guid, savePath, (key, oldValue) => savePath);

Copilot uses AI. Check for mistakes.
Logger.Log(Resources.BackupSavedMsg + ": " + savePath);
}

// Update PreferenceSettings with all current backup files
PreferenceSettings.BackupFiles.Clear();
PreferenceSettings.BackupFiles.AddRange(backupFilesDict.Values);
});
}
Expand Down
Loading