Significant performance improvement#668
Conversation
There was a problem hiding this comment.
Pull request overview
This PR targets analyzer performance hotspots in NestedInvocationWalker by reducing redundant Roslyn semantic symbol queries and avoiding duplicate traversal work, with an additional (unrelated) addition of Rider/JetBrains project configuration files.
Changes:
- Introduced
SymbolInfoCacheto cacheSemanticModel.GetSymbolInfo(...)results perExpressionSyntax. - Updated
NestedInvocationWalker.GetSymbol<T>to use the cache and reduced redundant processing inVisitMemberAccessExpressionfor invocation callees. - Added Rider/JetBrains
.ideaconfiguration files undersrc/.idea/.idea.Acuminator/.idea/.
Reviewed changes
Copilot reviewed 2 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Acuminator/Acuminator.Utilities/Roslyn/SymbolInfoCache.cs | New symbol-info cache (and optional statistics collection) used to reduce repeated semantic queries. |
| src/Acuminator/Acuminator.Utilities/Roslyn/NestedInvocationWalker.cs | Uses the cache in GetSymbol<T> and avoids redundant member-access processing for foo.Bar() patterns. |
| src/.idea/.idea.Acuminator/.idea/vcs.xml | Adds Rider VCS mapping configuration. |
| src/.idea/.idea.Acuminator/.idea/indexLayout.xml | Adds Rider index layout configuration. |
| src/.idea/.idea.Acuminator/.idea/.name / .gitignore | Adds Rider project name and IDE-local ignore rules. |
Files not reviewed (4)
- src/.idea/.idea.Acuminator/.idea/.gitignore: Language not supported
- src/.idea/.idea.Acuminator/.idea/.name: Language not supported
- src/.idea/.idea.Acuminator/.idea/indexLayout.xml: Language not supported
- src/.idea/.idea.Acuminator/.idea/vcs.xml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// </summary> | ||
| public sealed class SymbolInfoCache | ||
| { | ||
| private readonly Dictionary<ExpressionSyntax, SymbolInfo> _map = new(); |
There was a problem hiding this comment.
@gelverpl we should cache ISymbol?, there is no need to keep intermediate symbol info DTO.
There was a problem hiding this comment.
I find this type more convenient for analysis.
Won't be fixed.
| var semanticModel = GetSemanticModel(node.SyntaxTree); | ||
|
|
||
| if (semanticModel != null) | ||
| SymbolInfo? cached = _cache.GetOrCreate(node, () => |
There was a problem hiding this comment.
Here should be the retrieval of the symbol from cache, not SymbolInfo.
As I wrote in https://github.com/Acumatica/Acuminator/pull/668/changes#r3197690911, cache should keep ISymbol?.
Also, since the symbol is not always cached, IMHO it's better to call it just symbol
There was a problem hiding this comment.
I find this type more convenient for analysis.
Won't be fixed.
SENya1990
left a comment
There was a problem hiding this comment.
Overall, the PR content is good, there is no need to significantly change anything.
But I have some remarks that should be processed:
- about the type of cached data,
- some minor remarks about the concurrent code
- remarks about the code style.
Proposed changes
NestedInvocationWalkernow caches retrievedISymbolinstances using a newSymbolInfoCache.ISymbol.Motivation
While profiling the analyzer, I noticed extremely high memory usage, and running solution analysis from the CLI took more than 40 minutes.


dotTrace pointed to
Acuminator.Utilities.Roslyn.NestedInvocationWalker.GetSymbol(ExpressionSyntax)as a primary hotspot:Memory profiling confirmed the same method:

Caching
As a load-reducing measure, I implemented caching. This significantly reduced both execution time and memory consumption.
Yes, caching is often a last-resort approach and ideally shouldn't be here. However, when implemented carefully it can also provide valuable diagnostic data that helps guide further optimization work.
Cache statistics
SymbolInfoCachecan optionally collect statistics. Enable it by defining theSYMBOL_INFO_CACHE_STATISTICSconstant, then inspectAcuminator.Utilities.Roslyn.SymbolInfoCache.SymbolInfoCacheStatisticsContext.GetStatisticsunder the debugger.I intentionally did not add any logging of it because the statistics can become very large. It's much more practical to analyze it interactively (e.g., via LINQ in a debug session).
While digging into the stats, I noticed something weird: the same expression was cached under two different key types:
InvocationExpressionSyntaxandMemberAccessExpressionSyntax. Debugging led toVisitMemberAccessExpression.foo.Bar()is bothMemberAccessExpressionSyntaxandInvocationExpressionSyntaxBy filtering out redundant invocation processing inside
MemberAccessExpressionSyntax, I added an additional fix. This change, similarly to caching, delivered a substantial improvement in both time and memory.Benchmarks
I measured the impact of each fix independently, as well as both together.
Total runtime (wall-clock):

Memory allocation / GC pressure:

With both fixes applied, traces no longer show


GetSymbolas a top hotspot; instead, the cache path (GetOrCreate) appears, and overall memory usage is dramatically improved.Conclusion
Even though caching contributes less after the second fix, I'd still like to keep it (at least for upcoming optimization work) as a way to collect detailed statistics. The goal is to reach a hit rate that is 0 or as close to 0 as possible (i.e., eliminate the underlying redundant symbol queries altogether).
I plan to continue working on this area and believe we can achieve even more significant improvements.