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

DYN-1754: As a developer I want detailed per node profiling during perf benchmarks #9667

Merged
merged 48 commits into from
May 16, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
56dbd73
Update changes from librarie.js to fix QNTM-3710
ColinDayOrg Mar 22, 2018
eec64b9
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Mar 23, 2018
4362151
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jul 19, 2018
75643c7
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 2, 2018
6a07e6c
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 19, 2018
38b8461
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 21, 2018
44a00a8
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Nov 29, 2018
5c04a06
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 7, 2018
a21fd6a
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 10, 2018
4235133
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
302b9dd
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
6bf5548
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 14, 2018
8d4511b
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Dec 21, 2018
8f9df0f
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 8, 2019
e91ef30
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 9, 2019
82373cc
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 10, 2019
1434ba0
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 10, 2019
706974c
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Jan 16, 2019
760ae95
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 4, 2019
6ffd883
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 15, 2019
5b1c073
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Feb 27, 2019
f11f924
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Mar 27, 2019
710c7ee
Merge branch 'master' of https://github.com/DynamoDS/Dynamo
ColinDayOrg Apr 1, 2019
e8f5ade
Initial add and code cleanup for profiling data
ColinDayOrg Apr 10, 2019
f740c1d
Add profiling changes
ColinDayOrg Apr 17, 2019
7644f6b
Add missing summaries and accessor items
ColinDayOrg Apr 18, 2019
dc0eb40
Add an accessor for the profiling data to the profiling session
ColinDayOrg Apr 18, 2019
7e903d2
Make profiling session private
ColinDayOrg Apr 19, 2019
c1aee1d
Improve infrastructure (switch to property, make DateTime property n…
ColinDayOrg Apr 19, 2019
3fc8b74
Add test for pre-run, post-run, and node deletion post-run
ColinDayOrg Apr 19, 2019
c5d3a1d
Remove unused AstCompilationEvents class
ColinDayOrg Apr 22, 2019
0cb178c
Remove unused CompilationEventArgs class
ColinDayOrg Apr 22, 2019
c463ba3
Rename IProfilingData to IProfilingExecutionTimeData
ColinDayOrg Apr 22, 2019
f13049c
Remove derivation from NotificationObject
ColinDayOrg Apr 22, 2019
c228452
Restrict access to NodeProfilingData methods as much as possible
ColinDayOrg Apr 23, 2019
43129bd
Pull strings out to constants
ColinDayOrg Apr 23, 2019
320ae6b
Update naming
ColinDayOrg Apr 29, 2019
6eb5cc5
Obsolete Node property getter and add NodeId property getter
ColinDayOrg May 3, 2019
edb88eb
Change property name from Node to NodeId and obsolete Node to avoid c…
ColinDayOrg May 3, 2019
b94aefd
Move profiling data housekeeping out of the engine controller
ColinDayOrg May 3, 2019
9caccd6
Add the abillity to switch profiling on/off (off by default), includi…
ColinDayOrg May 8, 2019
995ee9b
Move ownership of profiling data to the ast builder
ColinDayOrg May 9, 2019
8e902db
minor code fixes
aparajit-pratap May 15, 2019
3cb15a7
Merge pull request #1 from aparajit-pratap/DYN-1754
ColinDayOrg May 15, 2019
7a82082
Fix typo
ColinDayOrg May 15, 2019
9173bc8
Add public interface to get profiling data
ColinDayOrg May 15, 2019
87564af
Merge branch 'DYN-1754' of https://github.com/ColinDayOrg/Dynamo into…
ColinDayOrg May 15, 2019
686f72c
Add test for public data access method
ColinDayOrg May 16, 2019
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
6 changes: 5 additions & 1 deletion src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ public class AstBuilder : LogSourceBase
{
private readonly IAstNodeContainer nodeContainer;

private ProfilingSession profilingSession = null;
internal ProfilingSession profilingSession = null;
Copy link
Member

Choose a reason for hiding this comment

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

is this internal for a specific reason? (sorry have not looked further down to see if that's the case yet)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, has been changed to be private now

internal void SetProfilingSession(ProfilingSession profilingSession)
{
this.profilingSession = profilingSession;
}

/// <summary>
/// Construct a AstBuilder with AST node contiainer.
Expand Down
31 changes: 29 additions & 2 deletions src/DynamoCore/Engine/EngineController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Dynamo.Engine.NodeToCode;
using Dynamo.Engine.Profiling;
using Dynamo.Graph.Nodes;
using Dynamo.Graph.Workspaces;
using Dynamo.Logging;
using Dynamo.Scheduler;
using ProtoCore.AST.AssociativeAST;
Expand Down Expand Up @@ -69,7 +70,7 @@ private void OnTraceReconciliationComplete(TraceReconciliationEventArgs e)
/// </summary>
public static CompilationServices CompilationServices;

internal ProfilingSession profilingSession;
internal ProfilingSession profilingSession = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the ProfilingSession need to be owned both by AstBuilder and EngineController?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have moved the ownership of the profiling data into the ASTBuilder only. Latest changes have just been pushed.


/// <summary>
/// Returns DesignScript core.
Expand Down Expand Up @@ -125,7 +126,6 @@ public EngineController(LibraryServices libraryServices, string geometryFactoryF
this.libraryServices = libraryServices;
libraryServices.LibraryLoaded += LibraryLoaded;
CompilationServices = new CompilationServices(libraryServices);
profilingSession = new ProfilingSession();

liveRunnerServices = new LiveRunnerServices(this, geometryFactoryFileName);

Expand All @@ -151,6 +151,33 @@ public void Dispose()
codeCompletionServices = null;
}

/// <summary>
/// Enables or disables profiling depending on the given argument
/// </summary>
/// <param name="enable">Indicates enabling or disabling of profiling.</param>
/// <param name="workspace">The workspace to enable or disable profiling for.</param>
/// <param name="nodes">The list of nodes to enable or disable profiling for.</param>
public void EnableProfiling(bool enable, HomeWorkspaceModel workspace, IEnumerable<NodeModel> nodes)
{
Validity.Assert(workspace != null, "Workspace csannot be null");
Validity.Assert(nodes != null, "Node list csannot be null");

if (enable)
{
if (profilingSession == null)
{
profilingSession = new ProfilingSession();
}
}
else
{
profilingSession = null;
}

astBuilder.SetProfilingSession(profilingSession);
workspace.MarkNodesAsModifiedAndRequestRun(nodes);
}

#region Function Groups

/// <summary>
Expand Down
3 changes: 1 addition & 2 deletions src/DynamoCore/Engine/Profiling/ProfilingSession.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using System;
using System.Collections.Generic;
using System.Linq;
using Dynamo.Core;
using Dynamo.Events;
using Dynamo.Graph.Nodes;
using ProtoCore.AST.AssociativeAST;
Expand All @@ -12,7 +11,7 @@ namespace Dynamo.Engine.Profiling
/// <summary>
/// This class manages a diagnostic session and the data collected in a session.
/// </summary>
class ProfilingSession : NotificationObject, IDisposable
class ProfilingSession : IDisposable
{
private ProfilingData profilingData;

Expand Down
33 changes: 25 additions & 8 deletions test/DynamoCoreTests/ProfilingTest.cs
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
using System;
using System.Collections.Generic;
using System.Collections.Generic;
using System.IO;
using System.Linq;

using Dynamo.Engine.CodeGeneration;
using Dynamo.Graph;
using Dynamo.Graph.Nodes;
using Dynamo.Graph.Workspaces;
using Dynamo.Models;
using NUnit.Framework;

Expand All @@ -29,15 +26,30 @@ public void TestProfilingSingleNode()
string openPath = Path.Combine(TestDirectory, @"core\profiling\SingleNode.dyn");
OpenModel(openPath);

Assert.IsNotNull(CurrentDynamoModel.EngineController.profilingSession);
var profilingData = CurrentDynamoModel.EngineController.profilingSession.ProfilingData;
var node = CurrentDynamoModel.CurrentWorkspace.Nodes.FirstOrDefault();
// Assert that profiling is disabled by default
var engineController = CurrentDynamoModel.EngineController;
Assert.IsNull(engineController.profilingSession);

// Assert that no profiling data is created after a run when profiling is disabled
BeginRun();
Assert.IsNull(engineController.profilingSession);

var homeWorkspace = CurrentDynamoModel.Workspaces.OfType<HomeWorkspaceModel>().FirstOrDefault();
var nodes = CurrentDynamoModel.CurrentWorkspace.Nodes;

// Assert that profiling can be enabled
engineController.EnableProfiling(true, homeWorkspace, nodes);
Assert.IsNotNull(engineController.profilingSession);

// Assert that there is no profiling data before the graph is run
var profilingData = engineController.profilingSession.ProfilingData;
var node = nodes.FirstOrDefault();
Assert.IsNull(profilingData.TotalExecutionTime);
Assert.IsNull(profilingData.NodeExecutionTime(node));

BeginRun();

// Assert that profiling data exists after a run occurs
Assert.IsNotNull(profilingData.TotalExecutionTime);
Assert.Greater(profilingData.TotalExecutionTime?.Ticks, 0);
Assert.IsNotNull(profilingData.NodeExecutionTime(node));
Expand All @@ -47,9 +59,14 @@ public void TestProfilingSingleNode()
new DynamoModel.DeleteModelCommand(node.GUID));
BeginRun();

// Assert that per-node profiling data is deleted after a node is deleted
Assert.IsNotNull(profilingData.TotalExecutionTime);
Assert.Greater(profilingData.TotalExecutionTime?.Ticks, 0);
Assert.IsNull(profilingData.NodeExecutionTime(node));

// Assert that profiling can be disabled
engineController.EnableProfiling(false, homeWorkspace, nodes);
Assert.IsNull(engineController.profilingSession);
}
}
}