Skip to content

Improve startup and opening times by speeding up changes to recent files #15759

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
wants to merge 6 commits into
base: master
Choose a base branch
from
+131 −75
Open

Improve startup and opening times by speeding up changes to recent files #15759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f9eb37d
improvements
dimven-adsk Jan 16, 2025
2104a1f
formatting
dimven-adsk Jan 16, 2025
28fc316
Merge remote-tracking branch 'upstream/master' into reope/performance…
dimven-adsk Jan 23, 2025
b82f6f1
implement feedback, bugfix
dimven-adsk Jan 23, 2025
21e5e2f
testing, further optimizations
dimven-adsk Jan 27, 2025
f9ba056
write start page graph properties first
dimven-adsk Jan 31, 2025
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
58 changes: 34 additions & 24 deletions src/DynamoCore/Graph/Workspaces/SerializationConverters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -819,28 +819,46 @@ public override bool CanRead
public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
{
var ws = (WorkspaceModel)value;
bool isCustomNode = value is CustomNodeWorkspaceModel;
var customWorkspace = ws as CustomNodeWorkspaceModel;
var isCustomNode = customWorkspace != null;
var homeWorkspace = ws as HomeWorkspaceModel;
var isHomeWsModel = homeWorkspace != null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change all about reordering the serialized data ?
I think @mjkkirschner mentioned an easier way to control the order : [JsonProperty(Order = 1)]
But seeing numbers here https://github.com/DynamoDS/Dynamo/pull/15759/files#r1932364787 is this still worth doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. Unfortunately, using [JsonProperty(Order = N)] is not applicable in our case because there's a custom json serializer implementation that writes the properties line by line. That's why I modified it this way

writer.WriteStartObject();
writer.WritePropertyName("Uuid");

// Write the start page graph properties first

// Graph Author
writer.WritePropertyName(nameof(WorkspaceModel.Author));
writer.WriteValue(ws.Author);

// Description
writer.WritePropertyName("Description");
if (isCustomNode)
writer.WriteValue((ws as CustomNodeWorkspaceModel).CustomNodeId.ToString());
writer.WriteValue(customWorkspace.Description);
else
writer.WriteValue(ws.Guid.ToString());
writer.WriteValue(ws.Description);

// Thumbnail
if (!isCustomNode && isHomeWsModel)
{
writer.WritePropertyName(nameof(HomeWorkspaceModel.Thumbnail));
writer.WriteValue(homeWorkspace.Thumbnail);
}

writer.WritePropertyName("Uuid");
writer.WriteValue(isCustomNode ?
customWorkspace.CustomNodeId.ToString():
ws.Guid.ToString());

// TODO: revisit IsCustomNode during DYN/DYF convergence
writer.WritePropertyName("IsCustomNode");
writer.WriteValue(value is CustomNodeWorkspaceModel ? true : false);
writer.WriteValue(isCustomNode);
if (isCustomNode)
{
writer.WritePropertyName("Category");
writer.WriteValue(((CustomNodeWorkspaceModel)value).Category);
}

// Description
writer.WritePropertyName("Description");
if (isCustomNode)
writer.WriteValue(((CustomNodeWorkspaceModel)ws).Description);
else
writer.WriteValue(ws.Description);
writer.WritePropertyName("Name");
writer.WriteValue(ws.Name);

Expand Down Expand Up @@ -884,7 +902,7 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s
}
writer.WriteEndArray();

// Join NodeLibraryDependencies & NodeLocalDefinitions and serialze them.
// Join NodeLibraryDependencies & NodeLocalDefinitions and serialize them.
writer.WritePropertyName(WorkspaceReadConverter.NodeLibraryDependenciesPropString);

