delay graph execution when adding/modifying multiple nodes at once #11230

pinzart90 · 2020-11-03T15:58:42Z

Please Note:

Before submitting the PR, please review How to Contribute to Dynamo
Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
PRs will be reviewed from oldest to newest
If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

https://jira.autodesk.com/browse/AGD-1801

Fix slow undo, copy paste, nodeToCode (basically operations applied on many nodes at once that might trigger recomputation).
Added mechanism to delay graph execution (instead of creating many UpdateGraphAsyncTasks for each node that is modified, we create a single UpdateGraphAsyncTask that will contain all modified nodes)

Changes to code follow the example bellow:

// old code, with 100 Nodes
foreach (node in Nodes) {
  workspace.AddAndRegisterNode(node);
}
// => will result in 100 UpdateGraphAsyncTasks, each one with 1 ModifiedNodes
// these will be executed one at a time by the DynamoScheduler 

new code, with 100 Nodes
using(workspace.BeginDelayedGraphExecution)
{
  foreach (node in Nodes) {
    workspace.AddAndRegisterNode(node);
  }
}
// => will result in 1 UpdateGraphAsyncTask with 100 ModifiedNodes
// will be executed by DynamoScheduler

Declarations

Check these if you believe they are true

The codebase 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.
This PR modifies some build requirements and the readme is updated

Reviewers

(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)

