-
Notifications
You must be signed in to change notification settings - Fork 299
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.
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
perf: Rework ExecuteReaderAsync to minimize allocations #528
Merged
cheenamalhotra
merged 5 commits into
dotnet:master
from
Wraith2:perf-registerclosestate
Jul 21, 2020
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
80 changes: 80 additions & 0 deletions
80
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/AAsyncCallContext.cs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
// See the LICENSE file in the project root for more information. | ||
|
||
using System; | ||
using System.Threading.Tasks; | ||
|
||
namespace Microsoft.Data.SqlClient | ||
{ | ||
// this class is a base class for creating derived objects that will store state for async operations | ||
// avoiding the use of closures and allowing caching/reuse of the instances for frequently used async | ||
// calls | ||
// | ||
// DO derive from this and seal your class | ||
// DO add additional fields or properties needed for the async operation and then override Clear to zero them | ||
// DO override AfterClear and use the owner parameter to return the object to a cache location if you have one, this is the purpose of the method | ||
// CONSIDER creating your own Set method that calls the base Set rather than providing a parameterized ctor, it is friendlier to caching | ||
// DO NOT use this class' state after Dispose has been called. It will not throw ObjectDisposedException but it will be a cleared object | ||
|
||
internal abstract class AAsyncCallContext<TOwner, TTask> : IDisposable | ||
where TOwner : class | ||
{ | ||
protected TOwner _owner; | ||
protected TaskCompletionSource<TTask> _source; | ||
protected IDisposable _disposable; | ||
|
||
protected AAsyncCallContext() | ||
{ | ||
} | ||
|
||
protected AAsyncCallContext(TOwner owner, TaskCompletionSource<TTask> source, IDisposable disposable = null) | ||
{ | ||
Set(owner, source, disposable); | ||
} | ||
|
||
protected void Set(TOwner owner, TaskCompletionSource<TTask> source, IDisposable disposable = null) | ||
{ | ||
_owner = owner ?? throw new ArgumentNullException(nameof(owner)); | ||
_source = source ?? throw new ArgumentNullException(nameof(source)); | ||
_disposable = disposable; | ||
} | ||
|
||
protected void ClearCore() | ||
{ | ||
_source = null; | ||
_owner = default; | ||
IDisposable copyDisposable = _disposable; | ||
_disposable = null; | ||
copyDisposable?.Dispose(); | ||
} | ||
|
||
/// <summary> | ||
/// override this method to cleanup instance data before ClearCore is called which will blank the base data | ||
/// </summary> | ||
protected virtual void Clear() | ||
{ | ||
} | ||
|
||
/// <summary> | ||
/// override this method to do work after the instance has been totally blanked, intended for cache return etc | ||
/// </summary> | ||
protected virtual void AfterCleared(TOwner owner) | ||
{ | ||
} | ||
|
||
public void Dispose() | ||
{ | ||
TOwner owner = _owner; | ||
try | ||
{ | ||
Clear(); | ||
} | ||
finally | ||
{ | ||
ClearCore(); | ||
} | ||
AfterCleared(owner); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comment @Wraith2: ContinueWith generally performs much worse than a simple async method with await (not to mention that's it's more brittle wrt exception handling etc.). I can post some benchmarking if you want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though this may be totally justified if TaskContinuationOptions.LongRunning is important here - just making the general remark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Language supported async await would be easier to understand as well.
Whoever wrote the original code had some reason to use imperative constructs instead of language support but I've no idea what it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely.
I suspect a lot of the code was simply written before async/await was introduced (in .NET 4.5, relatively "late"). I'd definitely consider replacing the callback approach as you work through the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine with me but it's down to the MS team to make that sort of decision. I don't want to make working on this library any harder than it already is so I try to replicate the existing style wherever it's sensible to do so in my changes.
There are several library wide things that would be nice to do.
We've got 1 in progress but the others really need 1 to be done and judged stable so we don't have to duplicate a lot of work. 1 will aoso bring perf improvements done in netcore to netfx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I vote for prioritizing 1 - it is currently a quite confusing codebase, which makes community contributions harder than the should be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
working on it bit by bit. #625 The easy part is the identical files. The complex part is the files which have opposing or complex changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure thing, thing kind of cleanup can be done separately of course. I'd suggest that if ContinueWith continuations are being rewritten for unrelated reasons, it may be a good opportunity to just do that with async/await, but of course that's up to the team and you.
I also totally agree that merging the netfx/netcore codebases is a high-priority task.