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 37 commits
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
13 changes: 13 additions & 0 deletions src/DynamoCore/DynamoCore.csproj
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,14 @@ limitations under the License.
<Compile Include="Configuration\DebugSettings.cs" />
<Compile Include="Configuration\Context.cs" />
<Compile Include="Core\DynamoMigrator.cs" />
<Compile Include="Engine\CodeGeneration\CompilationContext.cs" />
<Compile Include="Engine\CodeGeneration\CompiledEventArgs.cs" />
<Compile Include="Engine\CodeGeneration\CompilingEventArgs.cs" />
<Compile Include="Engine\CodeGeneration\IAstNodeContainer.cs" />
<Compile Include="Engine\Profiling\IProfilingExecutionTimeData.cs" />
<Compile Include="Engine\Profiling\NodeProfilingData.cs" />
<Compile Include="Engine\Profiling\ProfilingData.cs" />
<Compile Include="Engine\Profiling\ProfilingSession.cs" />
<Compile Include="Engine\ShortestQualifiedNameReplacer.cs" />
<Compile Include="Exceptions\LibraryLoadFailedException.cs" />
<Compile Include="Graph\Nodes\NodeOutputData.cs" />
Expand Down Expand Up @@ -342,6 +350,10 @@ limitations under the License.
<Project>{6e0a079e-85f1-45a1-ad5b-9855e4344809}</Project>
<Name>Units</Name>
</ProjectReference>
<ProjectReference Include="..\Libraries\VMDataBridge\VMDataBridge.csproj">
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
<Project>{ccb6e56b-2da1-4eba-a1f9-e8510e129d12}</Project>
<Name>VMDataBridge</Name>
</ProjectReference>
<ProjectReference Include="..\NodeServices\DynamoServices.csproj">
<Project>{ef879a10-041d-4c68-83e7-3192685f1bae}</Project>
<Name>DynamoServices</Name>
Expand All @@ -360,6 +372,7 @@ limitations under the License.
<ItemGroup>
<Resource Include="BuiltInAndOperators\BuiltIn.Migrations.xml" />
</ItemGroup>
<ItemGroup />
<Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />
<Target Name="BeforeBuild">
<ItemGroup>
Expand Down
172 changes: 58 additions & 114 deletions src/DynamoCore/Engine/CodeGeneration/AstBuilder.cs
Original file line number Diff line number Diff line change
@@ -1,125 +1,36 @@
#region

using System;
using System;
using System.Collections.Generic;
using System.Linq;
using Dynamo.Graph.Nodes;
using Dynamo.Library;
using Dynamo.Logging;
using Dynamo.Engine.Profiling;

using ProtoCore;
using ProtoCore.AST.AssociativeAST;
using ProtoCore.Utils;
using Type = ProtoCore.Type;

#endregion