IEnumerable<INodeLibraryDependencyInfo> referencesList = ws.NodeLibraryDependencies;
Expand Down Expand Up @@ -915,29 +933,21 @@ public override void WriteJson(JsonWriter writer, object value, JsonSerializer s

serializer.Serialize(writer, referencesList);

if (!isCustomNode && ws is HomeWorkspaceModel hws)
if (!isCustomNode && isHomeWsModel)
{
// EnableLegacyPolyCurveBehavior
writer.WritePropertyName(nameof(HomeWorkspaceModel.EnableLegacyPolyCurveBehavior));
serializer.Serialize(writer, hws.EnableLegacyPolyCurveBehavior);

// Thumbnail
writer.WritePropertyName(nameof(HomeWorkspaceModel.Thumbnail));
writer.WriteValue(hws.Thumbnail);
serializer.Serialize(writer, homeWorkspace.EnableLegacyPolyCurveBehavior);

// GraphDocumentaionLink
writer.WritePropertyName(nameof(HomeWorkspaceModel.GraphDocumentationURL));
writer.WriteValue(hws.GraphDocumentationURL);
writer.WriteValue(homeWorkspace.GraphDocumentationURL);

// ExtensionData
writer.WritePropertyName(WorkspaceReadConverter.EXTENSION_WORKSPACE_DATA);
serializer.Serialize(writer, hws.ExtensionData);
serializer.Serialize(writer, homeWorkspace.ExtensionData);
}

// Graph Author
writer.WritePropertyName(nameof(WorkspaceModel.Author));
writer.WriteValue(ws.Author);

// Linter
if(!(ws.linterManager is null))
{
Expand Down
101 changes: 67 additions & 34 deletions src/DynamoCoreWpf/Controls/StartPage.xaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -465,51 +465,81 @@ private void RefreshFileList(ObservableCollection<StartPageListItem> files,
}
}

private Dictionary<string, object> DeserializeJsonFile(string filePath)
private readonly string[] jsonKeys = { "Description", "Thumbnail", "Author" };
private readonly Dictionary<string, object> propertyLookup = [];
private static DefaultJsonNameTable propertyTable = null;

private class GraphData
{
if (DynamoUtilities.PathHelper.isValidJson(filePath, out string jsonString, out Exception ex))
{
return JsonConvert.DeserializeObject<Dictionary<string, object>>(jsonString);
}
else
public string Author = "";
public string Description = "";
public string Thumbnail = "";
}

private GraphData DeserializeJsonFileTest(string filePath)
{
return JsonConvert.DeserializeObject<GraphData>(File.ReadAllText(filePath));
Copy link
Contributor

Choose a reason for hiding this comment

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

so ? was this any faster/slower than streaming?

Copy link
Contributor

Choose a reason for hiding this comment

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

So if these are the numbers : https://github.com/DynamoDS/Dynamo/pull/15759/files#r1932364787
Am I to understand that JsonConvert.DeserializeObject is faster?

}

private Dictionary<string, string> DeserializeJsonFile(string filePath)
{
if (propertyTable == null)
{
if(ex is JsonReaderException)
propertyTable = new DefaultJsonNameTable();
foreach (var pn in jsonKeys)
{
DynamoViewModel.Model.Logger.Log("File is not a valid json format.");
propertyLookup[pn] = propertyTable.Add(pn);
}
else
{
DynamoViewModel.Model.Logger.Log("File is not valid: " + ex.StackTrace);
}
return null;
}
}

private const string BASE64PREFIX = "data:image/png;base64,";

private string GetGraphThumbnail(Dictionary<string, object> jsonObject)
{
jsonObject.TryGetValue("Thumbnail", out object thumbnail);
try
{
var data = new Dictionary<string, string>();
// JsonTextRead will automatically dispose of the stream reader
using (var fs = new FileStream(filePath, FileMode.Open, FileAccess.Read))
using (var jr = new JsonTextReader(new StreamReader(fs)) { PropertyNameTable = propertyTable })
{
while(jr.Read())
Copy link
Contributor

Choose a reason for hiding this comment

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

So is this low level parsing faster then just doing a Json.deserialize to a new class containing only the 3 properties that we need ? Doing the latter would at least be cleaner and less error prone

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The advantage of doing it this way, is that we don't have to read the entire content of the dyn file upfront. I'll do some more testing and see if deseralizing into a new mini class will work better.

Copy link
Contributor

Choose a reason for hiding this comment

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

The properties { "Description", "Thumbnail", "Author" }; are not always in a fixed position. I've seen those scattered through the dyn file. Maybe @mjkkirschner can confirm.
Reading it token by token (or line by line) might be faster in some cases, but it does look messier.
I could go either way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that it is messier. However you have to remember that this happens every time you open the home page and every time you open a new graph. Every ~100ms we can shave off from the overall process means a noticeably better user experience overall.

I did some quick tests and got some rough numbers:

file size (kb) JsonConvert.Deserialize (ms) JsonReader (ms)
800 28 40
1300 45 70
4300 170 450

Copy link
Member

Choose a reason for hiding this comment

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

The order of the json properties is not enforced or guaranteed. I've not yet looked deeply at this PR - is the logic dependent on the tokens being in a specific order?

We can enforce them using the order attribute but that won't help us with existing graphs.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not dependent on the order. If those attributes were always first, it would make it faster I guess

Copy link
Member

Choose a reason for hiding this comment

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

example:

Dynamo/src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs

Line 74 in 3aeb33b

[JsonProperty(Order = 1)]

{
if (data.Count == jsonKeys.Length)
{
break;
}

if (string.IsNullOrEmpty(thumbnail as string)) return string.Empty;
if (jr.Depth != 1)
{
continue;
}

var base64 = String.Format("{0}{1}", BASE64PREFIX, thumbnail as string);
if (jr.TokenType == JsonToken.PropertyName)
{
foreach (var prop in propertyLookup)
{
if (jr.Value == prop.Value)
{
data[prop.Key] = jr.ReadAsString() ?? "";
break;
}
}
}
}
}

return base64;
return data;
}
catch
{
DynamoViewModel.Model.Logger.Log("File is not a valid json format.");
return null;
}
}

private string GetGraphDescription(Dictionary<string, object> jsonObject)
{
jsonObject.TryGetValue("Description", out object description);

return description as string;
}
private const string BASE64PREFIX = "data:image/png;base64,";

private string GetGraphAuthor(Dictionary<string, object> jsonObject)
private string GetGraphThumbnail(Dictionary<string, string> jsonObject)
{
jsonObject.TryGetValue("Author", out object author);

return author as string;
jsonObject.TryGetValue("Thumbnail", out var thumbnail);
return string.IsNullOrEmpty(thumbnail) ? string.Empty : $"{BASE64PREFIX}{thumbnail}";
}

private void HandleRegularCommand(StartPageListItem item)
Expand Down Expand Up @@ -559,9 +589,12 @@ private void HandleExternalUrl(StartPageListItem item)
try
{
var jsonObject = DeserializeJsonFile(filePath);
var description = jsonObject != null ? GetGraphDescription(jsonObject) : string.Empty;
//var test = DeserializeJsonFileTest(filePath);
var description = jsonObject?.TryGetValue("Description", out var description_) == true ?
description_ : string.Empty;
var author = jsonObject?.TryGetValue("Author", out var author_) == true ?
author_ : Resources.DynamoXmlFileFormat;
var thumbnail = jsonObject != null ? GetGraphThumbnail(jsonObject) : string.Empty;
var author = jsonObject != null ? GetGraphAuthor(jsonObject) : Resources.DynamoXmlFileFormat;
var date = DynamoUtilities.PathHelper.GetDateModified(filePath);

return (description, thumbnail, author, date);
Expand Down
2 changes: 1 addition & 1 deletion src/DynamoCoreWpf/DynamoCoreWpf.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@
<PackageReference Include="HelixToolkit.Core.Wpf" Version="2.24.0" />
<PackageReference Include="HelixToolkit.SharpDX.Core.Wpf" Version="2.24.0" />
<PackageReference Include="System.Configuration.ConfigurationManager" Version="5.0.0" />
<PackageReference Include="DynamoVisualProgramming.LibG_231_0_0" Version="3.4.0.3546"/>
<PackageReference Include="DynamoVisualProgramming.LibG_231_0_0" Version="3.4.0.3546" />
<PackageReference Include="FontAwesome5" Version="2.1.11" />
<PackageReference Include="AvalonEdit" Version="6.3.0.90" CopyXML="true" />
<PackageReference Include="Greg" Version="3.0.2.6533" />
Expand Down
20 changes: 5 additions & 15 deletions src/DynamoCoreWpf/ViewModels/Core/DynamoViewModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,9 +518,9 @@ public ConnectorType ConnectorType
}
}

