Significant performance improvement by gelverpl · Pull Request #668 · Acumatica/Acuminator

gelverpl · 2026-05-06T19:26:47Z

Proposed changes

NestedInvocationWalker now caches retrieved ISymbol instances using a new SymbolInfoCache.
Fixed a serious issue that was causing a large amount of redundant work: multiple call graph traversals, which in turn caused an excessive number of semantic queries to resolve 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

SymbolInfoCache can optionally collect statistics. Enable it by defining the SYMBOL_INFO_CACHE_STATISTICS constant, then inspect Acuminator.Utilities.Roslyn.SymbolInfoCache.SymbolInfoCacheStatisticsContext.GetStatistics under 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: InvocationExpressionSyntax and MemberAccessExpressionSyntax. Debugging led to VisitMemberAccessExpression.

`foo.Bar()` is both `MemberAccessExpressionSyntax` and `InvocationExpressionSyntax`

By 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):

Speedup ranges from 1.30x to 7.43x (about 4x on average)

Memory allocation / GC pressure:

Allocations reduced by 1.59x to 8.78x (about 5x on average)
Significantly less GC overhead due to drastically fewer allocations

With both fixes applied, traces no longer show GetSymbol as 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.

Copilot

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 SymbolInfoCache to cache SemanticModel.GetSymbolInfo(...) results per ExpressionSyntax.
Updated NestedInvocationWalker.GetSymbol<T> to use the cache and reduced redundant processing in VisitMemberAccessExpression for invocation callees.
Added Rider/JetBrains .idea configuration files under src/.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.

SENya1990 · 2026-05-06T21:07:50Z

+		public StatisticsSnapshot GetSnapshot()
+		{
+			var lookupsTotal = _hitsTotal + _missesTotal;
+			return new StatisticsSnapshot(
+				_cacheId,
+				_owner,
+				_hitsTotal,
+				_missesTotal,
+				_setsTotal,
+				_byTypeCounter,
+				CalcRatio(_hitsTotal, lookupsTotal),
+				CalcRatio(_missesTotal, lookupsTotal)
+			);


@gelverpl this is a good point. It's not really a snapshot if you are passing the original reference which will be mutated after the creation of the snapshot.

SENya1990 · 2026-05-06T21:33:30Z

+		public void Hit() => _hitsTotal++;
+
+		public void Miss() => _missesTotal++;
+
+		public void Set(Type type)
+		{
+			if (!_byTypeCounter.ContainsKey(type))
+			{
+				_byTypeCounter[type] = 1;
+			}
+			else
+			{
+				_byTypeCounter[type]++;
+			}
+			_setsTotal++;
+		}


This shouldn't be a problem for now since each instance of symbol cache and related statistic is owned by only one dedicated nested invocation walker which is executed only by one thread.

Later, when this cache will be expanded to be shared with all analyzers, this indeed will be a problem.

SENya1990 · 2026-05-06T20:14:39Z

+<?xml version="1.0" encoding="UTF-8"?>
+<project version="4">
+  <component name="VcsDirectoryMappings">
+    <mapping directory="$PROJECT_DIR$/.." vcs="Git" />
+  </component>
+</project>


@gelverpl this seems like IDE specific. I think it's better to update the .gitignore file in the repo to not track the entire .idea folder.

SENya1990 · 2026-05-06T20:18:54Z

+public sealed class SymbolInfoCache
+{
+	private readonly Dictionary<ExpressionSyntax, SymbolInfo> _map = new();
+#if SYMBOL_INFO_CACHE_STATISTICS


@gelverpl we spoke today about using some kind of feature flags for performance profiling features. Did you decide not to use it in the end?

SENya1990 · 2026-05-06T20:23:01Z

+namespace Acuminator.Utilities.Roslyn;
+
+/// <summary>
+/// Caches Roslyn symbol lookup results for expression syntax nodes visited by a syntax walker.


IMHO, this can be already a bit generalized to be lookup for syntax nodes, not just expression syntax nodes. There are many other types of nodes that can be passed to the semantic model:
https://learn.microsoft.com/en-us/dotnet/api/microsoft.codeanalysis.semanticmodel?view=roslyn-dotnet-5.0.0

SENya1990 · 2026-05-06T20:25:29Z

+		private static double CalcRatio(long value, long total) => total == 0 ? 0 : (double)value / total;
+	}
+
+	public static class SymbolInfoCacheStatisticsContext


@gelverpl please use 1 type per file convention (except maybe 1 line records types) and move this to a separate file.

SENya1990 · 2026-05-06T20:28:02Z

+
+	public static class SymbolInfoCacheStatisticsContext
+	{
+		private static readonly object Gate = new();


For private fields the current convention is to use underscore-prefixed camel case.

Also it's not very important, but the established naming convention for these objects is to call them _locker

SENya1990 · 2026-05-06T20:32:42Z

+	}
+
+#if SYMBOL_INFO_CACHE_STATISTICS
+	public sealed class SymbolInfoCacheStatistics


Should be moved to a separate file.
The type should probably be marked as internal instead of public.

SENya1990 · 2026-05-06T20:46:42Z

+		}
+	}
+
+	public record StatisticsSnapshot(int CacheId, Type Owner, long HitsTotal, long MissesTotal, long SetsTotal, Dictionary<Type, long> ByTypeCounter, double HitRatio, double MissRatio);


Imho, this DTO can also be in a separate file, but I won't mind if it will be placed in a file with the SymbolInfoCacheStatistics type.

And it also should be internal.

SENya1990 · 2026-05-06T21:02:57Z