namespace Dynamo.Engine.CodeGeneration
{
/// <summary>
/// Returns notification for AST compilation events.
/// </summary>
public interface IAstNodeContainer
{
/// <summary>
/// Indicates to start compiling a NodeModel to AST nodes.
/// </summary>
/// <param name="nodeGuid"></param>
void OnCompiling(Guid nodeGuid);

/// <summary>
/// Indicates a NodeModel has been compiled to AST nodes.
/// </summary>
/// <param name="nodeGuid"></param>
/// <param name="astNodes"></param>
void OnCompiled(Guid nodeGuid, IEnumerable<AssociativeNode> astNodes);
}

/// <summary>
/// This event is triggerred when compiling a NodeModel to AST nodes.
/// </summary>
public class CompilingEventArgs : EventArgs
{
/// Construct ASTBuildingEventArgs with NodeModel.
public CompilingEventArgs(Guid node)
{
Node = node;
}

/// <summary>
/// Guid of NodeModel that is being compiled to AST.
/// </summary>
public Guid Node { get; private set; }
}

/// <summary>
/// This event is triggered when a NodeModel has been compiled to a list of
/// AST nodes.
/// </summary>
public class CompiledEventArgs : EventArgs
{
/// <summary>
/// Construct ASTBuiltEventArgs with NodeModel and AST nodes.
/// </summary>
/// <param name="node"></param>
/// <param name="astNodes"></param>
internal CompiledEventArgs(Guid node, IEnumerable<AssociativeNode> astNodes)
{
Node = node;
AstNodes = astNodes;
}

/// <summary>
/// Guid of node that has been built to AST nodes.
/// </summary>
public Guid Node { get; private set; }

/// <summary>
/// Built AST nodes.
/// </summary>
public IEnumerable<AssociativeNode> AstNodes { get; private set; }
}

/// <summary>
/// The context of AST compilation
/// </summary>
public enum CompilationContext
{
/// <summary>
/// No specific context.
/// </summary>
None,

/// <summary>
/// Compiled AST nodes finally will be executed.
/// </summary>
DeltaExecution,

/// <summary>
/// Compiled AST nodes used in node to code.
/// </summary>
NodeToCode,

/// <summary>
/// Compiled AST nodes used in previwing graph.
/// </summary>
PreviewGraph,
}

/// <summary>
/// AstBuilder is a factory class to create different kinds of AST nodes.
/// </summary>
public class AstBuilder : LogSourceBase
{
private readonly IAstNodeContainer nodeContainer;

private ProfilingSession profilingSession = null;

/// <summary>
/// Construct a AstBuilder with AST node contiainer.
/// </summary>
/// <param name="nodeContainer"></param>
internal AstBuilder(IAstNodeContainer nodeContainer)
/// <param name="profilingSession"></param>
internal AstBuilder(IAstNodeContainer nodeContainer, ProfilingSession profilingSession)
{
this.nodeContainer = nodeContainer;
this.profilingSession = profilingSession;
}

// Reverse post-order to sort nodes
Expand Down Expand Up @@ -262,7 +173,11 @@ private static IEnumerable<NodeModel> GetUnvisitedNodes(Dictionary<NodeModel, Ma
}
}

private void CompileToAstNodes(NodeModel node, List<AssociativeNode> resultList, CompilationContext context, bool verboseLogging)
private void CompileToAstNodes(
NodeModel node,
List<AssociativeNode> resultList,
CompilationContext context,
bool verboseLogging)
{

var inputAstNodes = new List<AssociativeNode>();
Expand Down Expand Up @@ -309,8 +224,19 @@ private void CompileToAstNodes(NodeModel node, List<AssociativeNode> resultList,
if (node.State == ElementState.Error)
Log("Error in Node. Not sent for building and compiling");

if (context == CompilationContext.DeltaExecution || context == CompilationContext.PreviewGraph)
if (context == CompilationContext.DeltaExecution)
{
OnAstNodeBuilding(node.GUID);
if (profilingSession != null)
{
profilingSession.RegisterNode(node);
resultList.Add(profilingSession.CreatePreCompilationAstNode(node, inputAstNodes));
}
}
else if (context == CompilationContext.PreviewGraph)
{
Copy link
Member

Choose a reason for hiding this comment

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

so what is a PreviewGraph compilation context?

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 do not know. The code from Sharad called the profiling VMDataBridge code for both, but then explicitly only did anything for DeltaCompute, so I figured it would be clearer to be explicit on the higher level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After discussing today it seems that no-one knows what PreviewGraph is, possibly we need a tech-debt task to figure out.

Copy link
Contributor

Choose a reason for hiding this comment

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

I looked at the preview graph functionality. It is related to node previews and therefore doesn't look like it should be removed any time soon.

OnAstNodeBuilding(node.GUID);
}

#if DEBUG
Validity.Assert(inputAstNodes.All(n => n != null),
Expand All @@ -331,42 +257,62 @@ private void CompileToAstNodes(NodeModel node, List<AssociativeNode> resultList,
}
}

if (verboseLogging)
if (null == astNodes)
{
resultList.AddRange(new AssociativeNode[0]);
}
else if (context == CompilationContext.DeltaExecution)
{
foreach (var n in astNodes)
resultList.AddRange(astNodes);
if (profilingSession != null)
{
Log(n.ToString());
resultList.Add(profilingSession.CreatePostCompilationAstNode(node, inputAstNodes));
}
}

if(null == astNodes)
resultList.AddRange(new AssociativeNode[0]);
else if (context == CompilationContext.DeltaExecution || context == CompilationContext.PreviewGraph)
OnAstNodeBuilt(node.GUID, resultList);
}
else if (context == CompilationContext.PreviewGraph)
{
OnAstNodeBuilt(node.GUID, astNodes);
resultList.AddRange(astNodes);

OnAstNodeBuilt(node.GUID, resultList);
}
else if (context == CompilationContext.NodeToCode)
{
resultList.AddRange(astNodes);
}
else //Inside custom node compilation.
else
{
// Inside custom node compilation
bool notified = false;
foreach (var item in astNodes)
{
if (item is FunctionDefinitionNode)
{
if (!notified)
{
OnAstNodeBuilding(node.GUID);
}

notified = true;
//Register the function node in global scope with Graph Sync data,
//so that we don't have a function definition inside the function def
//of custom node.

// Register the function node in global scope with Graph Sync data,
// so that we don't have a function definition inside the function def
// of custom node.
OnAstNodeBuilt(node.GUID, new[] { item });
}
else
{
resultList.Add(item);
}
}

if (verboseLogging)
{
foreach (var n in resultList)
{
Log(n.ToString());
Copy link
Member

Choose a reason for hiding this comment

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

where does this log? - just curious I know this is old code.

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'm not really sure, I can find out.

}
}
}
}
Expand Down Expand Up @@ -498,8 +444,7 @@ internal void CompileCustomNodeDefinition(
/// <param name="nodeGuid"></param>
private void OnAstNodeBuilding(Guid nodeGuid)
{
if (nodeContainer != null)
nodeContainer.OnCompiling(nodeGuid);
nodeContainer?.OnCompiling(nodeGuid);
Copy link
Member

Choose a reason for hiding this comment

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

hmm, maybe I am confused, did you mean to use ?? or is this just a syntax for null coalescing that also works?

Copy link
Member

Choose a reason for hiding this comment

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

}

/// <summary>
Expand All @@ -509,8 +454,7 @@ private void OnAstNodeBuilding(Guid nodeGuid)
/// <param name="astNodes"></param>
private void OnAstNodeBuilt(Guid nodeGuid, IEnumerable<AssociativeNode> astNodes)
{
if (nodeContainer != null)
nodeContainer.OnCompiled(nodeGuid, astNodes);
nodeContainer?.OnCompiled(nodeGuid, astNodes);
}

private enum MarkFlag
Expand Down
28 changes: 28 additions & 0 deletions src/DynamoCore/Engine/CodeGeneration/CompilationContext.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
namespace Dynamo.Engine.CodeGeneration
{
/// <summary>
/// The context of AST compilation
/// </summary>
public enum CompilationContext
{
/// <summary>
/// No specific context.
/// </summary>
None,

/// <summary>
/// Compiled AST nodes finally will be executed.
/// </summary>
DeltaExecution,

/// <summary>
/// Compiled AST nodes used in node to code.
/// </summary>
NodeToCode,

/// <summary>
/// Compiled AST nodes used in previwing graph.
Copy link
Member

Choose a reason for hiding this comment

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

any hints at what this means?

/// </summary>
PreviewGraph,
}
}
34 changes: 34 additions & 0 deletions src/DynamoCore/Engine/CodeGeneration/CompiledEventArgs.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
using System;
using System.Collections.Generic;
using ProtoCore.AST.AssociativeAST;

namespace Dynamo.Engine.CodeGeneration
{
/// <summary>
/// This event is triggered when a NodeModel has been compiled to a list of
/// AST nodes.
/// </summary>
public class CompiledEventArgs : EventArgs
{
/// <summary>
/// Construct ASTBuiltEventArgs with NodeModel and AST nodes.
/// </summary>
/// <param name="node"></param>
/// <param name="astNodes"></param>
internal CompiledEventArgs(Guid node, IEnumerable<AssociativeNode> astNodes)
Copy link
Member

Choose a reason for hiding this comment

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

same question here around AST type

Copy link
Member

Choose a reason for hiding this comment

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

please change to Node if that causes no issues.

{
Node = node;
AstNodes = astNodes;
}

/// <summary>
/// Guid of node that has been built to AST nodes.
/// </summary>
public Guid Node { get; private set; }
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Built AST nodes.
/// </summary>
public IEnumerable<AssociativeNode> AstNodes { get; private set; }
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
}
}
21 changes: 21 additions & 0 deletions src/DynamoCore/Engine/CodeGeneration/CompilingEventArgs.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using System;

namespace Dynamo.Engine.CodeGeneration
{
/// <summary>
/// This event is triggerred when compiling a NodeModel to AST nodes.
/// </summary>
public class CompilingEventArgs : EventArgs
{
/// Construct ASTBuildingEventArgs with NodeModel.
public CompilingEventArgs(Guid node)
{
Node = node;
}

/// <summary>
/// Guid of NodeModel that is being compiled to AST.
/// </summary>
public Guid Node { get; private set; }
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading