Improve graph opening times, reduce reflecion calls #15761
Conversation
dimven-adsk
changed the title
UI Smoke TestsTest: success. 11 passed, 0 failed. |
| /// <summary> | ||
| /// The method returns the assembly name from which the node originated. | ||
| /// </summary> | ||
| /// <returns>Assembly Name</returns> | ||
| internal virtual AssemblyName GetNameOfAssemblyReferencedByNode() | ||
| internal virtual string GetNameOfAssemblyReferencedByNode() |
There was a problem hiding this comment.
@dimven-adsk thanks for these changes - the cache is a good idea.
What I would prefer is that unless the change from AssemlyName -> String has clear performane benefits I would say to keep the signature of these methods the same.
I assume that the real perf gain here is from the cache and not using the GetType().Assembly / strange double lookup / allocation of AssemblyName. It seems if we just cache the AssemblyName we'll get most of the benefit, though I guess the AssemblyName objects are quite wasteful at the moment though I can see us using some of these other properties in the future and I'd rather avoid spreading out the cost of allocating these later in a less predictable manner, or doing error prone string munging - thoughts?
There was a problem hiding this comment.
I would echo the same:
What I would prefer is that unless the change from AssemlyName -> String has clear performane benefits I would say to keep the signature of these methods the same.
| @@ -1223,18 +1223,21 @@ protected NodeModel(IEnumerable<PortModel> inPorts, IEnumerable<PortModel> outPo | |||
| RaisesModificationEvents = true; | |||
| } | |||
|
|
|||
|
|
|||
| internal string cachedAsmName = null; | |||
There was a problem hiding this comment.
I would make this protected
There was a problem hiding this comment.
ANother name for the member could be
NameOfAssemblyReferencedByNode to match the existing method
| if (string.IsNullOrEmpty(cachedAsmName)) | ||
| { | ||
| cachedAsmName = GetType().Assembly.FullName; | ||
| } |
There was a problem hiding this comment.
could probably do it
NameOfAssemblyReferencedByNode ??= GetType().Assembly.GetName();
return NameOfAssemblyReferencedByNode;
or even
internal virtual AssemblyName GetNameOfAssemblyReferencedByNode() => NameOfAssemblyReferencedByNode ??= GetType().Assembly.GetName();
|
I've implemented the feedback and reverted the code back to using the full AssemblyName. You are totally correct, there is no observable difference between using that and a string:
I imagine the only difference would be a few extra bytes of memory for storing the AN instances, which would be insignificant :) |
Purpose
This PR targets a low-hanging fruit for improving the graph open (and saving) performance by reducing the CLR reflection calls that are used for getting a node's package assembly name (used in figuring out dependencies and used packages in a graph).
These reflection calls are not insignificant and the result can be cached (because it is not possible to change a node's assembly during runtime). If that ever changes, then you can override the method :) . Furthermore, the current implementation actually ignores the existing assembly reference and instead reloads it again from the referenced location path, which seems completely pointless.
A further improvement can be made by storing only the assembly's name as a string instead of the entire
AssemblyNameclass instance, since we're only using the name.The hot path before:
And after:
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Use N/A to indicate that the changes in this pull request do not apply to Release Notes. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of