Trim per-evaluation work in the JavaScript evaluator (#7892)
* perf(javascript): trim per-evaluation work in the JavaScript evaluator A Jint engine is built for every expression evaluation, so anything done during setup is paid for on every evaluation. Four pieces of that work are avoidable: * The three `IObjectConverter` implementations are stateless but were allocated fresh for every engine. They are now shared static instances. * Every prepared-script cache lookup — including hits — computed a SHA-256 hash of the expression text, base64-encoded it and concatenated a prefix, purely to build the cache key. Using a dedicated key type instead keeps the entries distinct from other users of the shared cache while letting the expression itself be the key, so a hit is a dictionary lookup. Looking the entry up directly rather than through `GetOrCreate` also keeps the factory closure off the hit path. * `ObjectConverterHelper.ConvertToJsObject` built an explicit `PropertyDescriptor` per property and called `DefineOwnProperty`. `CreateDataProperty` is public, produces exactly the same writable/enumerable/configurable descriptor, and is the engine's fast path for it. * The variable write-back resolved the workflow input names — walking the whole activity execution context ancestor chain — before checking whether there was anything to write back. Only variables the expression actually referenced are copied into the engine, so for the common case of an expression that never mentions `variables.` the container is empty and all of that work is wasted. The input names are also now looked up through a set rather than a list. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0179sA2T7HuRfRfSc2JirFik * test(javascript): pin the parse-failure test to the same exception every time Calling `ThrowsAnyAsync<Exception>` twice only proved that both evaluations threw something, which is exactly the assertion a poisoned cache would still satisfy: had the failed preparation left a null or half-built entry behind, the second evaluation would have failed too, just with a different exception. The test now captures both exceptions and asserts they are the same type with the same message, so "keeps reporting the same parse failure" is what is actually checked. The message is stable to compare: it is `Could not prepare script: Unexpected end of input (1:9)`, and since both evaluations run the identical script literal the position is identical as well. No file or path detail is involved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0179sA2T7HuRfRfSc2JirFik --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
14173a19eb
commit
2e9a3ad04f
|
|
@ -46,7 +46,13 @@ public partial class ConfigureEngineWithVariables(IOptions<JintOptions> options)
|
|||
var context = notification.Context;
|
||||
var engine = notification.Engine;
|
||||
var variablesContainer = (IDictionary<string, object?>)engine.GetValue("variables").ToObject()!;
|
||||
var inputNames = GetInputNames(context).FilterInvalidVariableNames().Distinct().ToList();
|
||||
|
||||
// Only the variables the expression actually referenced were copied in, so an expression that never
|
||||
// mentions "variables." has nothing to copy back and does not need the input names resolved at all.
|
||||
if (variablesContainer.Count == 0)
|
||||
return;
|
||||
|
||||
var inputNames = GetInputNames(context).FilterInvalidVariableNames().ToHashSet(StringComparer.Ordinal);
|
||||
|
||||
foreach (var (variableName, variableValue) in variablesContainer)
|
||||
{
|
||||
|
|
|
|||
|
|
@ -1,19 +1,14 @@
|
|||
using System.Collections;
|
||||
using System.Dynamic;
|
||||
using Elsa.Extensions;
|
||||
using Elsa.Expressions.JavaScript.Options;
|
||||
using Jint;
|
||||
using Jint.Native;
|
||||
using Jint.Native.Object;
|
||||
using Jint.Runtime.Descriptors;
|
||||
using Microsoft.Extensions.Options;
|
||||
|
||||
namespace Elsa.Expressions.JavaScript.Helpers;
|
||||
|
||||
internal static class ObjectConverterHelper
|
||||
{
|
||||
|
||||
|
||||
public static object? ProcessVariableValue(Engine engine, object? variableValue)
|
||||
{
|
||||
if (variableValue == null)
|
||||
|
|
@ -31,10 +26,9 @@ internal static class ObjectConverterHelper
|
|||
|
||||
foreach (var kvp in expando)
|
||||
{
|
||||
var value = kvp.Value;
|
||||
var jsValue = ConvertToJsValue(engine, value);
|
||||
var propertyDescriptor = new PropertyDescriptor(jsValue, true, true, true);
|
||||
jsObject.DefineOwnProperty(kvp.Key, propertyDescriptor);
|
||||
// CreateDataProperty defines a writable, enumerable and configurable property, which is what the
|
||||
// explicit descriptor used to spell out, and takes the engine's fast path for doing so.
|
||||
jsObject.CreateDataProperty(kvp.Key, ConvertToJsValue(engine, kvp.Value));
|
||||
}
|
||||
|
||||
return jsObject;
|
||||
|
|
|
|||
|
|
@ -1,6 +1,4 @@
|
|||
using System.Diagnostics.CodeAnalysis;
|
||||
using System.Security.Cryptography;
|
||||
using System.Text;
|
||||
using Acornima.Ast;
|
||||
using Elsa.Expressions.Helpers;
|
||||
using Elsa.Expressions.Models;
|
||||
|
|
@ -25,6 +23,9 @@ namespace Elsa.Expressions.JavaScript.Services;
|
|||
public class JintJavaScriptEvaluator(IConfiguration configuration, INotificationSender mediator, IOptions<JintOptions> scriptOptions, IMemoryCache memoryCache)
|
||||
: IJavaScriptEvaluator
|
||||
{
|
||||
// The converters are stateless, so a single instance of each can serve every engine.
|
||||
private static readonly IObjectConverter[] ObjectConverters = [new ByteArrayConverter(), new EnumToStringConverter(), new JsonElementConverter()];
|
||||
|
||||
private readonly JintOptions _jintOptions = scriptOptions.Value;
|
||||
|
||||
/// <inheritdoc />
|
||||
|
|
@ -97,7 +98,7 @@ public class JintJavaScriptEvaluator(IConfiguration configuration, INotification
|
|||
|
||||
private void ConfigureObjectConverters(Jint.Options options)
|
||||
{
|
||||
options.Interop.ObjectConverters.AddRange([new ByteArrayConverter(), new EnumToStringConverter(), new JsonElementConverter()]);
|
||||
options.Interop.ObjectConverters.AddRange(ObjectConverters);
|
||||
}
|
||||
|
||||
private void ConfigureArgumentGetters(Engine engine, ExpressionEvaluatorOptions options)
|
||||
|
|
@ -124,15 +125,24 @@ public class JintJavaScriptEvaluator(IConfiguration configuration, INotification
|
|||
|
||||
private Prepared<Script> GetOrCreatePrepareScript(string expression)
|
||||
{
|
||||
var cacheKey = "jint:script:" + Hash(expression);
|
||||
// The key type keeps these entries distinct from any other consumer of the shared cache, so the
|
||||
// expression itself can be used as the key. A cache hit then costs a dictionary lookup rather than
|
||||
// a hash of the entire expression plus the allocations needed to render that hash as a string.
|
||||
var cacheKey = new ScriptCacheKey(expression);
|
||||
|
||||
return memoryCache.GetOrCreate(cacheKey, entry =>
|
||||
{
|
||||
if (_jintOptions.ScriptCacheTimeout.HasValue)
|
||||
entry.SetSlidingExpiration(_jintOptions.ScriptCacheTimeout.Value);
|
||||
// Looking the entry up directly rather than through GetOrCreate keeps the factory closure off the
|
||||
// hot path: it is only needed on a miss.
|
||||
if (memoryCache.TryGetValue(cacheKey, out Prepared<Script> cachedScript))
|
||||
return cachedScript;
|
||||
|
||||
return PrepareScript(expression);
|
||||
})!;
|
||||
using var entry = memoryCache.CreateEntry(cacheKey);
|
||||
|
||||
if (_jintOptions.ScriptCacheTimeout.HasValue)
|
||||
entry.SetSlidingExpiration(_jintOptions.ScriptCacheTimeout.Value);
|
||||
|
||||
var preparedScript = PrepareScript(expression);
|
||||
entry.Value = preparedScript;
|
||||
return preparedScript;
|
||||
}
|
||||
|
||||
private Prepared<Script> PrepareScript(string expression)
|
||||
|
|
@ -147,10 +157,8 @@ public class JintJavaScriptEvaluator(IConfiguration configuration, INotification
|
|||
return Engine.PrepareScript(expression, options: prepareOptions);
|
||||
}
|
||||
|
||||
private string Hash(string input)
|
||||
{
|
||||
var bytes = Encoding.UTF8.GetBytes(input);
|
||||
var hash = SHA256.HashData(bytes);
|
||||
return Convert.ToBase64String(hash);
|
||||
}
|
||||
/// <summary>
|
||||
/// Identifies a prepared script in the shared memory cache.
|
||||
/// </summary>
|
||||
private readonly record struct ScriptCacheKey(string Expression);
|
||||
}
|
||||
|
|
@ -0,0 +1,52 @@
|
|||
using Elsa.Expressions.JavaScript.Contracts;
|
||||
using Elsa.Expressions.Models;
|
||||
using Elsa.Testing.Shared;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Xunit;
|
||||
using Xunit.Abstractions;
|
||||
|
||||
namespace Elsa.JavaScript.IntegrationTests;
|
||||
|
||||
/// <summary>
|
||||
/// Verifies the behaviour of the prepared-script cache.
|
||||
/// </summary>
|
||||
public class ScriptCacheTests
|
||||
{
|
||||
private readonly IServiceProvider _serviceProvider;
|
||||
private readonly IJavaScriptEvaluator _evaluator;
|
||||
|
||||
public ScriptCacheTests(ITestOutputHelper testOutputHelper)
|
||||
{
|
||||
_serviceProvider = new TestApplicationBuilder(testOutputHelper).Build();
|
||||
_evaluator = _serviceProvider.GetRequiredService<IJavaScriptEvaluator>();
|
||||
}
|
||||
|
||||
[Fact(DisplayName = "Repeated evaluation of the same expression produces the same result")]
|
||||
public async Task RepeatedEvaluationIsStable()
|
||||
{
|
||||
Assert.Equal("3", await EvaluateAsync("return '' + (1 + 2);"));
|
||||
Assert.Equal("3", await EvaluateAsync("return '' + (1 + 2);"));
|
||||
Assert.Equal("7", await EvaluateAsync("return '' + (3 + 4);"));
|
||||
Assert.Equal("3", await EvaluateAsync("return '' + (1 + 2);"));
|
||||
}
|
||||
|
||||
[Fact(DisplayName = "An expression that fails to parse reports the failure on every evaluation")]
|
||||
public async Task ExpressionThatFailsToParseKeepsFailing()
|
||||
{
|
||||
var first = await Assert.ThrowsAnyAsync<Exception>(() => EvaluateAsync("return ("));
|
||||
var second = await Assert.ThrowsAnyAsync<Exception>(() => EvaluateAsync("return ("));
|
||||
|
||||
// Every evaluation has to report the same parse failure. Merely throwing twice would also be satisfied
|
||||
// by a cache that stored a null or half-built entry for the failed preparation, because the second
|
||||
// evaluation would then fail too — just with a different exception. Comparing the two exceptions rules
|
||||
// that out. The script is identical on both evaluations, so any position the message carries is too.
|
||||
Assert.IsType(first.GetType(), second);
|
||||
Assert.Equal(first.Message, second.Message);
|
||||
}
|
||||
|
||||
private async Task<string?> EvaluateAsync(string script)
|
||||
{
|
||||
var context = new ExpressionExecutionContext(_serviceProvider, new());
|
||||
return (string?)await _evaluator.EvaluateAsync(script, typeof(string), context);
|
||||
}
|
||||
}
|
||||
Loading…
Reference in a new issue