WIP, Highlighting: hot methods and invocation of costly methods #816
Conversation
…ox to disable analyzer to plugin options.
…ur into step with finding hotroot methods
|
Briefly looked at the files and couldn't find any highlighting tests. Please add. |
|
It's very cool to have such inspections 👍 Though I have concerns about current implementation. In ReSharper there is a daemon stage which also builds call graph and is cross-procedural: While current implementation of |
| { | ||
| switch (descendantsEnumerator.Current) | ||
| { | ||
| // skip class declaration because all derived sym |
| break; | ||
| case IInvocationExpression invocationExpression: | ||
| // we should find 'StartCoroutine' method, because passed symbol will be hot too | ||
| var reference = (invocationExpression.InvokedExpression as IReferenceExpression)?.Reference; |
There was a problem hiding this comment.
there is an InvocationExpressionReference property available on IInvocationExpression
There was a problem hiding this comment.
InvocationExpressionReference has invalid DocumentRange (-1,-1)
(invocationExpression.InvokedExpression as IReferenceExpression)?.Reference has valid range for document
| if (reference == null) | ||
| break; | ||
|
|
||
| var info = reference.Resolve(); |
There was a problem hiding this comment.
ResolveResultWithInfo can be deconstructed:
var (declaredElement, substitution, resolveErrorType) = reference.Resolve();
|
|
||
| // 'StartCoroutine' has overload with string. We have already attached reference, so get symbol from | ||
| // reference | ||
| if (firstArgument.ConstantValue.IsString()) |
There was a problem hiding this comment.
code under condition can be extracted into separate method
| if (method == null) | ||
| break; | ||
|
|
||
| var declarations = method.GetDeclarationsIn(sourceFile).Where(t => t.GetSourceFile() == sourceFile); |
There was a problem hiding this comment.
GetDeclarationsIn(sourceFile) is called, Where seems to be redundant. But if type declaration is partial then method can be located in another source file.
| private readonly IHighlightingConsumer myConsumer; | ||
| private readonly int myMaxDepth; | ||
|
|
||
| public HotMethodAnalyzer(IPsiSourceFile sourceFile, IHighlightingConsumer consumer, int maxDepth) |
There was a problem hiding this comment.
maxDepth could be a constant declared in HotMethodAnalyzer
| #region CostlyMethodInvocationInspection | ||
|
|
||
| ISet<string> knownCostlyMethods = null; | ||
| if (containingType.GetClrName().Equals(KnownTypes.Component)) |
There was a problem hiding this comment.
containingType.GetClrName() is called several types, but could be saved into local variable
|
|
||
| IExpressionType expressionType = null; | ||
|
|
||
| if (leftOperand.ConstantValue(new UniversalContext(equalityExpressionParam)).IsNull()) |
There was a problem hiding this comment.
there is a ConstantValue property, no need to call extension method
| if (containingType == null) | ||
| return; | ||
|
|
||
| #region CostlyMethodInvocationInspection |
There was a problem hiding this comment.
Regions are generally bad. Use methods to structure code.
| public int Depth { get; set; } | ||
|
|
||
| // Inverted call graph for costly reachable mark propagation after visit AST | ||
| public IReadOnlyDictionary<IDeclaredElement, HashSet<IDeclaredElement>> InvertedCallGraph |
There was a problem hiding this comment.
Creating a copy of the whole dictionary isn't very good in terms of memory consumption. Maybe return the original dictionary?
|
|
||
| private static List<IMethodDeclaration> FindHotRootMethods([NotNull]ICSharpFile file,[NotNull] IPsiSourceFile sourceFile) | ||
| { | ||
| var result = new List<IMethodDeclaration>(); |
There was a problem hiding this comment.
Often it's good to use LocaList<T> instead of List<T>, it doesn't allocate without need. Once you populated local list you can then invoke .ReadOnlyList() on it to get IReadOnlyList<T>.
| var descendantsEnumerator = file.Descendants(); | ||
| while (descendantsEnumerator.MoveNext()) | ||
| { | ||
| var checkForInterrupt = InterruptableActivityCookie.GetCheck().NotNull("checkForInterrupt != null"); |
There was a problem hiding this comment.
It's better to get check for interrupt in outer method and use it here and in HotMethodAnalyzer. Also it makes sense to call checkForInterrupt only in switch cases where we are doing some interesting work and not on every descendant node.
TODO