Add task merging for graph updates #4276
Conversation
|
(The automated testing for this is a bit more complicated - at this stage what I want to understand is: is there any reason that we can't follow this strategy?) |
|
This was what I tested, and it seemed to work well. I think merging update graph tasks can only be a good thing. The only time the scheduler gets flooded with update requests is when someone is sliding a slider. And the desired behavior during slider sliding is that graph updates feel as immediate as possible. So this makes that possible. And, because rendering is tied to update (we render some amount of stuff every time we update), and because rendering is more expensive than update in many cases, it is to everyone's benefit to render less - that is, to update less. |
| @@ -33,11 +33,11 @@ internal override TaskPriority Priority | |||
|
|
|||
| internal UpdateGraphAsyncTask(IScheduler scheduler, bool verboseLogging1) : base(scheduler) | |||
| { | |||
| verboseLogging = verboseLogging; | |||
| this.verboseLogging = verboseLogging1; | |||
There was a problem hiding this comment.
I've just updated this in master and RC0.8.0_ds branches.
|
Hi @ikeough, @lukechurch, an If we really do want to decide if either of the tasks should be kept, then we need to implement a strategy that meaningfully compares the tasks. For a start, I think storing Does that make sense? |
|
@Benglin I see. We could certainly use a comparison based strategy to fix the most common case which is a slider manipulation. In general it seems that what we need is an API where we can take two tasks and instead of simply comparing them, we can return a new task that replaces both. In this case it would be computing the union of the modified nodes? |
|
Oops, sorry @lukechurch I had this open in a browser tab and didn't get back to it before a system restart. To fix the slider case, adding a list of |
…church_magn7078 Conflicts: src/DynamoCore/Core/Threading/UpdateGraphAsyncTask.cs
|
@Benglin np, thanks for the input PTAL at these changes which I think address the concern |
|
|
||
| // They're different, keep both | ||
| return TaskMergeInstruction.KeepBoth; | ||
| } |
There was a problem hiding this comment.
Thanks for the update, @lukechurch, this looks great! :)
Another minor thing is, as @ke-yu pointed out, HashSet.IsSupersetOf can probably do this better but for graph nodes (which are not in huge numbers) this should be fine. LGTM!
Sorry for being late on the review anyway.
Add task merging for graph updates
@Benglin @ikeough Any reason to not do graph update task merging?
wrt: http://adsk-oss.myjetbrains.com/youtrack/issue/MAGN-7078
Opinions solicited.