DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update #10083

saintentropy · 2019-10-24T01:06:36Z

Purpose

The purpose of this PR is to address a slow down observed in Dynamo graphs with large node quantities when Dynamo4Revit is responding to Document Change events in Revit. The specific flaw begins when the event handler RevitServicesUpdater_ElementsUpdated() in D4R responds to the Document Change events. It filters for transactions originating from Dynamo then passes the associated Element Id's to GetNodesFromElementIds() in the ElementBinder. This method calls GetCallsitesForNodes() in DynamoCore to find the associated Callsites for the nodes in the Graph. This lookup can be extremely slow due to a series of nested loops. This PR address the slowdown by processing the lookup with a pre-populated dictionary mapping the node Guids to Callsite Identifiers as the existing Callsite Cache dictionary. The runtime difference for our sample graph with 456 nodes was 23sec to ~20ms.

Declarations

Check these if you believe they are true

The code base is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner assign to whoever you want

FYIs

@Dewb @QilongTang

… Callsite construction

…interaction with instruction stream data

…d and used correctly

src/Engine/ProtoCore/RuntimeData.cs

saintentropy · 2019-10-24T01:29:40Z

As a side note, looking at the original call in D4R GetNodesFromElementIds() method, it seems like it could just use the CallsiteCache directly instead of calling GetCallsitesForNodes() first. I need to check what node types it filters but may be faster just to process all the Callsites.

QilongTang · 2019-10-24T04:08:23Z

src/Engine/ProtoCore/RuntimeData.cs

+        /// <summary>		
+        /// Map from a graph UI node to callsite identifiers. 		
+        /// </summary>
+        public Dictionary<Guid, List<string>> NodeToCallsiteIdentifiersMap { get; }


Seems internal or private now, can we expose this later? Also looking at CallSiteToNodeMap above, maybe Dictionary<Guid, List<Guid>> would be more consistent.

Access modifier can be set more conservative but I would argue the other runtime dictionaries should be as well.

I purposefully did not use Dictionary<Guid, List<Guid>> as the primary lookup key for the CallsiteCache dictionary was via the Callsite Identifier string. Using the Callsite.CallsiteID Guid would require another series of ForEach loops or the creation of another map to the Callsite objects which did not seem advisable. CallsiteCache is the critical map here and is used in many places. The NodeToCallsite is only used in the ReconcileTraceDataAndNotify() which we should also look at for its inefficiency.

QilongTang · 2019-10-24T04:28:28Z

Can you also look at why the AppVeyor build fails?

saintentropy · 2019-10-24T11:39:56Z

AppVeyor fixed :-) When can we use C# 7 features @QilongTang?

QilongTang · 2019-10-24T15:36:11Z

