Significant performance improvement #668

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

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 · 2026-05-06T21:44:13Z

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

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

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:14:27Z

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

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

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

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

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-06T20:28:02Z

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-06T21:31:40Z

@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:02:57Z

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

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-06T20:46:42Z

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.

-Original file line number
+Diff line change
@@ Expand Up @@
     		private readonly ISet<(SyntaxNode, DiagnosticDescriptor)> _reportedDiagnostics = new HashSet<(SyntaxNode, DiagnosticDescriptor)>();
+    		private readonly SymbolInfoCache _cache;
             /// <summary>
             /// Cancellation token
             /// </summary>
@@ Expand Down Expand Up @@
     			//Use lazy to avoid calling virtual methods inside the constructor
     			_typesToBypass = new Lazy<HashSet<INamedTypeSymbol>>(valueFactory: GetTypesToBypass, isThreadSafe: false);
+    			_cache = new SymbolInfoCache(GetType());
     		}
     		/// <summary>
@@ Expand Down Expand Up @@
     		protected virtual T? GetSymbol<T>(ExpressionSyntax node)
     			where T : class, ISymbol
     		{
-    			var semanticModel = GetSemanticModel(node.SyntaxTree);
-    			if (semanticModel != null)
+    			SymbolInfo? cached = _cache.GetOrCreate(node, () =>
     			{
-    				var symbolInfo = semanticModel.GetSymbolInfo(node, CancellationToken);
+    				SemanticModel? semanticModel = GetSemanticModel(node.SyntaxTree);
+    				return semanticModel?.GetSymbolInfo(node, CancellationToken);
+    			});
-    				if (symbolInfo.Symbol is T symbol)
+    			if (cached is not null)
+    			{
+    				if (cached.Value.Symbol is T symbol)
     				{
     					return symbol;
     				}
-    				if (!symbolInfo.CandidateSymbols.IsEmpty)
+    				if (!cached.Value.CandidateSymbols.IsEmpty)
     				{
-    					return symbolInfo.CandidateSymbols.OfType<T>().FirstOrDefault();
+    					return cached.Value.CandidateSymbols.OfType<T>().FirstOrDefault();
     				}
     			}
@@ Expand Down Expand Up @@
     		public override void VisitMemberAccessExpression(MemberAccessExpressionSyntax node)
     		{
-    			VisitPropertyOrIndexerAccessExpression(node);
-    			base.VisitMemberAccessExpression(node);
+    			if (node.Parent is InvocationExpressionSyntax invocation && invocation.Expression == node)
+    			{
+    				// we already visit this node by VisitInvocationExpression, so we just skip it here
+    				base.VisitMemberAccessExpression(node);
+    			}
+    			else
+    			{
+    				VisitPropertyOrIndexerAccessExpression(node);
+    				base.VisitMemberAccessExpression(node);
+    			}
     		}
     		public override void VisitElementAccessExpression(ElementAccessExpressionSyntax node)
@@ Expand Down @@

-Original file line number
+Diff line change
@@ -0,0 +1,145 @@
+    using System;
+    using System.Collections.Generic;
+    using System.Threading;
+    using Microsoft.CodeAnalysis;
+    using Microsoft.CodeAnalysis.CSharp.Syntax;
+    namespace Acuminator.Utilities.Roslyn;
+    /// <summary>
+    /// Caches Roslyn symbol lookup results for expression syntax nodes visited by a syntax walker.
+    /// </summary>
+    public sealed class SymbolInfoCache
+    {
+    	private readonly Dictionary<ExpressionSyntax, SymbolInfo> _map = new();
+    #if SYMBOL_INFO_CACHE_STATISTICS
+    	private readonly SymbolInfoCacheStatistics _statistics;
+    #endif
+    	public SymbolInfoCache(Type owner)
+    	{
+    #if SYMBOL_INFO_CACHE_STATISTICS
+    		_statistics = SymbolInfoCacheStatisticsContext.Register(owner);
+    #endif
+    	}
+    	/// <summary>
+    	/// Gets the cached symbol information for the specified expression, or creates and stores it using the provided factory.
+    	/// </summary>
+    	public SymbolInfo? GetOrCreate(ExpressionSyntax key, Func<SymbolInfo?> factory)
+    	{
+    		if (_map.TryGetValue(key, out SymbolInfo cached))
+    		{
+    #if SYMBOL_INFO_CACHE_STATISTICS
+    			_statistics.Hit();
+    #endif
+    			return cached;
+    		}
+    #if SYMBOL_INFO_CACHE_STATISTICS
+    		_statistics.Miss();
+    #endif
+    		SymbolInfo? potentialValue = factory();
+    		if (potentialValue is not null)
+    		{
+    			_map[key] = potentialValue.Value;
+    #if SYMBOL_INFO_CACHE_STATISTICS
+    			_statistics.Set(key.GetType());
+    #endif
+    			return potentialValue;
+    		}
+    		return null;
+    	}
+    #if SYMBOL_INFO_CACHE_STATISTICS
+    	public sealed class SymbolInfoCacheStatistics
+    	{
+    		private readonly Dictionary<Type, long> _byTypeCounter = new();
+    		private readonly Type _owner;
+    		private readonly int _cacheId;
+    		private long _hitsTotal;
+    		private long _missesTotal;
+    		private long _setsTotal;
+    		public SymbolInfoCacheStatistics(int cacheId, Type owner)
+    		{
+    			_owner = owner;
+    			_cacheId = cacheId;
+    		}
+    		public void Hit() => _hitsTotal++;
+    		public void Miss() => _missesTotal++;
+    		public void Set(Type type)
+    		{
+    			if (!_byTypeCounter.ContainsKey(type))
+    			{
+    				_byTypeCounter[type] = 1;
+    			}
+    			else
+    			{
+    				_byTypeCounter[type]++;
+    			}
+    			_setsTotal++;
+    		}
+    		public StatisticsSnapshot GetSnapshot()
+    		{
+    			var lookupsTotal = _hitsTotal + _missesTotal;
+    			return new StatisticsSnapshot(
+    				_cacheId,
+    				_owner,
+    				_hitsTotal,
+    				_missesTotal,
+    				_setsTotal,
+    				_byTypeCounter,
+    				CalcRatio(_hitsTotal, lookupsTotal),
+    				CalcRatio(_missesTotal, lookupsTotal)
+    			);
+    		}
+    		private static double CalcRatio(long value, long total) => total == 0 ? 0 : (double)value / total;
+    	}
+    	public static class SymbolInfoCacheStatisticsContext
+    	{
+    		private static readonly object Gate = new();
+    		private static readonly List<SymbolInfoCacheStatistics> Statistics = new();
+    		private static int _nextCacheId;
+    		internal static SymbolInfoCacheStatistics Register(Type owner)
+    		{
+    			var cacheId = Interlocked.Increment(ref _nextCacheId);
+    			var statistics = new SymbolInfoCacheStatistics(cacheId, owner);
+    			lock (Gate)
+    			{
+    				Statistics.Add(statistics);
+    			}
+    			return statistics;
+    		}
+    		public static StatisticsSnapshot[] GetStatistics()
+    		{
+    			lock (Gate)
+    			{
+    				var result = new StatisticsSnapshot[Statistics.Count];
+    				for (var i = 0; i < Statistics.Count; i++)
+    				{
+    					result[i] = Statistics[i].GetSnapshot();
+    				}
+    				return result;
+    			}
+    		}
+    	}
+    	public record StatisticsSnapshot(int CacheId, Type Owner, long HitsTotal, long MissesTotal, long SetsTotal, Dictionary<Type, long> ByTypeCounter, double HitRatio, double MissRatio);
+    #endif
+    }

Significant performance improvement #668

Are you sure you want to change the base?

Uh oh!

Significant performance improvement #668

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SENya1990 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

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!

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

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!

SENya1990 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SENya1990 May 6, 2026

Choose a reason for hiding this comment

Uh oh!

SENya1990 May 6, 2026

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

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading

SENya1990 May 6, 2026 •

edited

Loading