Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add task merging for graph updates #4276

Merged
merged 3 commits into from
May 11, 2015
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 32 additions & 5 deletions src/DynamoCore/Core/Threading/UpdateGraphAsyncTask.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ class UpdateGraphAsyncTask : AsyncTask

private GraphSyncData graphSyncData;
private EngineController engineController;
private readonly bool verboseLogging;
private bool verboseLogging;

internal override TaskPriority Priority
{
Expand All @@ -31,13 +31,13 @@ internal override TaskPriority Priority

#region Public Class Operational Methods

internal UpdateGraphAsyncTask(IScheduler scheduler, bool verboseLogging) : base(scheduler)
internal UpdateGraphAsyncTask(IScheduler scheduler, bool verboseLogging1) : base(scheduler)
{
this.verboseLogging = verboseLogging;
this.verboseLogging = verboseLogging1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just updated this in master and RC0.8.0_ds branches.

}

/// <summary>
/// This method is called by codes that intent to start a graph update.
/// This method is called by code that intends to start a graph update.
/// This method is called on the main thread where node collection in a
/// WorkspaceModel can be safely accessed.
/// </summary>
Expand All @@ -62,8 +62,9 @@ internal bool Initialize(EngineController controller, WorkspaceModel workspace)
graphSyncData = engineController.ComputeSyncData(workspace.Nodes, ModifiedNodes, verboseLogging);
return graphSyncData != null;
}
catch (Exception)
catch (Exception e)
{
System.Diagnostics.Debug.WriteLine("UpgradeGraphAsyncTask saw: " + e.ToString());
return false;
}
}
Expand Down Expand Up @@ -102,6 +103,32 @@ protected override void HandleTaskCompletionCore()
}
}

protected override AsyncTask.TaskMergeInstruction CanMergeWithCore(AsyncTask otherTask)
{
var theOtherTask = otherTask as UpdateGraphAsyncTask;
if (theOtherTask == null)
return base.CanMergeWithCore(otherTask);


// Comparing to another UpdateGraphAsyncTask, verify that they are updating
// a similar set of nodes

// Other node is either equal or a superset of this task
if (ModifiedNodes.All(x => theOtherTask.ModifiedNodes.Contains(x)))
{
return TaskMergeInstruction.KeepOther;
}

// This node is a superset of the other
if (theOtherTask.ModifiedNodes.All(x => ModifiedNodes.Contains(x)))
{
return TaskMergeInstruction.KeepThis;
}

// They're different, keep both
return TaskMergeInstruction.KeepBoth;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#endregion

#region Public Class Properties
Expand Down