From c43b440b9b6bf451fda13c0cec30ef5ba5f83a98 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Sun, 21 Jan 2018 16:51:23 +0100 Subject: [PATCH 1/2] Avoid re-translating NhLinqExpression. NhLinqExpression instances may be translated multiple times in two cases: * When they have list parameters * When their plan is not cacheable, since it is looked-up two times in cache per execution, and (re)generated in case of cache miss. --- .../Async/NHSpecificTest/GH1547/Fixture.cs | 193 ++++++++++ .../NHSpecificTest/GH1547/Entity.cs | 10 + .../NHSpecificTest/GH1547/Fixture.cs | 344 ++++++++++++++++++ .../NHSpecificTest/GH1547/Mappings.hbm.xml | 9 + src/NHibernate/Impl/ExpressionQueryImpl.cs | 6 +- src/NHibernate/Linq/NhLinqExpression.cs | 21 +- 6 files changed, 581 insertions(+), 2 deletions(-) create mode 100644 src/NHibernate.Test/Async/NHSpecificTest/GH1547/Fixture.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1547/Entity.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1547/Fixture.cs create mode 100644 src/NHibernate.Test/NHSpecificTest/GH1547/Mappings.hbm.xml diff --git a/src/NHibernate.Test/Async/NHSpecificTest/GH1547/Fixture.cs b/src/NHibernate.Test/Async/NHSpecificTest/GH1547/Fixture.cs new file mode 100644 index 00000000000..8ff5cd8b37e --- /dev/null +++ b/src/NHibernate.Test/Async/NHSpecificTest/GH1547/Fixture.cs @@ -0,0 +1,193 @@ +//------------------------------------------------------------------------------ +// +// This code was generated by AsyncGenerator. +// +// Changes to this file may cause incorrect behavior and will be lost if +// the code is regenerated. +// +//------------------------------------------------------------------------------ + + +using System; +using System.Collections.Generic; +using System.Data; +using System.Data.Common; +using System.Diagnostics; +using System.Linq; +using System.Reflection; +using NHibernate.Cfg; +using NHibernate.Driver; +using NHibernate.Engine; +using NHibernate.Engine.Query; +using NHibernate.SqlCommand; +using NHibernate.SqlTypes; +using NHibernate.Util; +using NSubstitute; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1547 +{ + using System.Threading.Tasks; + using System.Threading; + [TestFixture, Explicit("Contains only performances benchmark")] + public class FixtureAsync : BugTestCase + { + protected override void Configure(Configuration configuration) + { + base.Configure(configuration); + + var driverClass = ReflectHelper.ClassForName(configuration.GetProperty(Cfg.Environment.ConnectionDriver)); + DriverForSubstitutedCommand.DriverClass = driverClass; + + configuration.SetProperty( + Cfg.Environment.ConnectionDriver, + typeof(DriverForSubstitutedCommand).AssemblyQualifiedName); + } + + protected override void DropSchema() + { + (Sfi.ConnectionProvider.Driver as DriverForSubstitutedCommand)?.CleanUp(); + base.DropSchema(); + } + + [Test] + public Task SimpleLinqPerfAsync() + { + return BenchmarkAsync( + "Simple LINQ", + s => + s + .Query() + .Where(e => e.Name == "Bob")); + } + + [Test] + public Task LinqWithNonParameterizedConstantPerfAsync() + { + return BenchmarkAsync( + "Non parameterized constant", + s => + s + .Query() + .Where(e => e.Name == "Bob") + .Select(e => new { e, c = 2 })); + } + + [Test] + public Task LinqWithListParameterPerfAsync() + { + try + { + var names = new[] { "Bob", "Sally" }; + return BenchmarkAsync( + "List parameter", + s => + s + .Query() + .Where(e => names.Contains(e.Name))); + } + catch (Exception ex) + { + return Task.FromException(ex); + } + } + + private async Task BenchmarkAsync(string test, Func> queryFactory, CancellationToken cancellationToken = default(CancellationToken)) + { + var driver = (DriverForSubstitutedCommand) Sfi.ConnectionProvider.Driver; + var timings = new List(); + var sw = new Stopwatch(); + + var cache = (SoftLimitMRUCache) + typeof(QueryPlanCache) + .GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic) + .GetValue(Sfi.QueryPlanCache); + + using (var session = OpenSession()) + using (var tx = session.BeginTransaction()) + { + using (driver.SubstituteCommand()) + { + var query = queryFactory(session); + // Warm up. + await (RunBenchmarkUnitAsync(cache, query)); + + for (var j = 0; j < 1000; j++) + { + sw.Restart(); + await (RunBenchmarkUnitAsync(cache, query)); + sw.Stop(); + timings.Add(sw.Elapsed.TotalMilliseconds); + } + } + + await (tx.CommitAsync(cancellationToken)); + } + + var avg = timings.Average(); + Console.WriteLine( + $"{test} average time: {avg}ms (s {Math.Sqrt(timings.Sum(t => Math.Pow(t - avg, 2)) / (timings.Count - 1))}ms)"); + } + + private static Task RunBenchmarkUnitAsync(SoftLimitMRUCache cache, IQueryable query) + { + try + { + // Do enough iterations for having a significant elapsed time in milliseconds. + for (var i = 0; i < 20; i++) + { + // Always clear the query plan cache before running the query, otherwise the impact of 1547 + // change would be hidden by it. Simulates having many different queries run. + cache.Clear(); + Assert.That(query.ToList, Throws.Nothing); + } + return Task.CompletedTask; + } + catch (Exception ex) + { + return Task.FromException(ex); + } + } + } + + public partial class DriverForSubstitutedCommand : IDriver + { + + #region Firebird mess + + #endregion + #region Pure forwarding + + #endregion + + private partial class SubstituteDbCommand : DbCommand + { + + protected override Task ExecuteDbDataReaderAsync(CommandBehavior behavior, CancellationToken cancellationToken) + { + try + { + return Task.FromResult(_substituteReader); + } + catch (Exception ex) + { + return Task.FromException(ex); + } + } + + public override Task ExecuteNonQueryAsync(CancellationToken cancellationToken) + { + return Task.FromResult(0); + } + + public override Task ExecuteScalarAsync(CancellationToken cancellationToken) + { + return Task.FromResult(null); + } + + #region Pure forwarding + + #endregion + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1547/Entity.cs b/src/NHibernate.Test/NHSpecificTest/GH1547/Entity.cs new file mode 100644 index 00000000000..32477a1206d --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1547/Entity.cs @@ -0,0 +1,10 @@ +using System; + +namespace NHibernate.Test.NHSpecificTest.GH1547 +{ + class Entity + { + public virtual Guid Id { get; set; } + public virtual string Name { get; set; } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1547/Fixture.cs b/src/NHibernate.Test/NHSpecificTest/GH1547/Fixture.cs new file mode 100644 index 00000000000..5dc6705647b --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1547/Fixture.cs @@ -0,0 +1,344 @@ +using System; +using System.Collections.Generic; +using System.Data; +using System.Data.Common; +using System.Diagnostics; +using System.Linq; +using System.Reflection; +using NHibernate.Cfg; +using NHibernate.Driver; +using NHibernate.Engine; +using NHibernate.Engine.Query; +using NHibernate.SqlCommand; +using NHibernate.SqlTypes; +using NHibernate.Util; +using NSubstitute; +using NUnit.Framework; + +namespace NHibernate.Test.NHSpecificTest.GH1547 +{ + [TestFixture, Explicit("Contains only performances benchmark")] + public class Fixture : BugTestCase + { + protected override void Configure(Configuration configuration) + { + base.Configure(configuration); + + var driverClass = ReflectHelper.ClassForName(configuration.GetProperty(Cfg.Environment.ConnectionDriver)); + DriverForSubstitutedCommand.DriverClass = driverClass; + + configuration.SetProperty( + Cfg.Environment.ConnectionDriver, + typeof(DriverForSubstitutedCommand).AssemblyQualifiedName); + } + + protected override void DropSchema() + { + (Sfi.ConnectionProvider.Driver as DriverForSubstitutedCommand)?.CleanUp(); + base.DropSchema(); + } + + [Test] + public void SimpleLinqPerf() + { + Benchmark( + "Simple LINQ", + s => + s + .Query() + .Where(e => e.Name == "Bob")); + } + + [Test] + public void LinqWithNonParameterizedConstantPerf() + { + Benchmark( + "Non parameterized constant", + s => + s + .Query() + .Where(e => e.Name == "Bob") + .Select(e => new { e, c = 2 })); + } + + [Test] + public void LinqWithListParameterPerf() + { + var names = new[] { "Bob", "Sally" }; + Benchmark( + "List parameter", + s => + s + .Query() + .Where(e => names.Contains(e.Name))); + } + + private void Benchmark(string test, Func> queryFactory) + { + var driver = (DriverForSubstitutedCommand) Sfi.ConnectionProvider.Driver; + var timings = new List(); + var sw = new Stopwatch(); + + var cache = (SoftLimitMRUCache) + typeof(QueryPlanCache) + .GetField("planCache", BindingFlags.Instance | BindingFlags.NonPublic) + .GetValue(Sfi.QueryPlanCache); + + using (var session = OpenSession()) + using (var tx = session.BeginTransaction()) + { + using (driver.SubstituteCommand()) + { + var query = queryFactory(session); + // Warm up. + RunBenchmarkUnit(cache, query); + + for (var j = 0; j < 1000; j++) + { + sw.Restart(); + RunBenchmarkUnit(cache, query); + sw.Stop(); + timings.Add(sw.Elapsed.TotalMilliseconds); + } + } + + tx.Commit(); + } + + var avg = timings.Average(); + Console.WriteLine( + $"{test} average time: {avg}ms (s {Math.Sqrt(timings.Sum(t => Math.Pow(t - avg, 2)) / (timings.Count - 1))}ms)"); + } + + private static void RunBenchmarkUnit(SoftLimitMRUCache cache, IQueryable query) + { + // Do enough iterations for having a significant elapsed time in milliseconds. + for (var i = 0; i < 20; i++) + { + // Always clear the query plan cache before running the query, otherwise the impact of 1547 + // change would be hidden by it. Simulates having many different queries run. + cache.Clear(); + Assert.That(query.ToList, Throws.Nothing); + } + } + } + + public partial class DriverForSubstitutedCommand : IDriver + { + internal static System.Type DriverClass { get; set; } + + private readonly IDriver _driverImplementation; + private bool _commandSubstituted; + + public DriverForSubstitutedCommand() + { + _driverImplementation = (IDriver) Cfg.Environment.BytecodeProvider.ObjectsFactory.CreateInstance(DriverClass); + } + + DbCommand IDriver.GenerateCommand(CommandType type, SqlString sqlString, SqlType[] parameterTypes) + { + var cmd = _driverImplementation.GenerateCommand(type, sqlString, parameterTypes); + if (!_commandSubstituted) + return cmd; + return new SubstituteDbCommand(cmd); + } + + public IDisposable SubstituteCommand() + { + _commandSubstituted = true; + return new EndSubstitute(this); + } + + #region Firebird mess + + public void CleanUp() + { + if (_driverImplementation is FirebirdClientDriver fbDriver) + { + // Firebird will pool each connection created during the test and will marked as used any table + // referenced by queries. It will at best delays those tables drop until connections are actually + // closed, or immediately fail dropping them. + // This results in other tests failing when they try to create tables with same name. + // By clearing the connection pool the tables will get dropped. This is done by the following code. + // Moved from NH1908 test case, contributed by Amro El-Fakharany. + fbDriver.ClearPool(null); + } + } + + #endregion + + #region Pure forwarding + + DbParameter IDriver.GenerateParameter(DbCommand command, string name, SqlType sqlType) + { + return _driverImplementation.GenerateParameter(command, name, sqlType); + } + + void IDriver.Configure(IDictionary settings) + { + _driverImplementation.Configure(settings); + } + + DbConnection IDriver.CreateConnection() + { + return _driverImplementation.CreateConnection(); + } + + bool IDriver.SupportsMultipleOpenReaders => _driverImplementation.SupportsMultipleOpenReaders; + + void IDriver.PrepareCommand(DbCommand command) + { + _driverImplementation.PrepareCommand(command); + } + + void IDriver.RemoveUnusedCommandParameters(DbCommand cmd, SqlString sqlString) + { + _driverImplementation.RemoveUnusedCommandParameters(cmd, sqlString); + } + + void IDriver.ExpandQueryParameters(DbCommand cmd, SqlString sqlString, SqlType[] parameterTypes) + { + _driverImplementation.ExpandQueryParameters(cmd, sqlString, parameterTypes); + } + + IResultSetsCommand IDriver.GetResultSetsCommand(ISessionImplementor session) + { + return _driverImplementation.GetResultSetsCommand(session); + } + + bool IDriver.SupportsMultipleQueries => _driverImplementation.SupportsMultipleQueries; + + void IDriver.AdjustCommand(DbCommand command) + { + _driverImplementation.AdjustCommand(command); + } + + bool IDriver.RequiresTimeSpanForTime => _driverImplementation.RequiresTimeSpanForTime; + + bool IDriver.SupportsSystemTransactions => _driverImplementation.SupportsSystemTransactions; + + bool IDriver.SupportsNullEnlistment => _driverImplementation.SupportsNullEnlistment; + + bool IDriver.SupportsEnlistmentWhenAutoEnlistmentIsDisabled => + _driverImplementation.SupportsEnlistmentWhenAutoEnlistmentIsDisabled; + + bool IDriver.HasDelayedDistributedTransactionCompletion => + _driverImplementation.HasDelayedDistributedTransactionCompletion; + + DateTime IDriver.MinDate => _driverImplementation.MinDate; + + #endregion + + private class EndSubstitute : IDisposable + { + private readonly DriverForSubstitutedCommand _driver; + + public EndSubstitute(DriverForSubstitutedCommand driver) + { + _driver = driver; + } + + public void Dispose() + { + _driver._commandSubstituted = false; + } + } + + private partial class SubstituteDbCommand : DbCommand + { + private static readonly DbDataReader _substituteReader = Substitute.For(); + private readonly DbCommand _concreteCommand; + + public SubstituteDbCommand(DbCommand concreteCommand) + { + _concreteCommand = concreteCommand; + } + + protected override DbDataReader ExecuteDbDataReader(CommandBehavior behavior) + { + return _substituteReader; + } + + public override void Prepare() + { + } + + public override int ExecuteNonQuery() + { + return 0; + } + + public override object ExecuteScalar() + { + return null; + } + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + if (disposing) + { + _concreteCommand.Dispose(); + } + } + + #region Pure forwarding + + public override string CommandText + { + get => _concreteCommand.CommandText; + set => _concreteCommand.CommandText = value; + } + + public override int CommandTimeout + { + get => _concreteCommand.CommandTimeout; + set => _concreteCommand.CommandTimeout = value; + } + + public override CommandType CommandType + { + get => _concreteCommand.CommandType; + set => _concreteCommand.CommandType = value; + } + + public override UpdateRowSource UpdatedRowSource + { + get => _concreteCommand.UpdatedRowSource; + set => _concreteCommand.UpdatedRowSource = value; + } + + protected override DbConnection DbConnection + { + get => _concreteCommand.Connection; + set => _concreteCommand.Connection = value; + } + + protected override DbParameterCollection DbParameterCollection => _concreteCommand.Parameters; + + protected override DbTransaction DbTransaction + { + get => _concreteCommand.Transaction; + set => _concreteCommand.Transaction = value; + } + + public override bool DesignTimeVisible + { + get => _concreteCommand.DesignTimeVisible; + set => _concreteCommand.DesignTimeVisible = value; + } + + public override void Cancel() + { + _concreteCommand.Cancel(); + } + + protected override DbParameter CreateDbParameter() + { + return _concreteCommand.CreateParameter(); + } + + #endregion + } + } +} diff --git a/src/NHibernate.Test/NHSpecificTest/GH1547/Mappings.hbm.xml b/src/NHibernate.Test/NHSpecificTest/GH1547/Mappings.hbm.xml new file mode 100644 index 00000000000..1af20ef529a --- /dev/null +++ b/src/NHibernate.Test/NHSpecificTest/GH1547/Mappings.hbm.xml @@ -0,0 +1,9 @@ + + + + + + + + + diff --git a/src/NHibernate/Impl/ExpressionQueryImpl.cs b/src/NHibernate/Impl/ExpressionQueryImpl.cs index c4cce671ef4..0d73f596f4b 100644 --- a/src/NHibernate/Impl/ExpressionQueryImpl.cs +++ b/src/NHibernate/Impl/ExpressionQueryImpl.cs @@ -74,11 +74,15 @@ protected override IQueryExpression ExpandParameters(IDictionary(); var queryModel = NhRelinqQueryParser.Parse(_expression); var visitorParameters = new VisitorParameters(sessionFactory, _constantToParameterMap, requiredHqlParameters, @@ -88,7 +96,8 @@ public IASTNode Translate(ISessionFactoryImplementor sessionFactory, bool filter .Except(requiredHqlParameters.Select(p => p.Name)) .Any(); - return ExpressionToHqlTranslationResults.Statement.AstNode; + // The ast node may be altered by caller, duplicate it for preserving the original one. + return DuplicateTree(ExpressionToHqlTranslationResults.Statement.AstNode); } internal void CopyExpressionTranslation(NhLinqExpression other) @@ -98,5 +107,15 @@ internal void CopyExpressionTranslation(NhLinqExpression other) // Type could have been overridden by translation. Type = other.Type; } + + private static IASTNode DuplicateTree(IASTNode ast) + { + var thisNode = ast.DupNode(); + foreach (var child in ast) + { + thisNode.AddChild(DuplicateTree(child)); + } + return thisNode; + } } } From 21dbe58474561e926c3c367be90d558e3ebb0e6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fr=C3=A9d=C3=A9ric=20Delaporte?= Date: Sun, 21 Jan 2018 16:58:31 +0100 Subject: [PATCH 2/2] Fix a wrong parameter value for query translation It was not having any impact because concrete used implementations are currently not using its value. --- .../Async/Impl/ExpressionQueryImpl.cs | 3 +-- src/NHibernate/Impl/ExpressionQueryImpl.cs | 20 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/NHibernate/Async/Impl/ExpressionQueryImpl.cs b/src/NHibernate/Async/Impl/ExpressionQueryImpl.cs index eb721ab3c05..a0eb03f8f15 100644 --- a/src/NHibernate/Async/Impl/ExpressionQueryImpl.cs +++ b/src/NHibernate/Async/Impl/ExpressionQueryImpl.cs @@ -8,7 +8,6 @@ //------------------------------------------------------------------------------ -using System; using System.Collections; using System.Collections.Generic; using System.Linq; @@ -26,7 +25,7 @@ namespace NHibernate.Impl using System.Threading.Tasks; using System.Threading; - partial class ExpressionFilterImpl : ExpressionQueryImpl + internal partial class ExpressionFilterImpl : ExpressionQueryImpl { public override async Task ListAsync(CancellationToken cancellationToken = default(CancellationToken)) diff --git a/src/NHibernate/Impl/ExpressionQueryImpl.cs b/src/NHibernate/Impl/ExpressionQueryImpl.cs index 0d73f596f4b..b27380eb7f5 100644 --- a/src/NHibernate/Impl/ExpressionQueryImpl.cs +++ b/src/NHibernate/Impl/ExpressionQueryImpl.cs @@ -1,4 +1,3 @@ -using System; using System.Collections; using System.Collections.Generic; using System.Linq; @@ -21,8 +20,17 @@ public ExpressionQueryImpl(IQueryExpression queryExpression, ISessionImplementor QueryExpression = queryExpression; } + protected ExpressionQueryImpl( + IQueryExpression queryExpression, ISessionImplementor session, ParameterMetadata parameterMetadata, bool isFilter) + : this(queryExpression, session, parameterMetadata) + { + _isFilter = isFilter; + } + public IQueryExpression QueryExpression { get; private set; } + protected readonly bool _isFilter; + /// /// Warning: adds new parameters to the argument by side-effect, as well as mutating the query expression tree! /// @@ -75,10 +83,8 @@ protected override IQueryExpression ExpandParameters(IDictionary