private ObservableCollection<string> recentFiles =
new ObservableCollection<string>();
public ObservableCollection<string> RecentFiles
private SmartObservableCollection<string> recentFiles =
new SmartObservableCollection<string>();
public SmartObservableCollection<string> RecentFiles
{
get { return recentFiles; }
set
Expand Down Expand Up @@ -1108,7 +1108,7 @@ protected virtual void UnsubscribeAllEvents()

private void InitializeRecentFiles()
{
this.RecentFiles = new ObservableCollection<string>(model.PreferenceSettings.RecentFiles);
this.RecentFiles = new SmartObservableCollection<string>(model.PreferenceSettings.RecentFiles);
this.RecentFiles.CollectionChanged += (sender, args) =>
{
model.PreferenceSettings.RecentFiles = this.RecentFiles.ToList();
Expand Down Expand Up @@ -1814,18 +1814,8 @@ internal void AddToRecentFiles(string path)
{
if (path == null) return;

if (RecentFiles.Contains(path))
{
RecentFiles.Remove(path);
}

RecentFiles.Insert(0, path);

int maxNumRecentFiles = Model.PreferenceSettings.MaxNumRecentFiles;
if (RecentFiles.Count > maxNumRecentFiles)
{
RecentFiles.RemoveRange(maxNumRecentFiles, RecentFiles.Count - maxNumRecentFiles);
}
RecentFiles.PushToFrontAndTrimExcess(path, maxNumRecentFiles);
}

// Get the nodemodel if a node is present in any open workspace.
Expand Down
25 changes: 24 additions & 1 deletion src/DynamoUtilities/SmartObservableCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,5 +214,28 @@ internal void SetCollection(IEnumerable<T> range)
}
}
}

internal void PushToFrontAndTrimExcess(T item, int maxNumItems = -1)
{
using (DeferCollectionNotification(NotifyCollectionChangedAction.Reset, Items.ToList()))
{
var index = Items.IndexOf(item);
if (index > 0)
{
RemoveAt(index);
}
Insert(0, item);

if (maxNumItems > 0 && Items.Count > maxNumItems)
{
var toRemove = Items.Count - maxNumItems;
while (toRemove > 0)
{
toRemove--;
RemoveAt(Items.Count - 1);
}
}
}
}
}
}
}
Loading