@saintentropy When we move everything to Visual Studio 2019, currently only me in the team using that so I got the same pain :(

mjkkirschner · 2019-10-25T14:59:13Z

@saintentropy it is likely a good idea to run the RTF tests over this change after it is merged - perhaps @AndyDu1985 or @ZiyunShang can help there?

mjkkirschner · 2019-10-25T14:59:37Z

I'm also curious if the elementBinding perf tests get faster?
The answer is not really - but they run in 10 already so...

22:57:16  |    Run |      ElementBindingLarge |    Base |     11.31 ms |   0.34 ms |   0.20 ms |
22:57:16  |        |                          |     New |     10.80 ms |   0.13 ms |   0.03 ms |
22:57:16  |        |                          |         |       -4.51% |   -61.27% |   -83.09% |PASS

seems we should have a longer running version of this test.

saintentropy · 2019-10-30T00:35:31Z

@mjkkirschner I don't think the ElementBindingLarge test is testing Revit Element binding though its more like testing the code path I updated. ElementBindingLarge is more about the trace mechanism. We should run it on the pre/post thread local storage changes.

aparajit-pratap · 2019-11-01T03:07:20Z

src/Engine/ProtoCore/RuntimeData.cs

+                }
+                else
+                {
+                    NodeToCallsiteIdentifiersMap[topGraphNode.guid] = new List<string>() { callsiteID };


Why can't we simply declare a NodeToCallsiteMap as:

Dictionary<Guid, List<CallSite>> NodeToCallsiteMap;

Then we can simply say:

NodeToCallsiteMap[topGraphNode.guid] = new List<CallSite>() { csInstance };

Yes we can do that instead.... I wasn't sure the implications of having two maps that have Callsite Values. Seems ok since the CallsiteCache is never modified other than add

aparajit-pratap · 2019-11-01T03:12:08Z

src/Engine/ProtoCore/RuntimeData.cs

+                else
+                {
+                    nodeMap[nodeGuid] = new List<CallSite>();
+                }


Then if we have the NodeToCallsiteMap, we won't need to do any of this either; we could simply return that map; or am I missing something?

Yes... this function still might need to filter the map based on the incoming dynamo graph node Guids but it could be simpler. Back in the D4R they could simply use the map

aparajit-pratap · 2019-11-01T03:22:42Z

@saintentropy is this specific slowdown you're observing in DYN-2226 due to document-change events occurring in Revit caused by specific Revit UI interactions (Revit transactions) or caused by D4R nodes that cause the Revit document to change?

saintentropy · 2019-11-01T16:50:15Z

@aparajit-pratap I haven't tested but could be due to both in the graph was in automatic mode. Our specific case was due to a graph running in D4R

mjkkirschner · 2019-11-01T17:01:46Z

@saintentropy I actually believe I checked with Nate that the graph was in manual mode. He should have the full stack trace from the profiling session.

saintentropy · 2019-11-01T17:32:47Z

@mjkkirschner I mean you can also see this action cause by Revit UI interaction if D4R is open and is in automatic mode. Our Repo case was a single run of the graph caused by D4R

mjkkirschner · 2019-11-04T16:48:49Z

src/Engine/ProtoCore/RuntimeData.cs

-                    {
-                        var graph = stream[0].dependencyGraph;
-                        if (graph != null)
-                            graphNodes = graph.GraphList;


@aparajit-pratap can you give us a summary of what a GraphNode is in the context of the VM - and then is there any reason not to rename this to something less confusing as soon as possible?

It's basically a node in the dependency graph held by the VM. During codegen phase the compiler generates graphnodes that represent each executable statement

Graphnodes contain information about dependencies between statements and runtime properties instructing VM which instruction to execute next - they have a property that points to start and end of an instruction block

Every time a graph node executes it jumps to its target label as per dependency property stored in it - the “dep” instruction is responsible for finding which graph node to execute next given what
was currently modified

mjkkirschner · 2019-11-04T16:49:57Z

src/Engine/ProtoCore/RuntimeData.cs

@@ -120,6 +125,15 @@ public CallSite GetCallSite(int classScope, string methodName, Executable execut

                CallsiteCache[callsiteID] = csInstance;
                CallSiteToNodeMap[csInstance.CallSiteID] = topGraphNode.guid;
+                List<CallSite> callsites;


@saintentropy did you notice that the params and comment for this method are completely wrong?

@mjkkirschner Which method are you meaning? GetCallsite

…tting element bound Revit nodes to update (DynamoDS#10083) * Add Map for Node guids to Callsite identifier strings * Load RuntimeData map for Node guids to Callsite identifier strings on Callsite construction * look up matching callsites directly for a node Guid with maps versus interaction with instruction stream data * Add test to validate map for node to callsite identifier is intialized and used correctly * Remove c#7 inline out variable declaration * remove more inline declaration * Remove graphnode check * Change map from callsiteIdentifier to callsite object * Update test strings * make new property internal * add code comments

…und nodes to update (#10160) * DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update (#10083) * Add Map for Node guids to Callsite identifier strings * Load RuntimeData map for Node guids to Callsite identifier strings on Callsite construction * look up matching callsites directly for a node Guid with maps versus interaction with instruction stream data * Add test to validate map for node to callsite identifier is intialized and used correctly * Remove c#7 inline out variable declaration * remove more inline declaration * Remove graphnode check * Change map from callsiteIdentifier to callsite object * Update test strings * make new property internal * add code comments

saintentropy added 4 commits October 23, 2019 17:10

Add Map for Node guids to Callsite identifier strings

53ac7e8

Load RuntimeData map for Node guids to Callsite identifier strings on…

f380646

… Callsite construction

look up matching callsites directly for a node Guid with maps versus …

b29bc96

…interaction with instruction stream data

Add test to validate map for node to callsite identifier is intialize…

a468534

…d and used correctly

saintentropy changed the title ~~DYN-2226: Graphs with large node quantities cause long delays when setting element bond Revit nodes to update~~ WIP DYN-2226: Graphs with large node quantities cause long delays when setting element bond Revit nodes to update Oct 24, 2019

saintentropy added the WIP label Oct 24, 2019

saintentropy commented Oct 24, 2019

View reviewed changes

src/Engine/ProtoCore/RuntimeData.cs Show resolved Hide resolved

QilongTang reviewed Oct 24, 2019

View reviewed changes

Remove c#7 inline out variable declaration

ed37d04

saintentropy added the performance label Oct 24, 2019

remove more inline declaration

b356243

aparajit-pratap reviewed Nov 1, 2019

View reviewed changes

saintentropy added 3 commits November 4, 2019 09:04

Remove graphnode check

65a498d

Change map from callsiteIdentifier to callsite object

73304e3

Update test strings

70c4bce

mjkkirschner changed the title ~~WIP DYN-2226: Graphs with large node quantities cause long delays when setting element bond Revit nodes to update~~ WIP DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update Nov 4, 2019

mjkkirschner reviewed Nov 4, 2019

View reviewed changes

saintentropy removed the WIP label Nov 8, 2019

saintentropy changed the title ~~WIP DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update~~ DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update Nov 8, 2019

mjkkirschner added this to the 2.5.0 milestone Nov 20, 2019

make new property internal

982ea82

aparajit-pratap added the LGTM Looks good to me label Nov 21, 2019

add code comments

9dde591

aparajit-pratap merged commit 9115b98 into DynamoDS:master Nov 22, 2019

aparajit-pratap mentioned this pull request Nov 22, 2019

Cherry-pick #10083: Large graphs cause delays when setting element bound nodes to update #10160

Merged

7 tasks

QilongTang mentioned this pull request Feb 13, 2020

Cherrypick Element Binding Performance IMP #10386

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update #10083

DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update #10083

saintentropy commented Oct 24, 2019

saintentropy commented Oct 24, 2019

QilongTang Oct 24, 2019 •

edited

Loading

saintentropy Oct 24, 2019

saintentropy Oct 24, 2019 •

edited

Loading

QilongTang commented Oct 24, 2019

saintentropy commented Oct 24, 2019

QilongTang commented Oct 24, 2019

mjkkirschner commented Oct 25, 2019

mjkkirschner commented Oct 25, 2019 •

edited

Loading

saintentropy commented Oct 30, 2019

aparajit-pratap Nov 1, 2019

saintentropy Nov 1, 2019

aparajit-pratap Nov 1, 2019

saintentropy Nov 1, 2019

aparajit-pratap commented Nov 1, 2019

saintentropy commented Nov 1, 2019

mjkkirschner commented Nov 1, 2019

saintentropy commented Nov 1, 2019

mjkkirschner Nov 4, 2019

aparajit-pratap Nov 15, 2019 •

edited

Loading

mjkkirschner Nov 4, 2019

saintentropy Nov 4, 2019

mjkkirschner Nov 4, 2019

DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update #10083

DYN-2226: Graphs with large node quantities cause long delays when setting element bound Revit nodes to update #10083

Conversation

saintentropy commented Oct 24, 2019

Purpose

Declarations

Reviewers

FYIs

saintentropy commented Oct 24, 2019

QilongTang Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saintentropy Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

QilongTang commented Oct 24, 2019

saintentropy commented Oct 24, 2019

QilongTang commented Oct 24, 2019

mjkkirschner commented Oct 25, 2019

mjkkirschner commented Oct 25, 2019 • edited Loading

saintentropy commented Oct 30, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap commented Nov 1, 2019

saintentropy commented Nov 1, 2019

mjkkirschner commented Nov 1, 2019

saintentropy commented Nov 1, 2019

Choose a reason for hiding this comment

aparajit-pratap Nov 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QilongTang Oct 24, 2019 •

edited

Loading

saintentropy Oct 24, 2019 •

edited

Loading

mjkkirschner commented Oct 25, 2019 •

edited

Loading

aparajit-pratap Nov 15, 2019 •

edited

Loading