Improve startup and opening times #15759
Conversation
|
|
||
| namespace Dynamo.Wpf.Utilities | ||
| { | ||
| public class QuietObservableCollection<T> : ObservableCollection<T> |
There was a problem hiding this comment.
we already have a similar implementation.
Please use or enhance the existing one it if necessary
https://github.com/DynamoDS/Dynamo/blob/master/src/DynamoUtilities/SmartObservableCollection.cs
There was a problem hiding this comment.
Yes, makes sense. Sorry, I did not find that one :D
| @@ -465,51 +465,50 @@ private void RefreshFileList(ObservableCollection<StartPageListItem> files, | |||
| } | |||
| } | |||
|
|
|||
| private Dictionary<string, object> DeserializeJsonFile(string filePath) | |||
| private static string[] jsonKeys = { "Description", "Thumbnail", "Author" }; | |||
| else | ||
| { | ||
| DynamoViewModel.Model.Logger.Log("File is not valid: " + ex.StackTrace); | ||
| while(jr.Read()) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is not dependent on the order. If those attributes were always first, it would make it faster I guess
There was a problem hiding this comment.
example:
Purpose
Imagine you have 10 very large graphs in your recent files list. Every time the start page opens, it reads the entire contents of those files (plus any backup files), and then parses the content of each file twice! Then every time you open a new file, the whole process is repeated multiple times 😿
I propose we:
The way the
RecentFilesis used right now doesn't really necessitate anObservableCollectionand a list property would work just the same. However, in case that changes in the future, a quiet observable collection will preserve the flexibility.Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
(FILL ME IN) @mjkkirschner
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of