From d75b8bbfebbf147e75a99d5c72e89f9d53386e42 Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Thu, 24 Mar 2022 12:39:51 +0200 Subject: [PATCH] Reimplement MaxBatchSize as a pre-check For better perf on SQLite (#27681) Also moves the handling of MaxBatchSize to ReaderModificationCommandBatch and does some cleanup. --- .../Update/ModificationCommandBatch.cs | 4 +-- .../Update/ReaderModificationCommandBatch.cs | 30 ++++++++++++++----- .../SingularModificationCommandBatch.cs | 9 ++---- .../SqlServerModificationCommandBatch.cs | 24 +++++++-------- ...qlServerModificationCommandBatchFactory.cs | 21 ++++++++----- .../TestModificationCommandBatch.cs | 9 ++---- .../ReaderModificationCommandBatchTest.cs | 3 ++ .../SqlServerModificationCommandBatchTest.cs | 4 +-- 8 files changed, 59 insertions(+), 45 deletions(-) diff --git a/src/EFCore.Relational/Update/ModificationCommandBatch.cs b/src/EFCore.Relational/Update/ModificationCommandBatch.cs index 3d97110eb52..b7b01752428 100644 --- a/src/EFCore.Relational/Update/ModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/ModificationCommandBatch.cs @@ -56,7 +56,5 @@ public abstract class ModificationCommandBatch /// A to observe while waiting for the task to complete. /// A task that represents the asynchronous save operation. /// If the is canceled. - public abstract Task ExecuteAsync( - IRelationalConnection connection, - CancellationToken cancellationToken = default); + public abstract Task ExecuteAsync(IRelationalConnection connection, CancellationToken cancellationToken = default); } diff --git a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs index 641d886d0ee..f49c52ccf9d 100644 --- a/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/ReaderModificationCommandBatch.cs @@ -23,9 +23,9 @@ public abstract class ReaderModificationCommandBatch : ModificationCommandBatch { private readonly List _modificationCommands = new(); private readonly int _batchHeaderLength; - private readonly List _pendingParameterNames = new(); private bool _requiresTransaction = true; private int _sqlBuilderPosition, _commandResultSetCount, _resultsPositionalMappingEnabledLength; + private int _pendingParameters; /// /// Creates a new instance. @@ -57,6 +57,12 @@ protected ReaderModificationCommandBatch(ModificationCommandBatchFactoryDependen /// protected virtual IRelationalCommandBuilder RelationalCommandBuilder { get; } + /// + /// The maximum number of instances that can be added to a single batch. + /// + protected virtual int MaxBatchSize + => 1000; + /// /// Gets the command text builder for the commands in the batch. /// @@ -98,9 +104,14 @@ public override bool TryAddCommand(IReadOnlyModificationCommand modificationComm throw new InvalidOperationException(RelationalStrings.ModificationCommandBatchAlreadyComplete); } + if (_modificationCommands.Count >= MaxBatchSize) + { + return false; + } + _sqlBuilderPosition = SqlBuilder.Length; _commandResultSetCount = CommandResultSet.Count; - _pendingParameterNames.Clear(); + _pendingParameters = 0; _resultsPositionalMappingEnabledLength = ResultsPositionalMappingEnabled?.Length ?? 0; AddCommand(modificationCommand); @@ -144,11 +155,13 @@ protected virtual void RollbackLastCommand() ResultsPositionalMappingEnabled.Length = _resultsPositionalMappingEnabledLength; } - foreach (var pendingParameterName in _pendingParameterNames) + for (var i = 0; i < _pendingParameters; i++) { - ParameterValues.Remove(pendingParameterName); + var parameterIndex = RelationalCommandBuilder.Parameters.Count - 1; + var parameter = RelationalCommandBuilder.Parameters[parameterIndex]; - RelationalCommandBuilder.RemoveParameterAt(RelationalCommandBuilder.Parameters.Count - 1); + RelationalCommandBuilder.RemoveParameterAt(parameterIndex); + ParameterValues.Remove(parameter.InvariantName); } } @@ -173,7 +186,8 @@ protected virtual void SetRequiresTransaction(bool requiresTransaction) /// Checks whether the command text is valid. /// /// if the command text is valid; otherwise. - protected abstract bool IsValid(); + protected virtual bool IsValid() + => true; /// /// Adds Updates the command text for the command at the given position in the list. @@ -265,7 +279,7 @@ protected virtual void AddParameter(IColumnModification columnModification) ParameterValues.Add(columnModification.ParameterName, columnModification.Value); - _pendingParameterNames.Add(columnModification.ParameterName); + _pendingParameters++; } if (columnModification.UseOriginalValueParameter) @@ -278,7 +292,7 @@ protected virtual void AddParameter(IColumnModification columnModification) ParameterValues.Add(columnModification.OriginalParameterName, columnModification.OriginalValue); - _pendingParameterNames.Add(columnModification.OriginalParameterName); + _pendingParameters++; } } diff --git a/src/EFCore.Relational/Update/SingularModificationCommandBatch.cs b/src/EFCore.Relational/Update/SingularModificationCommandBatch.cs index e9763f2ab94..e63dd91c95f 100644 --- a/src/EFCore.Relational/Update/SingularModificationCommandBatch.cs +++ b/src/EFCore.Relational/Update/SingularModificationCommandBatch.cs @@ -28,11 +28,8 @@ public SingularModificationCommandBatch(ModificationCommandBatchFactoryDependenc } /// - /// Returns only when the batch contains a single command. + /// The maximum number of instances that can be added to a single batch; always returns 1. /// - /// - /// when the batch contains a single command, otherwise. - /// - protected override bool IsValid() - => ModificationCommands.Count == 1; + protected override int MaxBatchSize + => 1; } diff --git a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs index 336e318c894..c6d8a9085e4 100644 --- a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs +++ b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatch.cs @@ -18,7 +18,6 @@ public class SqlServerModificationCommandBatch : AffectedCountModificationComman private const int DefaultNetworkPacketSizeBytes = 4096; private const int MaxScriptLength = 65536 * DefaultNetworkPacketSizeBytes / 2; private const int MaxParameterCount = 2100; - private const int MaxRowCount = 1000; private readonly int _maxBatchSize; private readonly List _pendingBulkInsertCommands = new(); @@ -30,16 +29,9 @@ public class SqlServerModificationCommandBatch : AffectedCountModificationComman /// public SqlServerModificationCommandBatch( ModificationCommandBatchFactoryDependencies dependencies, - int? maxBatchSize) + int maxBatchSize) : base(dependencies) - { - if (maxBatchSize is <= 0) - { - throw new ArgumentOutOfRangeException(nameof(maxBatchSize), RelationalStrings.InvalidMaxBatchSize(maxBatchSize.Value)); - } - - _maxBatchSize = Math.Min(maxBatchSize ?? 42, MaxRowCount); - } + => _maxBatchSize = maxBatchSize; /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -50,6 +42,15 @@ public SqlServerModificationCommandBatch( protected new virtual ISqlServerUpdateSqlGenerator UpdateSqlGenerator => (ISqlServerUpdateSqlGenerator)base.UpdateSqlGenerator; + /// + /// The maximum number of instances that can be added to a single batch. + /// + /// + /// For SQL Server, this is 42 by default, and cannot exceed 1000. + /// + protected override int MaxBatchSize + => _maxBatchSize; + /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to /// the same compatibility standards as public APIs. It may be changed or removed without notice in @@ -74,8 +75,7 @@ protected override void RollbackLastCommand() /// doing so can result in application failures when updating to a new Entity Framework Core release. /// protected override bool IsValid() - => ModificationCommands.Count <= _maxBatchSize - && SqlBuilder.Length < MaxScriptLength + => SqlBuilder.Length < MaxScriptLength // A single implicit parameter for the command text itself && ParameterValues.Count + 1 < MaxParameterCount; diff --git a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatchFactory.cs b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatchFactory.cs index bbb1aad0212..bf2f09f8210 100644 --- a/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatchFactory.cs +++ b/src/EFCore.SqlServer/Update/Internal/SqlServerModificationCommandBatchFactory.cs @@ -13,7 +13,9 @@ namespace Microsoft.EntityFrameworkCore.SqlServer.Update.Internal; /// public class SqlServerModificationCommandBatchFactory : IModificationCommandBatchFactory { - private readonly IDbContextOptions _options; + private const int DefaultMaxBatchSize = 42; + private const int MaxMaxBatchSize = 1000; + private readonly int _maxBatchSize; /// /// This is an internal API that supports the Entity Framework Core infrastructure and not subject to @@ -26,7 +28,16 @@ public SqlServerModificationCommandBatchFactory( IDbContextOptions options) { Dependencies = dependencies; - _options = options; + + _maxBatchSize = Math.Min( + options.Extensions.OfType().FirstOrDefault()?.MaxBatchSize ?? DefaultMaxBatchSize, + MaxMaxBatchSize); + + if (_maxBatchSize <= 0) + { + throw new ArgumentOutOfRangeException( + nameof(RelationalOptionsExtension.MaxBatchSize), RelationalStrings.InvalidMaxBatchSize(_maxBatchSize)); + } } /// @@ -41,9 +52,5 @@ public SqlServerModificationCommandBatchFactory( /// doing so can result in application failures when updating to a new Entity Framework Core release. /// public virtual ModificationCommandBatch Create() - { - var optionsExtension = _options.Extensions.OfType().FirstOrDefault(); - - return new SqlServerModificationCommandBatch(Dependencies, optionsExtension?.MaxBatchSize); - } + => new SqlServerModificationCommandBatch(Dependencies, _maxBatchSize); } diff --git a/test/EFCore.Relational.Tests/TestUtilities/TestModificationCommandBatch.cs b/test/EFCore.Relational.Tests/TestUtilities/TestModificationCommandBatch.cs index 9d50183e4f0..e27a95b4a16 100644 --- a/test/EFCore.Relational.Tests/TestUtilities/TestModificationCommandBatch.cs +++ b/test/EFCore.Relational.Tests/TestUtilities/TestModificationCommandBatch.cs @@ -5,16 +5,11 @@ namespace Microsoft.EntityFrameworkCore.TestUtilities; public class TestModificationCommandBatch : SingularModificationCommandBatch { - private readonly int _maxBatchSize; - public TestModificationCommandBatch( ModificationCommandBatchFactoryDependencies dependencies, int? maxBatchSize) : base(dependencies) - { - _maxBatchSize = maxBatchSize ?? 1; - } + => MaxBatchSize = maxBatchSize ?? 1; - protected override bool IsValid() - => ModificationCommands.Count <= _maxBatchSize; + protected override int MaxBatchSize { get; } } diff --git a/test/EFCore.Relational.Tests/Update/ReaderModificationCommandBatchTest.cs b/test/EFCore.Relational.Tests/Update/ReaderModificationCommandBatchTest.cs index 8bcd49c6c92..2551ec6d605 100644 --- a/test/EFCore.Relational.Tests/Update/ReaderModificationCommandBatchTest.cs +++ b/test/EFCore.Relational.Tests/Update/ReaderModificationCommandBatchTest.cs @@ -125,6 +125,9 @@ public void AddCommand_does_not_add_command_batch_is_invalid() ", batch.CommandText, ignoreLineEndingDifferences: true); + + Assert.Equal(1, batch.StoreCommand.RelationalCommand.Parameters.Count); + Assert.Equal(1, batch.StoreCommand.ParameterValues.Count); } [ConditionalFact] diff --git a/test/EFCore.SqlServer.Tests/Update/SqlServerModificationCommandBatchTest.cs b/test/EFCore.SqlServer.Tests/Update/SqlServerModificationCommandBatchTest.cs index 606372df527..3d6fca78ffa 100644 --- a/test/EFCore.SqlServer.Tests/Update/SqlServerModificationCommandBatchTest.cs +++ b/test/EFCore.SqlServer.Tests/Update/SqlServerModificationCommandBatchTest.cs @@ -61,7 +61,7 @@ private class FakeDbContext : DbContext { } - private static TestSqlServerModificationCommandBatch CreateBatch(int? maxBatchSize = null) + private static TestSqlServerModificationCommandBatch CreateBatch(int maxBatchSize = 42) { var typeMapper = CreateTypeMappingSource(); @@ -100,7 +100,7 @@ private static IModificationCommand CreateModificationCommand( private class TestSqlServerModificationCommandBatch : SqlServerModificationCommandBatch { - public TestSqlServerModificationCommandBatch(ModificationCommandBatchFactoryDependencies dependencies, int? maxBatchSize) + public TestSqlServerModificationCommandBatch(ModificationCommandBatchFactoryDependencies dependencies, int maxBatchSize) : base(dependencies, maxBatchSize) { }