From fbca6331a75eb95f9bbe1b3a94ffba02ecb776f8 Mon Sep 17 00:00:00 2001 From: Benjamin Pannell Date: Mon, 6 Dec 2021 22:49:07 +0000 Subject: [PATCH 1/7] feat: Implement an optimized path for member access expressions within a LINQ-to-SQL expression This optimization handles the common case of accessing a member (variable, field, property) in the ambient scope, as shown below: ``` var bob = "Bob"; users.Where(u => u.Username == bob); ``` In this scenario, we will attempt to avoid compiling a lambda expression to evaluate `bob` and instead rely on reflection member access to retrieve the constant value. This helps us avoid the global lock held by `System.Reflection.Emit.DynamicMethod` while constructing the dynamic method table, which alleviates a serious performance bottleneck on highly threaded systems. In particular, this helps avoid a pathological async-over-sync situation which can lead to thread pool exhaustion and hot-lock thrashing. --- .../src/Linq/SubtreeEvaluator.cs | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs index 5a2df86760..188feee06b 100644 --- a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs +++ b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs @@ -6,12 +6,15 @@ namespace Microsoft.Azure.Cosmos.Linq using System; using System.Collections.Generic; using System.Linq.Expressions; + using System.Reflection; /// /// Evaluates and replaces sub-trees when first candidate is reached (top-down) /// internal sealed class SubtreeEvaluator : ExpressionVisitor { + private static readonly Dictionary<(Type Type, string Member), MemberInfo> memberInfoCache = new Dictionary<(Type Type, string Member), MemberInfo>(); + private HashSet candidates; public SubtreeEvaluator(HashSet candidates) @@ -44,12 +47,39 @@ protected override Expression VisitMemberInit(MemberInitExpression node) private Expression EvaluateConstant(Expression expression) { + if (expression.CanReduce) + { + expression = expression.Reduce(); + } + if (expression.NodeType == ExpressionType.Constant) { return expression; } + + // This is an optimization which attempts to avoid the compilation of a delegate lambda for + // conversion of an expression to a constant by handling member access on a constant through + // reflection instead. + // + // This is done because the compilation of a delegate takes a global lock which causes highly + // threaded clients to exhibit async-over-sync thread exhaustion behaviour on this call path + // even when doing relatively straightforward queries. + if (expression is MemberExpression memberExpression && memberExpression.Expression is ConstantExpression targetConstant) + { + if (memberExpression.Member is FieldInfo fieldInfo) + { + return Expression.Constant(fieldInfo.GetValue(targetConstant.Value)); + } + + if (memberExpression.Member is PropertyInfo propertyInfo) + { + return Expression.Constant(propertyInfo.GetValue(targetConstant.Value)); + } + } + LambdaExpression lambda = Expression.Lambda(expression); Delegate function = lambda.Compile(); + return Expression.Constant(function.DynamicInvoke(null), expression.Type); } } From ec1284fca3155e458b680ab8d2b6866ff17096a7 Mon Sep 17 00:00:00 2001 From: Benjamin Pannell <1760260+notheotherben@users.noreply.github.com> Date: Mon, 6 Dec 2021 23:11:13 +0000 Subject: [PATCH 2/7] style: Remove unused cache --- Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs index 188feee06b..dec7a78667 100644 --- a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs +++ b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs @@ -13,8 +13,6 @@ namespace Microsoft.Azure.Cosmos.Linq /// internal sealed class SubtreeEvaluator : ExpressionVisitor { - private static readonly Dictionary<(Type Type, string Member), MemberInfo> memberInfoCache = new Dictionary<(Type Type, string Member), MemberInfo>(); - private HashSet candidates; public SubtreeEvaluator(HashSet candidates) From 93a8b90d39b78f69c34aa5c766cfdac2381310ac Mon Sep 17 00:00:00 2001 From: Benjamin Pannell <1760260+notheotherben@users.noreply.github.com> Date: Mon, 6 Dec 2021 23:12:04 +0000 Subject: [PATCH 3/7] tweak: Fully reduce expression before evaluation --- Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs index dec7a78667..93af8a91cc 100644 --- a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs +++ b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs @@ -45,7 +45,7 @@ protected override Expression VisitMemberInit(MemberInitExpression node) private Expression EvaluateConstant(Expression expression) { - if (expression.CanReduce) + while (expression.CanReduce) { expression = expression.Reduce(); } From 9ea5f1e7d105f2644bb4173e88ac70a47133b8a4 Mon Sep 17 00:00:00 2001 From: Benjamin Pannell Date: Tue, 7 Dec 2021 10:31:48 +0000 Subject: [PATCH 4/7] test: Add some performance benchmarks to highlight the difference in reflection property access perf --- .../Linq/LinqToSqlBenchmark.cs | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs new file mode 100644 index 0000000000..2954738984 --- /dev/null +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs @@ -0,0 +1,41 @@ +namespace Microsoft.Azure.Cosmos.Linq +{ + using System; + using System.Collections.Generic; + using System.Linq; + using System.Linq.Expressions; + using System.Text; + using BenchmarkDotNet.Attributes; + + public class LinqToSqlBenchmark + { + private class BenchmarkDocument + { + public string Property { get; set; } + } + + [Benchmark(Baseline = true)] + public void DelegatePropertyAccess() + { + var holder = new + { + Property = "test" + }; + + this.DoTranslate(doc => doc.Property == holder.Property); + } + + [Benchmark] + public void ReflectionPropertyAccess() + { + string variable = "test"; + + this.DoTranslate(doc => doc.Property == variable); + } + + private void DoTranslate(Expression> expression) + { + SqlTranslator.TranslateExpression(expression.Body); + } + } +} From 13426736c99d6abf3a0ed26fc5a77acdf557a667 Mon Sep 17 00:00:00 2001 From: Benjamin Pannell Date: Tue, 7 Dec 2021 12:31:29 +0000 Subject: [PATCH 5/7] feat: Expand reflection property access to nested properties --- .../src/Linq/SubtreeEvaluator.cs | 50 +++++++++++++------ .../Linq/LinqToSqlBenchmark.cs | 13 +++-- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs index 93af8a91cc..c83e45ca83 100644 --- a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs +++ b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs @@ -43,18 +43,13 @@ protected override Expression VisitMemberInit(MemberInitExpression node) return node; } - private Expression EvaluateConstant(Expression expression) + private Expression EvaluateMemberAccess(Expression expression) { while (expression.CanReduce) { expression = expression.Reduce(); } - if (expression.NodeType == ExpressionType.Constant) - { - return expression; - } - // This is an optimization which attempts to avoid the compilation of a delegate lambda for // conversion of an expression to a constant by handling member access on a constant through // reflection instead. @@ -62,17 +57,40 @@ private Expression EvaluateConstant(Expression expression) // This is done because the compilation of a delegate takes a global lock which causes highly // threaded clients to exhibit async-over-sync thread exhaustion behaviour on this call path // even when doing relatively straightforward queries. - if (expression is MemberExpression memberExpression && memberExpression.Expression is ConstantExpression targetConstant) + if (!(expression is MemberExpression memberExpression)) + { + return expression; + } + + // We recursively attempt to evaluate member access expressions so that we can support + // nested property access (x.y.z) without needing to fall back on delegate compilation. + Expression targetExpression = this.EvaluateMemberAccess(memberExpression.Expression); + + if (!(targetExpression is ConstantExpression targetConstant)) + { + return expression; + } + + if (memberExpression.Member is FieldInfo fieldInfo) + { + return Expression.Constant(fieldInfo.GetValue(targetConstant.Value)); + } + + if (memberExpression.Member is PropertyInfo propertyInfo) + { + return Expression.Constant(propertyInfo.GetValue(targetConstant.Value)); + } + + return expression; + } + + private Expression EvaluateConstant(Expression expression) + { + expression = this.EvaluateMemberAccess(expression); + + if (expression.NodeType == ExpressionType.Constant) { - if (memberExpression.Member is FieldInfo fieldInfo) - { - return Expression.Constant(fieldInfo.GetValue(targetConstant.Value)); - } - - if (memberExpression.Member is PropertyInfo propertyInfo) - { - return Expression.Constant(propertyInfo.GetValue(targetConstant.Value)); - } + return expression; } LambdaExpression lambda = Expression.Lambda(expression); diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs index 2954738984..1ac5108e8a 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Linq/LinqToSqlBenchmark.cs @@ -1,10 +1,7 @@ namespace Microsoft.Azure.Cosmos.Linq { using System; - using System.Collections.Generic; - using System.Linq; using System.Linq.Expressions; - using System.Text; using BenchmarkDotNet.Attributes; public class LinqToSqlBenchmark @@ -16,6 +13,14 @@ private class BenchmarkDocument [Benchmark(Baseline = true)] public void DelegatePropertyAccess() + { + string variable = "test"; + + this.DoTranslate(doc => doc.Property == variable + variable); + } + + [Benchmark] + public void NestedPropertyAccess() { var holder = new { @@ -26,7 +31,7 @@ public void DelegatePropertyAccess() } [Benchmark] - public void ReflectionPropertyAccess() + public void VariableAccess() { string variable = "test"; From ff07c90fd53b61274516c340149a9999050b4438 Mon Sep 17 00:00:00 2001 From: Benjamin Pannell Date: Thu, 9 Dec 2021 00:17:36 +0000 Subject: [PATCH 6/7] fix: Retain type information when evaluating fields/properties in SubtreeEvaluator --- Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs index c83e45ca83..aaaf31d839 100644 --- a/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs +++ b/Microsoft.Azure.Cosmos/src/Linq/SubtreeEvaluator.cs @@ -73,12 +73,12 @@ private Expression EvaluateMemberAccess(Expression expression) if (memberExpression.Member is FieldInfo fieldInfo) { - return Expression.Constant(fieldInfo.GetValue(targetConstant.Value)); + return Expression.Constant(fieldInfo.GetValue(targetConstant.Value), memberExpression.Type); } if (memberExpression.Member is PropertyInfo propertyInfo) { - return Expression.Constant(propertyInfo.GetValue(targetConstant.Value)); + return Expression.Constant(propertyInfo.GetValue(targetConstant.Value), memberExpression.Type); } return expression; From 78ff617f9dd2af2922af3c90add97be067d70afa Mon Sep 17 00:00:00 2001 From: Benjamin Pannell Date: Thu, 28 Apr 2022 10:32:00 +0100 Subject: [PATCH 7/7] test: Add additional tests showcasing how the new reflection expression evaluator can be triggered or bypassed --- ...nslationBaselineTests.TestMemberAccess.xml | 38 +++++++++++++++++++ .../LinqTranslationBaselineTests.cs | 37 ++++++++++++++++++ ...icrosoft.Azure.Cosmos.EmulatorTests.csproj | 3 ++ .../Contracts/PerformanceValidation.cs | 2 +- 4 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BaselineTest/TestBaseline/LinqTranslationBaselineTests.TestMemberAccess.xml diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BaselineTest/TestBaseline/LinqTranslationBaselineTests.TestMemberAccess.xml b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BaselineTest/TestBaseline/LinqTranslationBaselineTests.TestMemberAccess.xml new file mode 100644 index 0000000000..9b5974fe37 --- /dev/null +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BaselineTest/TestBaseline/LinqTranslationBaselineTests.TestMemberAccess.xml @@ -0,0 +1,38 @@ + + + + + (doc.NumericField == DisplayClass.ambientContext.MethodAccess()))]]> + + + + + + + + + (doc.NumericField == DisplayClass.ambientContext.FieldAccess))]]> + + + + + + + + + (doc.NumericField == DisplayClass.ambientContext.PropertyAccess))]]> + + + + + + \ No newline at end of file diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/LinqTranslationBaselineTests.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/LinqTranslationBaselineTests.cs index cda2091c4f..a2eff4baa2 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/LinqTranslationBaselineTests.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/LinqTranslationBaselineTests.cs @@ -145,6 +145,15 @@ internal class DataObject : LinqTestObject public string Pk; } + internal class AmbientContextObject + { + public double FieldAccess; + + public double PropertyAccess { get; set; } + + public double MethodAccess() => 1.0; + } + [TestMethod] public void TestLiteralSerialization() { @@ -527,6 +536,34 @@ public void TestMathFunctions() this.ExecuteTestSuite(inputs); } + + [TestMethod] + public void TestMemberAccess() + { + AmbientContextObject ambientContext = new AmbientContextObject + { + FieldAccess = 2.0, + PropertyAccess = 3.0 + }; + + List testData = new List(); + IOrderedQueryable constantQuery = testContainer.GetItemLinqQueryable(allowSynchronousQueryExecution: true); + Func> getQuery = useQuery => useQuery ? constantQuery : testData.AsQueryable(); + + List inputs = new List + { + // This test case will use the legacy delegate compilation expression evaluator + new LinqTestInput("Filter on Method value", b => getQuery(b).Where(doc => doc.NumericField == ambientContext.MethodAccess())), + + // These test cases will use the new reflection-based member access expression evaluator + // to avoid the acquisition of a global lock when compiling the delegate (yielding a substantial + // performance boost, especially under highly concurrent workloads). + new LinqTestInput("Filter on Field value", b => getQuery(b).Where(doc => doc.NumericField == ambientContext.FieldAccess)), + new LinqTestInput("Filter on Property value", b => getQuery(b).Where(doc => doc.NumericField == ambientContext.PropertyAccess)), + }; + this.ExecuteTestSuite(inputs); + } + private Func> CreateDataTestStringFunctions() { const int Records = 100; diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Microsoft.Azure.Cosmos.EmulatorTests.csproj b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Microsoft.Azure.Cosmos.EmulatorTests.csproj index ca54df2dc7..586de09004 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Microsoft.Azure.Cosmos.EmulatorTests.csproj +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/Microsoft.Azure.Cosmos.EmulatorTests.csproj @@ -231,6 +231,9 @@ PreserveNewest + + PreserveNewest + PreserveNewest diff --git a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Contracts/PerformanceValidation.cs b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Contracts/PerformanceValidation.cs index fd90bf6267..e32cb7f296 100644 --- a/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Contracts/PerformanceValidation.cs +++ b/Microsoft.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.Performance.Tests/Contracts/PerformanceValidation.cs @@ -43,7 +43,7 @@ public static bool TryUpdateAllocatedMemoryAverage(IEnumerable summarie foreach (BenchmarkReport report in summary.Reports) { double allocatedMemory = report.Metrics["Allocated Memory"].Value; - string operationName = report.BenchmarkCase.Descriptor.ToString() + ";" + string.Join(';', report.BenchmarkCase.Parameters.ValueInfo); + string operationName = report.BenchmarkCase.Descriptor.ToString() + ";" + string.Join(";", report.BenchmarkCase.Parameters.ValueInfo); // Average if the operation name already is in the dictionary if(operationToMemoryAllocated.TryGetValue(operationName, out double value))