+
+		public static StatisticsSnapshot[] GetStatistics()
+		{
+			lock (Gate)


This is not a big improvement, since GetSnapshot doesn't do a lot currently, but you can reduce the time under the lock in this algorithm, if you make under the lock a copy of the Statistics list. Then you can call GetSnapshot on the copy of the list without locking the Statistics collection.

SENya1990 · 2026-05-06T21:23:02Z

+				var result = new StatisticsSnapshot[Statistics.Count];
+				for (var i = 0; i < Statistics.Count; i++)
+				{
+					result[i] = Statistics[i].GetSnapshot();


Since it's a static context, it can be shared by multiple analyzers running on different threads. Which means that while the call to GetStatistics is executed on one thread, the other thread may update the counters for a concrete statistic. This make this collection a bit different from a full snapshot and makes the measurement a bit inaccurate - snapshot for one statistic is collected at the moment X, snapshot for the next statistic will be collected for the moment X + some small delta.

For full snapshots you would need to use sync the updates of the counters with the retrieval of statistics... which will make things more difficult for us and introduce undesirable threads synchronization.

I think it's OK to leave it as is since we are not micro benchmarking and looking for milliseconds. But this should be a conscious decision, so I think a comment about this decision should be added here describing it.

SENya1990 · 2026-05-06T21:27:17Z


 		private readonly ISet<(SyntaxNode, DiagnosticDescriptor)> _reportedDiagnostics = new HashSet<(SyntaxNode, DiagnosticDescriptor)>();

+		private readonly SymbolInfoCache _cache;


There are other caches in this type, so IMHO a more specific name should be used like _symbolsCache.

SENya1990 · 2026-05-06T21:31:40Z

+		private static readonly List<SymbolInfoCacheStatistics> Statistics = new();
+		private static int _nextCacheId;
+
+		internal static SymbolInfoCacheStatistics Register(Type owner)


@gelverpl there can be multiple statistics registered for the same owner (walker). Is it okay to not aggregate them into one statistic?

SENya1990 · 2026-05-06T21:44:13Z

+/// </summary>
+public sealed class SymbolInfoCache
+{
+	private readonly Dictionary<ExpressionSyntax, SymbolInfo> _map = new();


@gelverpl we should cache ISymbol?, there is no need to keep intermediate symbol info DTO.

SENya1990 · 2026-05-06T21:47:34Z

-			var semanticModel = GetSemanticModel(node.SyntaxTree);
-
-			if (semanticModel != null)
+			SymbolInfo? cached = _cache.GetOrCreate(node, () =>


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

SENya1990

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.

SENya1990 · 2026-05-06T22:01:39Z

+			_cacheId = cacheId;
+		}
+
+		public void Hit() => _hitsTotal++;


All counting methods should be annotated with the System.Diagnostics.ConditionalAttribute instead of using pragma directives at their every usage.
https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.conditionalattribute?view=netstandard-2.0

SENya1990 · 2026-05-06T22:01:59Z

+	{
+		if (_map.TryGetValue(key, out SymbolInfo cached))
+		{
+#if SYMBOL_INFO_CACHE_STATISTICS


All counting methods can be annotated with the System.Diagnostics.ConditionalAttribute instead of using pragma directives at their every usage.
https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.conditionalattribute?view=netstandard-2.0

It won't compile without the pragma directive as is though. So, to make it compile and get rid of to many pragmas, maybe you can always keep the field with statistics object and for regular builds initialize it to null or an empty static object.

SENya1990 · 2026-05-06T22:11:28Z

+#if SYMBOL_INFO_CACHE_STATISTICS
+	public sealed class SymbolInfoCacheStatistics
+	{
+		private readonly Dictionary<Type, long> _byTypeCounter = new();


Imho, the name can indicate that the keys are type of syntax nodes. For example, it can be _byNodeTypeCounter

SENya1990 · 2026-05-06T22:12:15Z

+
+		public void Miss() => _missesTotal++;
+
+		public void Set(Type type)


Imho, nodeType is clearer

SENya1990 · 2026-05-06T22:14:27Z

+#endif
+
+		SymbolInfo? potentialValue = factory();
+		if (potentialValue is not null)


Why not caching null results? It's not like the semantic model will return a different result with another call.

gelverpl and others added 6 commits May 3, 2026 19:24

chore: configure JetBrains IDE settings

ef253ce

feat: cache symbol info lookups in NestedInvocationWalker

4ad366f

feat: add cache statistics

5dfc45c

feat: add cache statistics

8a73725

feat: ignore invocation expressions in VisitMemberAccessExpression

85f2a69

Merge remote-tracking branch 'origin/playground' into playground

51048e9

SENya1990 requested a review from Copilot May 6, 2026 19:47

Copilot started reviewing on behalf of SENya1990 May 6, 2026 19:47 View session

Copilot AI reviewed May 6, 2026

View reviewed changes

SENya1990 self-requested a review May 6, 2026 20:10

SENya1990 reviewed May 6, 2026

View reviewed changes

SENya1990 requested changes May 6, 2026

View reviewed changes

SENya1990 reviewed May 6, 2026

View reviewed changes


		private readonly ISet<(SyntaxNode, DiagnosticDescriptor)> _reportedDiagnostics = new HashSet<(SyntaxNode, DiagnosticDescriptor)>();

		private readonly SymbolInfoCache _cache;


		public void Miss() => _missesTotal++;

		public void Set(Type type)

Conversation

gelverpl commented May 6, 2026

Proposed changes

Motivation

Caching

Cache statistics

foo.Bar() is both MemberAccessExpressionSyntax and InvocationExpressionSyntax

Benchmarks

Conclusion

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

`foo.Bar()` is both `MemberAccessExpressionSyntax` and `InvocationExpressionSyntax`

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading