DYN-1754: As a developer I want detailed per node profiling during perf benchmarks #9667
Conversation
| /// <summary> | ||
| /// AstBuilder is a factory class to create different kinds of AST nodes. | ||
| /// </summary> | ||
| public class AstBuilder : LogSourceBase | ||
| { | ||
| private readonly IAstNodeContainer nodeContainer; | ||
|
|
||
| internal ProfilingSession profilingSession = null; |
There was a problem hiding this comment.
is this internal for a specific reason? (sorry have not looked further down to see if that's the case yet)
There was a problem hiding this comment.
No, has been changed to be private now
|
Hi @ColinDayOrg ! Hope you don't mind me chiming in but I am very interested on this feature. We participated on the London Dynamo Hackathon with a diagnostic toolkit for Dynamo in mind, and we based our work on the existing RechargeEast2016 branch, which I think is the same you used for this branch. Based on our experience, I wanted to share some comments:
I understand @mjkkirschner comments on making some of these classes I found a bit obscure and difficult to follow and implement the logic using the Not sure how feasible it would be to implement these events or if it would even be a good idea from the Dynamo API perspective, but we are more than happy to help on anything we can, so maybe we can release the |
| @@ -63,6 +63,8 @@ internal bool Initialize(EngineController controller, WorkspaceModel workspace) | |||
| if (graphSyncData == null) | |||
| return false; | |||
|
|
|||
| engineController.UpdateProfilingData(workspace.Nodes); | |||
There was a problem hiding this comment.
is there a more specific location this can be handled in - the profiling session itself or is this the cleanest?
There was a problem hiding this comment.
Moved to the profiling session, engine controller methods cleaned up and test updated to reflect the change.
| @@ -69,7 +70,7 @@ private void OnTraceReconciliationComplete(TraceReconciliationEventArgs e) | |||
| /// </summary> | |||
| public static CompilationServices CompilationServices; | |||
|
|
|||
| internal ProfilingSession profilingSession; | |||
| internal ProfilingSession profilingSession = null; | |||
There was a problem hiding this comment.
Does the ProfilingSession need to be owned both by AstBuilder and EngineController?
There was a problem hiding this comment.
I have moved the ownership of the profiling data into the ASTBuilder only. Latest changes have just been pushed.
|
Thanks Colin, LGTM. |
|
@ColinDayOrg - If I wrote an extension today how would I actually collect any timing data, is the entry point on engineController meant to be internal? Is there another entry point I am missing? Or did you just want to keep it internal until we start working on the extension? |
| @@ -71,5 +71,28 @@ public void TestProfilingSingleNode() | |||
| engineController.EnableProfiling(false, homeWorkspace, nodes); | |||
| Assert.IsNull(engineController.ProfilingSession); | |||
| } | |||
|
|
|||
| [Test] | |||
| public void TestProfilingSingleNodePublicMethodsOnly() | |||
There was a problem hiding this comment.
I feel this test is redundant; it's testing what is already being tested in the first test case you added, is it not?
There was a problem hiding this comment.
It is a little redundant, but it is testing the public data accessor as opposed to the other test that is using internal methods.
ColinDayOrg
changed the title
…rf benchmarks (#9667) * Update changes from librarie.js to fix QNTM-3710 * Initial add and code cleanup for profiling data * Add profiling changes * Add missing summaries and accessor items * Add an accessor for the profiling data to the profiling session * Make profiling session private * Improve infrastructure (switch to property, make DateTime property nullable) * Add test for pre-run, post-run, and node deletion post-run * Remove unused AstCompilationEvents class * Remove unused CompilationEventArgs class * Rename IProfilingData to IProfilingExecutionTimeData * Remove derivation from NotificationObject * Restrict access to NodeProfilingData methods as much as possible * Pull strings out to constants * Update naming * Obsolete Node property getter and add NodeId property getter * Change property name from Node to NodeId and obsolete Node to avoid confusion * Move profiling data housekeeping out of the engine controller * Add the abillity to switch profiling on/off (off by default), including tests * Move ownership of profiling data to the ast builder * minor code fixes * Fix typo * Add public interface to get profiling data * Add test for public data access method
Please Note:
DynamoRevitrepo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after aLGTMlabel is added to the PR.Purpose
This PR uses the VMDataBridge to add extra AST nodes to the beginning and end of the list of AST nodes for each workspace node that store the start and end time of compile/run in a data structure for each node for profiling reasons.
Note that tests will be added, they are just being cleaned up.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner