Improve trace reconciliation performance #15725
Conversation
|
|
||
| var currentSerializables = traceData.SelectMany(td => td.RecursiveGetNestedData()); | ||
| result.AddRange(beforeFirstRunSerializables.Where(hs => !currentSerializables.Contains(hs)).ToList()); | ||
| var currentSerializables = traceData.SelectMany(td => td.RecursiveGetNestedData()).ToHashSet(); |
There was a problem hiding this comment.
This line with the added .ToHashSet() call and the line below are the likeliest culprits of the bad performance, as they would previously keep reloading the TraceData from the CallSite object for every .Contains test
There was a problem hiding this comment.
if I'm understanding that correctly, then it could be solved by actualizing the currentSerializables outside the deferred call - right?
Is the cost to create the hashset worth the lookup cost?
UI Smoke TestsTest: success. 11 passed, 0 failed. |
| var orphanedSerializables = cs.GetOrphanedSerializables().ToList(); | ||
| if (callsiteToOrphanMap.ContainsKey(cs.CallSiteID)) | ||
| var orphanedSerializables = cs.GetOrphanedSerializables(); | ||
| if (callsiteToOrphanMap.TryGetValue(cs.CallSiteID, out var serializablesForCallsite)) |
There was a problem hiding this comment.
how does this change improve performance?
There was a problem hiding this comment.
Since the immediate action after the ContainsKey is to mutate the value at that key, using TryGetValue avoids a redundant dictionary lookup
There was a problem hiding this comment.
okay, seems like a pretty safe change, though under profiling does it make an actual difference here?
| } | ||
| else | ||
| { | ||
| callsiteToOrphanMap.Add(cs.CallSiteID, orphanedSerializables); | ||
| callsiteToOrphanMap.Add(cs.CallSiteID, orphanedSerializables.ToList()); |
There was a problem hiding this comment.
so orphanedSerializables is already a List<string> -
Dynamo/src/Engine/ProtoCore/Lang/CallSite.cs
Line 475 in 10df94f
IList - I assume that this calls the List constructor passing in the existing list, have you verified there is any benefit to moving this ToList call around?
If it's really such a big performance impact you could probably change this data structure to be a dict of IList
| @@ -897,7 +897,7 @@ internal IList<string> GetOrphanedSerializablesAndClearHistoricalTraceData() | |||
|
|
|||
| if (Nodes.All(n => n.GUID != nodeGuid)) | |||
| { | |||
| orphans.AddRange(nodeData.Value.SelectMany(CallSite.GetAllSerializablesFromSingleRunTraceData).ToList()); | |||
| orphans.AddRange(nodeData.Value.SelectMany(CallSite.GetAllSerializablesFromSingleRunTraceData)); | |||
There was a problem hiding this comment.
does calling add range immediately execute the deferred selectMany call?
|
|
||
| foreach (var ws in Workspaces.OfType<HomeWorkspaceModel>()) | ||
| foreach (var maybeWs in Workspaces) |
There was a problem hiding this comment.
I don't think there is any value in iterating all the nodes of a custom node workspace... maybe you can say a bit more about why you need this other map to improve performance.
| { | ||
| foreach (var node in maybeWs.Nodes) |
There was a problem hiding this comment.
also consider that this could be a very large number of nodes.
| if (!nodeToWorkspaceMap.TryGetValue(nodeGuid, out var nodeSpace)) | ||
| continue; | ||
|
|
||
| //var nodeSpace = |
|
hey @bendikberg I would like some more description in the PR about what your changes are intending and how you validated that they make a substantial improvement. Maybe not all of these changes are equally important. |
|
|
||
| if (!wsOrphans.Any()) | ||
| continue; | ||
|
|
||
| if (!workspaceOrphanMap.ContainsKey(ws.Guid)) |
There was a problem hiding this comment.
whats the benefit of this change - and the downstream changes below.
| ws => | ||
| ws.Nodes.FirstOrDefault(n => n.GUID == nodeGuid) | ||
| != null); | ||
| if (!nodeToWorkspaceMap.TryGetValue(nodeGuid, out var nodeSpace)) |
There was a problem hiding this comment.
hmm... on first glance memoizing this lookup makes sense, but I'm unclear exactly how many times we recreate the map vs how fast we bail on first finding the node in the existing code.
Is the map recreated only once per workspace open? Does it need to be updated at some point?
|
|
||
| // Add the node's orphaned serializables to the workspace | ||
| // orphan map. | ||
| if (workspaceOrphanMap.ContainsKey(nodeSpace.Guid)) |
There was a problem hiding this comment.
same question about this lookup change.
|
|
||
| public void RecursiveFillWithNestedData(List<string> listToFill) |
There was a problem hiding this comment.
seems this can be private or internal.
|
|
||
| public void RecursiveFillWithNestedData(List<string> listToFill) |
There was a problem hiding this comment.
what is the intention of your change here?
|
@mjkkirschner To add some more context: this is more relevant to D4R because that writes huge amounts of trace data tying in the dynamo wrapper elements to the matching Revit elements for continuous updates between Dynamo/Revit sessions. In cases where the graph stored a lot of references that are no longer valid, the first execution would be very slow. Look at the attached trace data images. The I'm not familiar with Dynamo for Civil3d but if it tracks the elements in a similar way, then this would be relevant there as well. |
| if (!beforeFirstRunSerializables.Any()) | ||
| return result; | ||
| if (beforeFirstRunSerializables.Count == 0) | ||
| return new List<string>(); |
There was a problem hiding this comment.
Is this any more performant?
I made this PR during the D4R performance profiling project. This code was originally written to be more of a research and test thing, and is in a pretty raw state. The reason for the raw state of the PR is that I assumed someone on the Dynamo team would just grab the parts of this PR they deemed relevant and implement those. The thing I was trying to do was to figure out where and why the Callsite TraceData was slowing down the first run of a graph, so I touched on a few different files related to the TraceData handling. Most of the .ToList and ContainsKey/TryGetValue-juggling can be left out of this PR or skipped entirely when they are compared to the performance gained from algorithmic optmizations. They don't cause the same type of exponentital slowdowns, but still do unecessary work. I made those changes since I figured as long as I was upgrading the TraceData performance on the whole, I would also include simple (but tiny) optimizations. I made some benchmarks for the most impactful changes: This gets slower as the number of TraceData items on a node increases (this can easily get very large for a Revit file). RecursiveFillWithNestedData change This scales poorly with the number of nodes on a canvas. For small input sizes it doesn't add up to many microseconds, but it scales very poorly with larger input, which is bad since users might save whatever data in their files, which will make Dynamo hang for seconds or minutes. TL;DR:This PR can be reduced to only include the algorithmic improvements that I've shown in the benchmarks. |
Purpose
When loading a file with a lot of bindings stored in CallSite.SingleRunTraceData, the trace reconciliation spends a lot of time reading nested data due to list allocations. And in general it does unecessary work copying lists and iterating through IEnumerables, for .Contains-testing and the like.
These changes have a large impact on the graph execution time for graphs with a lot of orphaned serializable data, as seen in these profiling results.
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
N/A
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
@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
@dimven