Repository navigation
Open
Feature
1 / 31 of 3 issues completed
Copy link
Labels
ApprovedUse for Features approved for implementation.Use for Features approved for implementation.Area\AsyncPerformance 📈Issues that are targeted to performance improvements.Issues that are targeted to performance improvements.
Description
Reading a 10MB VARBINARY(MAX) value asynchronously takes around 5 seconds my machine, while doing the same synchronously takes around 20ms. Increasing the data size to 20MB increases the running time to around 52 seconds.
These numbers were with Microsoft.Data.SqlClient 1.1.3, but 2.0.0-preview4.20142.4 has the same issue (it actually seems slightly slower). Note that I'm not posting final BDN results because the benchmark ran extremely slowly.
Note that this looks very similar to #245, but that issue was fixed for 1.1.0. The difference may be that here we're writing binary data - or possibly the bigger size.
Benchmark code
[MemoryDiagnoser]
public class Benchmarks
{
const string ConnectionString = "Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0";
[GlobalSetup]
public void Setup()
{
using var conn = new SqlConnection(ConnectionString);
conn.Open();
using var cmd = conn.CreateCommand();
cmd.CommandText = @"
IF OBJECT_ID('dbo.data', 'U') IS NOT NULL
DROP TABLE data;
CREATE TABLE data (id INT, foo VARBINARY(MAX))
";
cmd.ExecuteNonQuery();
cmd.CommandText = "INSERT INTO data (id, foo) VALUES (@id, @foo)";
cmd.Parameters.AddWithValue("id", 1);
cmd.Parameters.AddWithValue("foo", new byte[1024 * 1024 * 10]);
cmd.ExecuteNonQuery();
}
[Benchmark]
public async ValueTask<int> Async()
{
using var conn = new SqlConnection(ConnectionString);
using var cmd = new SqlCommand("SELECT foo FROM data", conn);
await conn.OpenAsync();
return ((byte[])await cmd.ExecuteScalarAsync()).Length;
}
[Benchmark]
public async ValueTask<int> Sync()
{
using var conn = new SqlConnection(ConnectionString);
using var cmd = new SqlCommand("SELECT foo FROM data", conn);
conn.Open();
return ((byte[])cmd.ExecuteScalar()).Length;
}
static void Main(string[] args)
=> BenchmarkRunner.Run<Benchmarks>();
}Originally raised in dotnet/efcore#21147
Reacted by Eli-Black-Work, Tri Ân, yepeekai, Ruslan Batkaev, hilari0n, Adam Willden, CodeEx.CN, Logan Dam, cr, Andrew Scott and 154 more
Reacted by Jason Triplett
Reacted by Duranom, Leo Erlandsson, tedchirvasiu, Haakon and Jason Triplett
Metadata
Metadata
Assignees
Labels
ApprovedUse for Features approved for implementation.Use for Features approved for implementation.Area\AsyncPerformance 📈Issues that are targeted to performance improvements.Issues that are targeted to performance improvements.
Type
Projects
- StatusShow more project fieldsIn validation
Activity
ErikEJ commentedon Jun 5, 2020
Cannot see much relation to #245 really.
DavoudEshtehari commentedon Jun 5, 2020
Here is the result of the test at NetCore & Netfx:
Wraith2 commentedon Jun 9, 2020
Working from memory here. Reading a MAX defined field will mean using chunked returns and the complete length of the field may not be known ahead of time so the optimization I put in place for reading won't help. If that's right then you'll see the same horrific escalating performance with increased data sizes as the reader continuously allocates and releases the temporary buffers, those GC numbers seem to support that.
It might be possible to do adapt the cached buffer we've got for multipacket known lengths to work like a list and auto expand as we go. What I can't do is prevent the repeated rescanning of the async packet queue.
Wraith2 commentedon Jun 10, 2020
I've had a look into the profiles and I was wrong. The optimization from #245 is being used because it's reading as PLP data. The excessive memory usage is caused mostly by input packet buffers on the TDS side being lost into the snapshot packet chain. The time escalation is cause by the snapshot chain repeatedly being replayed.
The only currently available mitigations are to increase the packet size to as close to 16K as you can get or to use a stream reader mode which should work on a per-packet basis and not buffer the entire contents (I'm a bit hazy on that for async, it probably needs checking).
The long term fix would be to rewrite async reads to avoid replaying the entire packet chain on every receipt. This is akin to brain surgery. I've looked into this while reviewing other performance issues around async and it isn't something to attempt lightly and if I managed it solo and dropped a PR for it the team they may all justifiably quit in protest.
The
Wraith2 commentedon Jun 10, 2020
Oh, and this in System.Data.Common causes about 1/3 of the allocations and really isn't helpful.
https://github.com/dotnet/runtime/blob/7a57b927c3bdc31b91b7605f25f8598b8e6e58b8/src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLBinary.cs#L61-L74
We already use type workarounds with an overlay type to access the single value field directly to create SqlBinary instances so we could do the same to get it out but we'd then be handing the user a direct reference to a mutable array instead of a copy an I've no doubt that some idiot somewhere in the world is relying on the copy semantic to make their code work.
roji commentedon Jun 11, 2020
@Wraith2
You're referring to the fact that the binary data is copied out into an array that's given to the user, right? If so, isn't that the same between sync and async?
In any case, that's indeed a lot of allocations, but the alternative would be to return memory that's owned by SqlClient, right? If I'm understanding things right, that would mean that if the user reads the next row, the reference they get would suddenly start pointing to a totally different value... I did think about this at some point as an advanced/high-perf/"unsafe" API in dotnet/runtime#24899 - but that's tricky because of the lifetime issue, and also because the entire binary data isn't necessarily in memory if CommandBehavior.Sequential is used (as it should if values are big).
Anyway, all that stuff is less important than making async at least perform similar to sync...
Wraith2 commentedon Jun 11, 2020
Yes, and yes.
Each row of data is stored in an array of SqlBuffer instances stored in the reader, The SqlBuffer type is a discriminated union and it'll hold a reference to an SqlBinary. When you move row or move field in sequential mode the buffer instance is dropped to the GC. The backing byte[] array inside SqlBinary is not re-used.
If we returned the array directly it would only have an effect on multiple reads of the same field in the same row and the only way to observe a change of behaviour would be to change the contents of the array and then re-request it from the reader. It's a pretty clear optimization that I'd like to be able to make but I'm pretty sure I can't because someone will have relied on it returning a new array each time.
As you suggest the ideal approach would be to use a ReadOnlyMemory then it'd all be safe but that'll require a runtime api change and for all providers to consume it.
The entire array is skipped in Sequential mode if the field is not requested, as you say.
In standard mode it will be read into memory and kept until Read() is called to move to the next row if it or any field after it in the column order is read.
I can sort of understand how it might be done but it's daunting and difficult.
roji commentedon Jun 11, 2020
Wouldn't it be possible for a provider to simply support
GetFieldValue<ReadOnlyMemory<byte>>(), without introducing a new API?I can certainly understand (and appreciate) that, and I'm definitely not suggesting anyone do anything specific (was just reporting the problem). Along with #547 and possibly other async-related issues, it seems that at least documenting these limitations could help users avoid pitfalls.
BTW do you think there are any workarounds to this, besides switching to sync?
Wraith2 commentedon Jun 11, 2020
I hadn't considered that, nice idea.
Currently the only workaround I can think of is to use a stream read overload because I think those drive the parser per-packet directly so you don't have to replay the packet queue, that'll need verifying.
roji commentedon Jun 11, 2020
That at least sounds like a good workaround if it works! Definitely worth benchmarking that.
Wraith2 commentedon Jun 11, 2020
Unless I've got something very wrong it won't work because there's a bug.
This should work but freezes in a task wait after a single read cycle, the second read never completed. IF you change it to standard mode it'll work but it does so by fetching the entire field and giving you a reader over the byte[]. So no workaround, this needs fixing if the team agree that it's a bug.
roji commentedon Jun 11, 2020
Thanks for testing this... Definitely looks like a bug to me.
Is this at least fast - in the sense that it resembles the sync perf rather than the async? If so it's at least a workaround, even if not ideal...
Wraith2 commentedon Jun 11, 2020
So no. The only way to get the speed benefit Is to put the reader in sequential mode.
SqlClient/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlDataReader.cs
Line 1557 in f2fd632
There's also an interesting comment elsewhere in the file that says streaming isn't supported when using encryption. I think that makes streaming unusable for a generic consumer like EFCore because you'll have to special case it per driver.
We need to fix the sequential stream issue and then we need to rewrite async reads.
roji commentedon Jun 11, 2020
Thanks for the details @Wraith2.
290 remaining items
Wraith2 commentedon Mar 27, 2026
Version 7.0 is live https://github.com/dotnet/SqlClient/blob/main/release-notes/7.0/7.0.0.md
You should now be able to test and use the AppContext detailed earlier in this thread to enable the new behaviour.
For this version the old behaviour is the default for safety reasons, those of you who remember the last time we tried to put this new behaviour live will remember why. I'd appreciate any feedback of testing, positive or negative, to help inform any future decision to turn on the new switches by default.
paulmedynski commentedon Mar 27, 2026
AppContext switch mitigation (v7.0.0)
SqlClient 7.0.0 includes experimental code behind AppContext switches that directly addresses the root cause (full snapshot replay on every async retry). Setting both switches below enables a continuation-based PLP read path that resumes from where the last read left off, instead of replaying all previously received packets:
Important: Both switches must be set together. Setting
UseCompatibilityAsyncBehaviour=falsealone has no effect — theUseCompatibilityProcessSnigetter forcibly overrides it totrue. Set these before opening anySqlConnection, as switch values are cached on first access.This enables the packet multiplexer (
UseCompatibilityProcessSni=false) and continuation-based snapshot replay (UseCompatibilityAsyncBehaviour=false). The retry cost drops from O(n) (replay all prior packets) to O(1) per packet. These paths are still considered experimental — please report any issues you encounter.✨ Enable SqlClient v7 improved async/SNI behavior in tests
✨ Enable SqlClient v7 improved async/SNI behavior in tests
✨ Enable SqlClient v7 improved async/SNI behavior in tests
✨ Enable SqlClient v7 improved async/SNI behavior in tests
✨ Enable SqlClient v7 improved async/SNI behavior in tests
Enable Opt-in features in Sql Server Tests (#1681)
第8章:データベースアクセスと ORM(Entity Framework Core)を追加 (#54)
priyankatiwari08 commentedon Sep 17, 2026
/triage