(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

src/DynamoCore/Graph/Workspaces/UndoRedo.cs

aparajit-pratap · 2020-11-03T20:25:51Z

@pinzart90 now that you've removed the Run step, when and where does the Run actually take place?

pinzart90 · 2020-11-03T20:39:12Z

@pinzart90 now that you've removed the Run step, when and where does the Run actually take place?

Dynamo/src/DynamoCore/Graph/Workspaces/UndoRedo.cs

Lines 52 to 56 in 17f1104

    
           undoRecorder.Undo(); 
        
           // http://adsk-oss.myjetbrains.com/youtrack/issue/MAGN-7883 
        
           // Request run for every undo action 
        
           RequestRun();

undoRecorder.Undo(); will eventually call the code that I modified.
RequestRun(); will do the Run

aparajit-pratap

One comment.

mjkkirschner · 2021-01-21T14:00:36Z

looks like 7 test failures.

pinzart90 · 2021-01-21T14:56:07Z

looks like 7 test failures.

Thanks, casting issue. Should be fixed now

src/DynamoCore/Core/DelayedGraphExecution.cs

mjkkirschner · 2021-01-21T16:30:14Z

src/DynamoCore/Core/UndoRedoRecorder.cs

-            // that they were done (due to inter-dependencies among components).
-            // 
-            for (int index = actions.Count - 1; index >= 0; index--)
+            using ((undoClient as WorkspaceModel)?.BeginDelayedGraphExecution())


the syntax is pretty neat.

src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs

mjkkirschner · 2021-01-21T16:34:24Z

There are a few diffs here that are hard to read - are there really that many changes or is that all from wrapping that code in an outer using statement?

mjkkirschner · 2021-01-21T16:35:34Z

I think we should probably add simple unit tests for this new class to enforce its behavior though I guess it would be "covered" by its usage as part of undo/redo?

pinzart90 · 2021-01-21T18:49:50Z

t all from wrapping that code

Changes are simple, it;s just the indentation of the wrapped code.
@mjkkirschner
You can hide whitespace changes in the review. Access the PR with this link https://github.com/DynamoDS/Dynamo/pull/11230/files?diff=unified&w=1

pinzart90 · 2021-01-21T18:50:32Z

I think we should probably add simple unit tests for this new class to enforce its behavior though I guess it would be "covered" by its usage as part of undo/redo?

Wanted a review before adding tests. I will need to add specific tests to target this functionality (even for undo/redo)

…moDS/Dynamo into fix_undo_one_elem_at_a_time

mjkkirschner · 2021-01-22T17:34:59Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

+        /// Class is thread safe, although behavior is not well defined. 
+        /// Nested instance of this class do not have a well defined behavior.
+        /// </summary>
+        internal class DelayedGraphExecution : IDisposable


hmm - I like the new design, what about DelayedWorkspaceExecution for a name? Or is that an implementation detail rather than an API one?

hmm, I'm not convinced that is better or worse

test/DynamoCoreTests/DelayedGraphExecutionTests.cs

mjkkirschner · 2021-01-22T18:05:45Z

@QilongTang if you are going to tackle undo/redo moving of nodes you may want to look at this new internal functionality.

aparajit-pratap · 2021-01-22T19:34:07Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

+                {
+                    workspace.delayGraphExecutionCounter--;
+                }
+                workspace.RequestRun();


There is a RequestRun call, I think, at the end of each of these actions - undo, redo, convertnodetocode etc, so why call it again here?

DelayedGraphExecution can be used in other cases, besides the ones I covered in this PR
The DelayedGraphExecution class does not know that there will be a RequestRun called after it is used.
In the cases of undo, redo, convertnodetocode etc the requestRun calls (after DelayedGraphExecution) will not register any UpdateGraphAsyncTasks, because there will be no more dirty nodes.

do not register each individual node on undo

77035ba

pinzart90 requested review from aparajit-pratap and mjkkirschner November 3, 2020 15:58

pinzart90 commented Nov 3, 2020

View reviewed changes

src/DynamoCore/Graph/Workspaces/UndoRedo.cs Outdated Show resolved Hide resolved

pinzart90 added the WIP label Nov 3, 2020

aparajit-pratap reviewed Nov 3, 2020

View reviewed changes

pinzart added 3 commits November 4, 2020 10:08

Merge branch 'master' into fix_undo_one_elem_at_a_time

c81c86d

add mechanism to delay graph execution

a58010b

Update UndoRedo.cs

a33a412

pinzart90 requested review from aparajit-pratap and sm6srw January 21, 2021 03:55

pinzart90 added PTAL Please Take A Look 👀 and removed WIP labels Jan 21, 2021

fix undoClient cast to Workspace

0fd2e4c

pinzart90 changed the title ~~do not register each individual node on undo~~ delay graph execution when adding/modifying multiple nodes at once Jan 21, 2021

Merge branch 'master' into fix_undo_one_elem_at_a_time

6672e0d

mjkkirschner reviewed Jan 21, 2021

View reviewed changes

src/DynamoCore/Core/DelayedGraphExecution.cs Outdated Show resolved Hide resolved

mjkkirschner reviewed Jan 21, 2021

View reviewed changes

src/DynamoCore/Graph/Workspaces/HomeWorkspaceModel.cs Outdated Show resolved Hide resolved

pinzart added 3 commits January 21, 2021 18:30

update

cd35d37

Merge branch 'fix_undo_one_elem_at_a_time' of https://github.com/Dyna…

3e62984

…moDS/Dynamo into fix_undo_one_elem_at_a_time

Merge branch 'master' into fix_undo_one_elem_at_a_time

b61470f

pinzart added 4 commits January 21, 2021 22:12

Update DynamoCore.csproj

856eb97

add test and refactor

b97c8dd

Update UndoRedo.cs

5b93d6d

Update UndoRedo.cs

cc7daa6

pinzart90 requested a review from mjkkirschner January 22, 2021 17:17

mjkkirschner reviewed Jan 22, 2021

View reviewed changes

test/DynamoCoreTests/DelayedGraphExecutionTests.cs Show resolved Hide resolved

mjkkirschner reviewed Jan 22, 2021

View reviewed changes

test/DynamoCoreTests/DelayedGraphExecutionTests.cs Outdated Show resolved Hide resolved

pinzart added 2 commits January 22, 2021 12:42

Update DelayedGraphExecutionTests.cs

7d411b0

Update DelayedGraphExecutionTests.cs

419af7e

mjkkirschner approved these changes Jan 22, 2021

View reviewed changes

aparajit-pratap reviewed Jan 22, 2021

View reviewed changes

aparajit-pratap approved these changes Jan 23, 2021

View reviewed changes

pinzart90 merged commit fc170b1 into master Jan 24, 2021

pinzart90 deleted the fix_undo_one_elem_at_a_time branch January 24, 2021 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

delay graph execution when adding/modifying multiple nodes at once #11230

delay graph execution when adding/modifying multiple nodes at once #11230

pinzart90 commented Nov 3, 2020 •

edited

Loading

aparajit-pratap commented Nov 3, 2020

pinzart90 commented Nov 3, 2020 •

edited

Loading

aparajit-pratap left a comment

mjkkirschner commented Jan 21, 2021

pinzart90 commented Jan 21, 2021

mjkkirschner Jan 21, 2021

mjkkirschner commented Jan 21, 2021

mjkkirschner commented Jan 21, 2021

pinzart90 commented Jan 21, 2021 •

edited

Loading

pinzart90 commented Jan 21, 2021

mjkkirschner Jan 22, 2021 •

edited

Loading

pinzart90 Jan 22, 2021

mjkkirschner commented Jan 22, 2021

aparajit-pratap Jan 22, 2021 •

edited

Loading

pinzart90 Jan 22, 2021 •

edited

Loading

delay graph execution when adding/modifying multiple nodes at once #11230

delay graph execution when adding/modifying multiple nodes at once #11230

Conversation

pinzart90 commented Nov 3, 2020 • edited Loading

Purpose

Declarations

Reviewers

FYIs

aparajit-pratap commented Nov 3, 2020

pinzart90 commented Nov 3, 2020 • edited Loading

aparajit-pratap left a comment

Choose a reason for hiding this comment

mjkkirschner commented Jan 21, 2021

pinzart90 commented Jan 21, 2021

mjkkirschner Jan 21, 2021

Choose a reason for hiding this comment

mjkkirschner commented Jan 21, 2021

mjkkirschner commented Jan 21, 2021

pinzart90 commented Jan 21, 2021 • edited Loading

pinzart90 commented Jan 21, 2021

mjkkirschner Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

pinzart90 Jan 22, 2021

Choose a reason for hiding this comment

mjkkirschner commented Jan 22, 2021

aparajit-pratap Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

pinzart90 Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

pinzart90 commented Nov 3, 2020 •

edited

Loading

pinzart90 commented Nov 3, 2020 •

edited

Loading

pinzart90 commented Jan 21, 2021 •

edited

Loading

mjkkirschner Jan 22, 2021 •

edited

Loading

aparajit-pratap Jan 22, 2021 •

edited

Loading

pinzart90 Jan 22, 2021 •

edited

Loading