Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Recycling relational and ADO.NET objects in query pipeline #24207

Merged
merged 2 commits into from
Mar 4, 2021
Merged

Recycling relational and ADO.NET objects in query pipeline #24207

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a696a4
Recycle various objects participating in query
roji Feb 25, 2021
b72ac84
Take care of split query commands
roji Feb 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions src/EFCore.Relational/Query/Internal/FromSqlQueryingEnumerable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,10 @@ public virtual DbCommand CreateDbCommand()
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual string ToQueryString()
=> _relationalQueryContext.RelationalQueryStringFactory.Create(CreateDbCommand());
{
using var dbCommand = CreateDbCommand();
return _relationalQueryContext.RelationalQueryStringFactory.Create(dbCommand);
}

/// <summary>
/// This is an internal API that supports the Entity Framework Core infrastructure and not subject to
Expand Down Expand Up @@ -160,6 +163,7 @@ private sealed class Enumerator : IEnumerator<T>
private readonly bool _detailedErrorsEnabled;
private readonly IConcurrencyDetector? _concurrencyDetector;

private IRelationalCommand? _relationalCommand;
private RelationalDataReader? _dataReader;
private int[]? _indexMap;

Expand Down Expand Up @@ -224,9 +228,12 @@ private static bool InitializeReader(Enumerator enumerator)
{
EntityFrameworkEventSource.Log.QueryExecuting();

var relationalCommand = enumerator._relationalCommandCache.GetRelationalCommand(
var relationalCommandTemplate = enumerator._relationalCommandCache.GetRelationalCommand(
enumerator._relationalQueryContext.ParameterValues);

var relationalCommand = enumerator._relationalCommand = enumerator._relationalQueryContext.Connection.RentCommand();
relationalCommand.PopulateFromTemplate(relationalCommandTemplate);

enumerator._dataReader = relationalCommand.ExecuteReader(
new RelationalCommandParameterObject(
enumerator._relationalQueryContext.Connection,
Expand All @@ -245,8 +252,12 @@ private static bool InitializeReader(Enumerator enumerator)

public void Dispose()
{
_dataReader?.Dispose();
_dataReader = null;
if (_dataReader is not null)
{
_relationalQueryContext.Connection.ReturnCommand(_relationalCommand!);
_dataReader?.Dispose();
_dataReader = null;
}
}

public void Reset()
Expand All @@ -265,6 +276,7 @@ private sealed class AsyncEnumerator : IAsyncEnumerator<T>
private readonly bool _detailedErrorsEnabled;
private readonly IConcurrencyDetector? _concurrencyDetector;

private IRelationalCommand? _relationalCommand;
private RelationalDataReader? _dataReader;
private int[]? _indexMap;

Expand Down Expand Up @@ -331,9 +343,12 @@ private static async Task<bool> InitializeReaderAsync(AsyncEnumerator enumerator
{
EntityFrameworkEventSource.Log.QueryExecuting();

var relationalCommand = enumerator._relationalCommandCache.GetRelationalCommand(
var relationalCommandTemplate = enumerator._relationalCommandCache.GetRelationalCommand(
enumerator._relationalQueryContext.ParameterValues);

var relationalCommand = enumerator._relationalCommand = enumerator._relationalQueryContext.Connection.RentCommand();
relationalCommand.PopulateFromTemplate(relationalCommandTemplate);

enumerator._dataReader = await relationalCommand.ExecuteReaderAsync(
new RelationalCommandParameterObject(
enumerator._relationalQueryContext.Connection,
Expand All @@ -356,6 +371,8 @@ public ValueTask DisposeAsync()
{
if (_dataReader != null)
{
_relationalQueryContext.Connection.ReturnCommand(_relationalCommand!);

var dataReader = _dataReader;
_dataReader = null;

Expand Down
29 changes: 23 additions & 6 deletions src/EFCore.Relational/Query/Internal/SingleQueryingEnumerable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,10 @@ public virtual DbCommand CreateDbCommand()
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual string ToQueryString()
=> _relationalQueryContext.RelationalQueryStringFactory.Create(CreateDbCommand());
{
using var dbCommand = CreateDbCommand();
return _relationalQueryContext.RelationalQueryStringFactory.Create(dbCommand);
}

private sealed class Enumerator : IEnumerator<T>
{
Expand All @@ -128,6 +131,7 @@ private sealed class Enumerator : IEnumerator<T>
private readonly bool _detailedErrorsEnabled;
private readonly IConcurrencyDetector? _concurrencyDetector;

private IRelationalCommand? _relationalCommand;
private RelationalDataReader? _dataReader;
private SingleQueryResultCoordinator? _resultCoordinator;

Expand Down Expand Up @@ -218,9 +222,12 @@ private static bool InitializeReader(Enumerator enumerator)
{
EntityFrameworkEventSource.Log.QueryExecuting();

var relationalCommand = enumerator._relationalCommandCache.GetRelationalCommand(
var relationalCommandTemplate = enumerator._relationalCommandCache.GetRelationalCommand(
enumerator._relationalQueryContext.ParameterValues);

var relationalCommand = enumerator._relationalCommand = enumerator._relationalQueryContext.Connection.RentCommand();
relationalCommand.PopulateFromTemplate(relationalCommandTemplate);

enumerator._dataReader = relationalCommand.ExecuteReader(
new RelationalCommandParameterObject(
enumerator._relationalQueryContext.Connection,
Expand All @@ -239,8 +246,12 @@ private static bool InitializeReader(Enumerator enumerator)

public void Dispose()
{
_dataReader?.Dispose();
_dataReader = null;
if (_dataReader is not null)
{
_relationalQueryContext.Connection.ReturnCommand(_relationalCommand!);
_dataReader.Dispose();
_dataReader = null;
}
}

public void Reset()
Expand All @@ -258,6 +269,7 @@ private sealed class AsyncEnumerator : IAsyncEnumerator<T>
private readonly bool _detailedErrorsEnabled;
private readonly IConcurrencyDetector? _concurrencyDetector;

private IRelationalCommand? _relationalCommand;
private RelationalDataReader? _dataReader;
private SingleQueryResultCoordinator? _resultCoordinator;

Expand Down Expand Up @@ -351,9 +363,12 @@ private static async Task<bool> InitializeReaderAsync(AsyncEnumerator enumerator
{
EntityFrameworkEventSource.Log.QueryExecuting();

var relationalCommand = enumerator._relationalCommandCache.GetRelationalCommand(
var relationalCommandTemplate = enumerator._relationalCommandCache.GetRelationalCommand(
enumerator._relationalQueryContext.ParameterValues);

var relationalCommand = enumerator._relationalCommand = enumerator._relationalQueryContext.Connection.RentCommand();
relationalCommand.PopulateFromTemplate(relationalCommandTemplate);

enumerator._dataReader = await relationalCommand.ExecuteReaderAsync(
new RelationalCommandParameterObject(
enumerator._relationalQueryContext.Connection,
Expand All @@ -374,8 +389,10 @@ private static async Task<bool> InitializeReaderAsync(AsyncEnumerator enumerator

public ValueTask DisposeAsync()
{
if (_dataReader != null)
if (_dataReader is not null)
{
_relationalQueryContext.Connection.ReturnCommand(_relationalCommand!);

var dataReader = _dataReader;
_dataReader = null;

Expand Down
40 changes: 28 additions & 12 deletions src/EFCore.Relational/Query/Internal/SplitQueryingEnumerable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,10 @@ public virtual DbCommand CreateDbCommand()
/// doing so can result in application failures when updating to a new Entity Framework Core release.
/// </summary>
public virtual string ToQueryString()
=> $"{_relationalQueryContext.RelationalQueryStringFactory.Create(CreateDbCommand())}{Environment.NewLine}{Environment.NewLine}{RelationalStrings.SplitQueryString}";
{
using var dbCommand = CreateDbCommand();
return $"{_relationalQueryContext.RelationalQueryStringFactory.Create(dbCommand)}{Environment.NewLine}{Environment.NewLine}{RelationalStrings.SplitQueryString}";
}

private sealed class Enumerator : IEnumerator<T>
{
Expand All @@ -136,6 +139,7 @@ private sealed class Enumerator : IEnumerator<T>
private readonly bool _detailedErrorsEnabled;
private readonly IConcurrencyDetector? _concurrencyDetector;

private IRelationalCommand? _relationalCommand;
private RelationalDataReader? _dataReader;
private SplitQueryResultCoordinator? _resultCoordinator;
private IExecutionStrategy? _executionStrategy;
Expand Down Expand Up @@ -212,9 +216,12 @@ private static bool InitializeReader(Enumerator enumerator)
{
EntityFrameworkEventSource.Log.QueryExecuting();

var relationalCommand = enumerator._relationalCommandCache.GetRelationalCommand(
var relationalCommandTemplate = enumerator._relationalCommandCache.GetRelationalCommand(
enumerator._relationalQueryContext.ParameterValues);

var relationalCommand = enumerator._relationalCommand = enumerator._relationalQueryContext.Connection.RentCommand();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the split queries are still using old system.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, this would have been a subtle bug.

I considered introducing a new type - maybe RelationalCommandTemplate - which cannot be executed, this way we immediately catch any bugs where we try to directly execute something that is not meant for execution (like here). I didn't do it yet because of the breaking change (and not strictly necessary), do you think that's valuable enough?

@smitpatel @AndriySvyryd @ajcvickers

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smitpatel regardless of RelationalCommandTemplate, I pushed a fix - please take a look (it's in ShaperProcessingExpressionVisitor 😨😨😨).

This slightly regresses split query performance, since we don't cache the related commands, and so allocate an additional RelationalCommand and populate it. This seems pretty negligible compared optimizations like #10878 - would you be OK with merging things as they area and opening an issue to optimize in the future?

To optimize this, related commands would need to be returned the the pool once their reader is consumed. For MARS, The SQL Server implementation of RentCommand could manage more than one RelationalCommand.

relationalCommand.PopulateFromTemplate(relationalCommandTemplate);

enumerator._dataReader = relationalCommand.ExecuteReader(
new RelationalCommandParameterObject(
enumerator._relationalQueryContext.Connection,
Expand All @@ -233,20 +240,24 @@ private static bool InitializeReader(Enumerator enumerator)

public void Dispose()
{
_dataReader?.Dispose();
if (_resultCoordinator != null)
if (_dataReader is not null)
{
foreach (var dataReader in _resultCoordinator.DataReaders)
_relationalQueryContext.Connection.ReturnCommand(_relationalCommand!);
_dataReader.Dispose();
if (_resultCoordinator != null)
{
dataReader?.DataReader.Dispose();
}
foreach (var dataReader in _resultCoordinator.DataReaders)
{
dataReader?.DataReader.Dispose();
}

_resultCoordinator.DataReaders.Clear();
_resultCoordinator.DataReaders.Clear();

_resultCoordinator = null;
}
_resultCoordinator = null;
}

_dataReader = null;
_dataReader = null;
}
}

public void Reset()
Expand All @@ -265,6 +276,7 @@ private sealed class AsyncEnumerator : IAsyncEnumerator<T>
private readonly bool _detailedErrorEnabled;
private readonly IConcurrencyDetector? _concurrencyDetector;

private IRelationalCommand? _relationalCommand;
private RelationalDataReader? _dataReader;
private SplitQueryResultCoordinator? _resultCoordinator;
private IExecutionStrategy? _executionStrategy;
Expand Down Expand Up @@ -348,9 +360,12 @@ private static async Task<bool> InitializeReaderAsync(AsyncEnumerator enumerator
{
EntityFrameworkEventSource.Log.QueryExecuting();

var relationalCommand = enumerator._relationalCommandCache.GetRelationalCommand(
var relationalCommandTemplate = enumerator._relationalCommandCache.GetRelationalCommand(
enumerator._relationalQueryContext.ParameterValues);

var relationalCommand = enumerator._relationalCommand = enumerator._relationalQueryContext.Connection.RentCommand();
relationalCommand.PopulateFromTemplate(relationalCommandTemplate);

enumerator._dataReader = await relationalCommand.ExecuteReaderAsync(
new RelationalCommandParameterObject(
enumerator._relationalQueryContext.Connection,
Expand All @@ -373,6 +388,7 @@ public async ValueTask DisposeAsync()
{
if (_dataReader != null)
{
_relationalQueryContext.Connection.ReturnCommand(_relationalCommand!);
await _dataReader.DisposeAsync().ConfigureAwait(false);
if (_resultCoordinator != null)
{
Expand Down
Loading