Avoid allocations from EvaluationTarget.GetId

GetId is used as part of a loop, and every call to it allocates a new string and possibly invokes Scope.Version(). We can avoid that string allocation by returning the two component pieces as a tuple, and by comparing the individual pieces, if the templates are different we can short-circuit and avoid the Version() invocation.
microsoft · Nov 16, 2024 · 3548c7d · 3548c7d
1 parent e449b3c
commit 3548c7d
Show file tree

Hide file tree

Showing 3 changed files with 44 additions and 20 deletions.
diff --git a/libraries/Microsoft.Bot.Builder.LanguageGeneration/EvaluationTarget.cs b/libraries/Microsoft.Bot.Builder.LanguageGeneration/EvaluationTarget.cs
@@ -1,7 +1,9 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 
+using System;
 using System.Collections.Generic;
+using System.Linq;
 using AdaptiveExpressions.Memory;
 
 namespace Microsoft.Bot.Builder.LanguageGeneration
@@ -46,14 +48,40 @@ public EvaluationTarget(string templateName, IMemory scope)
         /// </value>
         public IMemory Scope { get; set; }
 
+        /// <summary>Throws an exception if any of the specified targets equals the specified id such that a loop is detected.</summary>
+        /// <param name="targets">The targets to compare.</param>
+        /// <param name="id">The id against which the targets should be compared.</param>
+        /// <param name="templateName">The template name to include in any resulting exception.</param>
+        public static void ThrowIfLoopDetected(
+            Stack<EvaluationTarget> targets,
+            (string TemplateName, string ScopeVersion) id,
+            string templateName)
+        {
+            foreach (var target in targets)
+            {
+                if (target.TemplateName == id.TemplateName && target.Scope?.Version() == id.ScopeVersion)
+                {
+                    throw new InvalidOperationException($"{TemplateErrors.LoopDetected} {string.Join(" => ", targets.Reverse().Select(e => e.TemplateName))} => {templateName}");
+                }
+            }
+        }
+
+        /// <summary>Combines the components of the specified <paramref name="id"/> to create a string key.</summary>
+        /// <param name="id">The id retrieved from <see cref="GetId"/>.</param>
+        /// <returns>The created string key.</returns>
+        public static string CreateKey((string TemplateName, string ScopeVersion) id)
+        {
+            return id.TemplateName + id.ScopeVersion;
+        }
+
         /// <summary>
         /// Get current instance id. If two target has the same Id,
         /// we can say they have the same template evaluation.
         /// </summary>
         /// <returns>Id.</returns>
-        public string GetId()
+        public (string TemplateName, string ScopeVersion) GetId()
         {
-            return TemplateName + Scope?.Version();
+            return (TemplateName, Scope?.Version());
         }
     }
 }
diff --git a/libraries/Microsoft.Bot.Builder.LanguageGeneration/Evaluator.cs b/libraries/Microsoft.Bot.Builder.LanguageGeneration/Evaluator.cs
@@ -108,20 +108,18 @@ public object EvaluateTemplate(string inputTemplateName, object scope)
             var templateTarget = new EvaluationTarget(templateName, memory);
             var currentEvaluateId = templateTarget.GetId();
 
-            if (_evaluationTargetStack.Any(e => e.GetId() == currentEvaluateId))
-            {
-                throw new InvalidOperationException($"{TemplateErrors.LoopDetected} {string.Join(" => ", _evaluationTargetStack.Reverse().Select(e => e.TemplateName))} => {templateName}");
-            }
+            EvaluationTarget.ThrowIfLoopDetected(_evaluationTargetStack, currentEvaluateId, templateName);
 
+            string key = null;
             object result = null;
             var hasResult = false;
             if (!reExecute)
             {
                 if (_lgOptions.CacheScope == LGCacheScope.Global)
                 {
-                    if (_cachedResult.ContainsKey(currentEvaluateId))
+                    key ??= EvaluationTarget.CreateKey(currentEvaluateId);
+                    if (_cachedResult.TryGetValue(key, out result))
                     {
-                        result = _cachedResult[currentEvaluateId];
                         hasResult = true;
                     }
                 }
@@ -132,9 +130,9 @@ public object EvaluateTemplate(string inputTemplateName, object scope)
                     {
                         previousEvaluateTarget = CurrentTarget();
 
-                        if (previousEvaluateTarget.CachedEvaluatedChildren.ContainsKey(currentEvaluateId))
+                        key ??= EvaluationTarget.CreateKey(currentEvaluateId);
+                        if (previousEvaluateTarget.CachedEvaluatedChildren.TryGetValue(key, out result))
                         {
-                            result = previousEvaluateTarget.CachedEvaluatedChildren[currentEvaluateId];
                             hasResult = true;
                         }
                     }
@@ -162,13 +160,15 @@ public object EvaluateTemplate(string inputTemplateName, object scope)
                 {
                     if (_lgOptions.CacheScope == LGCacheScope.Global)
                     {
-                        _cachedResult[currentEvaluateId] = result;
+                        key ??= EvaluationTarget.CreateKey(currentEvaluateId);
+                        _cachedResult[key] = result;
                     }
                     else if (_lgOptions.CacheScope == null || _lgOptions.CacheScope == LGCacheScope.Local)
                     {
                         if (_evaluationTargetStack.Count > 0)
                         {
-                            _evaluationTargetStack.Peek().CachedEvaluatedChildren[currentEvaluateId] = result;
+                            key ??= EvaluationTarget.CreateKey(currentEvaluateId);
+                            _evaluationTargetStack.Peek().CachedEvaluatedChildren[key] = result;
                         }
                     }
                 }
@@ -524,13 +524,14 @@ private EvaluationTarget CurrentTarget() =>
             if (currentTemplate != null)
             {
                 var source = currentTemplate.SourceRange.Source;
-                if (expressionContext != null && _lgOptions.OnEvent != null)
+                if (expressionContext != null && _lgOptions.OnEvent != null && currentTemplate.Expressions.Count != 0)
                 {
                     var lineOffset = currentTemplate.SourceRange.Range.Start.Line;
                     var sourceRange = new SourceRange(expressionContext, source, lineOffset);
                     var expressionRef = new ExpressionRef(exp, sourceRange);
+                    var expressionRefId = expressionRef.GetId();
 
-                    var expression = currentTemplate.Expressions.FirstOrDefault(u => u.GetId() == expressionRef.GetId());
+                    var expression = currentTemplate.Expressions.FirstOrDefault(u => u.Equals(expressionRefId));
                     if (expression != null)
                     {
                         _lgOptions.OnEvent(expression, new BeginExpressionEvaluationArgs { Source = source, Expression = exp });

diff --git a/libraries/Microsoft.Bot.Builder.LanguageGeneration/Expander.cs b/libraries/Microsoft.Bot.Builder.LanguageGeneration/Expander.cs
@@ -89,12 +89,7 @@ public List<object> ExpandTemplate(string templateName, object scope)
 
             var templateTarget = new EvaluationTarget(templateName, memory);
 
-            var currentEvaluateId = templateTarget.GetId();
-
-            if (_evaluationTargetStack.Any(e => e.GetId() == currentEvaluateId))
-            {
-                throw new InvalidOperationException($"{TemplateErrors.LoopDetected} {string.Join(" => ", _evaluationTargetStack.Reverse().Select(e => e.TemplateName))} => {templateName}");
-            }
+            EvaluationTarget.ThrowIfLoopDetected(_evaluationTargetStack, templateTarget.GetId(), templateName);
 
             // Using a stack to track the evaluation trace
             _evaluationTargetStack.Push(templateTarget);