Fix crashes by undoing/redoing nodes deletion #11021
Conversation
Fixes two crashes whie doing this at a high pace: One crash happened in HomeWorkspaceViewModel, when postprocessing the nodes after execution. These changes were done from the scheduler thread, which when timed just right conflicted with chnages done from the UI thread, by the undo and redo operations. Another one happened in ChnageSetComputer, when trying to compute the delta AST. The cause was an optimization that attempted to skip tasks based on containment comparison but disrespecting causality. The path taken was to prevent skipping tasks altogether, but some more elaborate optimization can probably be put in place.
| protected override TaskMergeInstruction CanMergeWithCore(AsyncTask otherTask) | ||
| { | ||
| var theOtherTask = otherTask as UpdateGraphAsyncTask; |
There was a problem hiding this comment.
I think this change may have significant performance impacts - we should investigate the original reason for the implementation - my memory is that when moving a slider it kicked off hundreds of unnecessary runs and made dynamo unresponsive - if this is related then theres an argument that the crash is actually acceptable...?
There was a problem hiding this comment.
Strange that was fixed by doing this instead of throttling the requests created by the slider 🤔
Still, maybe we could restore a more elaborate merge strategy that accounted for this case in particular. What kind of graphSyncData would we expect? Do we have a test case?
There was a problem hiding this comment.
heres part of the discussion I recall:
#4276
There was a problem hiding this comment.
and a related github issue:
#4229
There was a problem hiding this comment.
Delay seems to work just fine according to my manual testing. Is there any special validation you would like to make beyond what's shown in that issue?
There was a problem hiding this comment.
I'm concerned if there are other scenarios (other than the slider) where we are not dropping redundant tasks that are quickly accumulating as a result of this change thus hitting performance.
There was a problem hiding this comment.
We don't control the UI that other package authors may build - delay for the slider is great, but it does not cover the case generally, though I guess we could document the idea somewhere to throttle their controls?
For example I am thinking of controls like:
and
There was a problem hiding this comment.
Granted. For those cases having a strategy to merge/discard would be useful. I'll try to come up with something.
|
Manually tested the slider without the |
|
Looks good to me. |
|
@mmisol I really like this explanation - thanks, and solution looks good.
|
|
@mjkkirschner I can give it a try in Revit to be sure. I kept the |
|
@mjkkirschner Worked fine in Revit. I'll wait for https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/1315/ to make sure this is not causing anything weird in the CI. In the end I removed the delay on the slider to keep it smooth like before. |
|
In the end I had to use The last build I ran is https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/1319/ |
| NodeViewModel nodeViewModel; | ||
| lock (Nodes) | ||
| { | ||
| nodeViewModel = Nodes.First(x => x.NodeLogic == node); |
There was a problem hiding this comment.
I guess this is safest, but is there any harm in scanning the collection outside the lock?
There was a problem hiding this comment.
The thing is you can run into errors by enumerating over it while it's being modified. That's the initial crash I found.
|
Thanks @mmisol - though this makes me wonder how rigorous we should be in applying locks or other mutual exclusion mechanisms in Dynamo - especially in the view - in the Revit context this rarely matters, but now that more integrations are using multiple threads this will become more necessary. |
Purpose
Fixes two crashes whie doing this at a high pace:
One crash happened in HomeWorkspaceViewModel, when postprocessing the
nodes after execution. These changes were done from the scheduler
thread, which when timed just right conflicted with changes done from
the UI thread, by the undo and redo operations.
Another one happened in ChangeSetComputer, when trying to compute the
delta AST. The cause was an optimization that attempted to skip tasks
based on containment comparison but disrespecting causality. The path
taken was to prevent skipping tasks altogether, but some more elaborate
optimization can probably be put in place.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@aparajit-pratap @mjkkirschner
FYIs
@DynamoDS/dynamo