Performance | Remove manual state machine from setup phase of SqlBulkCopy - #4687
edwardneal wants to merge 7 commits into
Conversation
This is a misnamed method on SqlBulkCopy which calls a misnamed method on SqlConnection. The only thing it does is call RemoveWeakReference.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| // @TODO: CER Exception Handling was removed here (see GH#3581) | ||
| finally | ||
| { | ||
| _columnMappings.ReadOnly = false; |
There was a problem hiding this comment.
Behavioural change 1 of 3: previously, ColumnMappings was marked as read-write immediately after the async task was started. It's now read-only for the entire duration. I think this was originally a bug.
| private Task<T> RegisterForConnectionCloseNotification<T>(Task<T> outerTask) | ||
| { | ||
| SqlConnection connection = _connection; | ||
| if (connection == null) | ||
| { | ||
| // No connection | ||
| throw ADP.ClosedConnectionError(); | ||
| } | ||
|
|
||
| return connection.RegisterForConnectionCloseNotification(outerTask, this, SqlReferenceCollection.BulkCopyTag); | ||
| } |
There was a problem hiding this comment.
Flagging this explicitly: the only thing this does is add the instance to the connection's SqlReferenceCollection while the bulk copy is ongoing. It's replaced with a direct call to the appropriate method, since this style of method doesn't align with the more modern async/await style.
| try | ||
| { | ||
| CleanUpStateObject(); | ||
| CleanUpStateObject(isCancelRequested: !completedSuccessfully); |
There was a problem hiding this comment.
Behavioural change 2 of 3: this is now called whenever the bulk copy throws an exception, and it sends a cancellation signal to the server. Previously, a fault in an async bulk copy didn't actually do this.
| return; | ||
| try | ||
| { | ||
| await reconnectTask.WaitAsync(timeoutCts.Token).ConfigureAwait(false); |
There was a problem hiding this comment.
Behavioural change 3 of 3: the original async connection resiliency logic would continue the reconnection task with the current method - so if a server successfully and then immediately disconnected, it'd loop through this reconnection logic repeatedly. Each reconnection attempt would start a new timer against BulkCopyTimeout.
I've assumed this is a bug, since it'd allow repeated disconnections/reconnections to the server to exceed the user-specified BulkCopyTimeout. We now only attempt to reconnect once - I can add a loop if that's an issue.
| else | ||
| { | ||
| var internalResult = new BulkCopySimpleResultSet(); | ||
| RunParserReliably(internalResult); |
There was a problem hiding this comment.
Small quirk: RunParserReliably and RunParser are identical. I've switched them over to use RunParser, and RunParserReliably can vanish in a follow-up.
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
| _rowSourceType = ValueSourceType.IDataReader; | ||
|
|
||
| WriteRowSourceToServerAsync(reader.FieldCount, CancellationToken.None); //It returns null since _isAsyncBulkCopy = false; | ||
| WriteRowSourceToServerAsync(reader.FieldCount, CancellationToken.None).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
I would really advise against calling GetAwaiter().GetResult() on a ValueTask unless you do know that it's going to complete synchronously. Otherwise, GetAwaiter().GetResult() on ValueTask doesn't guarantee that it's going to wait for it to actually complete.
There was a problem hiding this comment.
I agree with your general point about ValueTask - and we definitely want to avoid that. In this particular case though, the sync APIs unconditionally call ResetWriteToServerGlobalVariables before calling WriteRowSourceToServerAsync. Resetting the variables sets _isAsyncBulkCopy to false, and that variable is threaded through the existing code to ensure that the existing logic uses synchronous paths.
I'd personally prefer to pass something similar to an isAsync parameter through the call chain to make this clearer, but the variable is necessary to support the rest of the copy logic. If SqlBulkCopy is fully migrated, we can hopefully do this.
I could add an extension method which asserts that ValueTask has completed, then calls GetAwaiter().GetResult(), but I'm not sure that'd add much beyond documenting a fairly standard pattern of sharing sync and async implementations (albeit with unusual method names). Would that be helpful, or do you have another suggestion?
There was a problem hiding this comment.
Resetting the variables sets _isAsyncBulkCopy to false, and that variable is threaded through the existing code to ensure that the existing logic uses synchronous paths.
That's certainly a design choice of all times. But then again, there is/was a path with IO from a class ctor, so at this point I'm not really surprised.
I wonder if just making WriteRowSourceToServerAsync to return Task again wouldn't be a much simpler solution. You'll be able to remove AsTask you added, there's no issue with GetAwaiter().GetResult() and it's not like ValueTask brings much in this specific case.
There was a problem hiding this comment.
The design is slightly different to the normal pattern we'd use, I agree - and I'm not strongly attached to the use of Task vs. ValueTask, I've got no problem if that's what the team want me to use.
This design is somewhat similar to SslStream.Read though. You'll note that the top-level synchronous Read method calls ReadAsyncInternal, which returns a ValueTask<int>. It then asserts that the ValueTask is completed and calls .GetAwaiter().GetResult(). This doesn't cause an issue because ReadAsyncInternal is genuinely taking a synchronous path - just as WriteRowSourceToServerAsync is. In both situations, we're not using GetResult() to try to force the ValueTask to complete, we're using it to synchronously observe the ValueTask's result and force it to throw any exceptions.
The primary difference between SslStream.Read and these methods is actually just that debug assertion on vt.IsCompleted. I'm open to adding that alongside a brief comment explaining that that the ValueTask returned is always expected to be completed when called with _isAsyncBulkCopy = false, if you think that would make the behaviour clearer.
There was a problem hiding this comment.
This design is somewhat similar to SslStream.Read though. You'll note that the top-level synchronous Read method calls ReadAsyncInternal, which returns a ValueTask. It then asserts that the ValueTask is completed and calls .GetAwaiter().GetResult(). This doesn't cause an issue because ReadAsyncInternal is genuinely taking a synchronous path - just as WriteRowSourceToServerAsync is. In both situations, we're not using GetResult() to try to force the ValueTask to complete, we're using it to synchronously observe the ValueTask's result and force it to throw any exceptions.
SslStream passes SyncReadWriteAdapter so it is a somewhat explicit contract that it is supposed to be completely sync. Not amazing in my books, but still miles better compared to how SqlClient sets a field and calls it a day. When async is passed as a variable, at least the compiler is going to complain if you forget to pass it to a method. When async is a field? Good luck not forgetting to specify it for every single public method.
That's also ignoring perf implications, where SqlBulkcCopy object is a bit heavier because it also has to account for another field.
There was a problem hiding this comment.
The design is slightly different to the normal pattern we'd use, I agree - and I'm not strongly attached to the use of Task vs. ValueTask, I've got no problem if that's what the team want me to use.
I just don't see a reason for ValueTask, especially since you already have to convert it to Task anyway in multiple methods.
There was a problem hiding this comment.
I think your point around using a variable makes perfect sense. It's still needed today for the hand-written state machine, but it'd definitely be a good idea to pass that as a parameter where possible. I'll make sure that happens if it's possible in a follow-up (once everything is done via async/await.) Thanks.
The original reason I used ValueTask was to reflect that the method could has both synchronous and asynchronous completion paths (because this method can be called by both WriteToServer and WriteToServerAsync). When calling the method from WriteToServer we'll need to observe it via GetAwaiter().GetResult() no matter what - so I didn't see Task as providing anything useful here for the (admittedly minor) allocation overhead.
If the return type causes confusion within the team, I'm happy to either add a comment, assert ValueTask.IsCompleted, or return a Task instance
5 participants
Description
This is the same idea as #4685; strangely enough, it stops at the same boundary. The primary difference is that where #4685 handles the transport of rows, this PR handles the setup phase - metadata retrieval, connection resiliency and so on.
This PR removes the manual state machine orchestration from the outer layers of SqlBulkCopy, replacing it with native async/await. This simplifies those layers, although it leaves behind some modest scar tissue: slightly odd method names.
I've moved commit-by-commit, with a single cleanup layer at the end.
Issues
Contributes to #3459.
Testing
Existing SqlBulkCopy tests continue to pass. Performance benchmarking highlights no inexplicable performance regressions (although there's a little CPU noise - I'm testing against a local SQL Server), plus a few very minor reductions in memory usage.
Benchmark results
All benchmarks are run against .NET 10. A few points of note:
BulkCopyDeepwithUseCompatibilityAsyncset tofalse. This is because I was benchmarking against a local SQL Server - the first test was slow, no matter which test it was.UseCompatibilityAsynccovers cases where theUseCompatibilityAsyncBehaviourand theUseCompatibilityProcessSniAppContext switches are set to true or false. I don't expect it to have a bearing here, since we're not making sufficiently heavy use of these code paths during startup.