Skip to content

Commit 05df554

Browse files
Wraith2Wraith2
authored
Improve async string perf and fix reading chars with initial offset. (#3377)
* refine handling of moving between replay and continue states * for plp reads increase the buffer size in relatively small chunks to decrease memcopy and gc time spent * change TryReadPlpUnicodeChars to check plp length field not max len parameter * sync netfx and netcore * add test covering #3331 and fix * re-remove CurrentPacketIndex, fix test name error * review feedback
1 parent 97be747 commit 05df554

File tree

4 files changed

+121
-21
lines changed

4 files changed

+121
-21
lines changed

‎src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs

Lines changed: 11 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -13102,17 +13102,17 @@ bool writeDataSizeToSnapshot
1310213102
||
1310313103
(buff.Length >= offst + len)
1310413104
||
13105-
(buff.Length == (startOffsetByteCount >> 1) + 1),
13105+
(buff.Length >= (startOffsetByteCount >> 1) + 1),
1310613106
"Invalid length sent to ReadPlpUnicodeChars()!"
1310713107
);
1310813108
charsLeft = len;
1310913109

13110-
// If total length is known up front, the length isn't specified as unknown
13111-
// and the caller doesn't pass int.max/2 indicating that it doesn't know the length
13112-
// allocate the whole buffer in one shot instead of realloc'ing and copying over each time
13113-
if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && len < (int.MaxValue >> 1))
13110+
// If total data length is known up front from the plp header by being not SQL_PLP_UNKNOWNLEN
13111+
// and the number of chars required is less than int.max/2 allocate the entire buffer now to avoid
13112+
// later needing to repeatedly allocate new target buffers and copy data as we discover new data
13113+
if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && stateObj._longlen < (int.MaxValue >> 1))
1311413114
{
13115-
if (supportRentedBuff && len < 1073741824) // 1 Gib
13115+
if (supportRentedBuff && stateObj._longlen < 1073741824) // 1 Gib
1311613116
{
1311713117
buff = ArrayPool<char>.Shared.Rent((int)Math.Min((int)stateObj._longlen, len));
1311813118
rentedBuff = true;
@@ -13147,8 +13147,7 @@ bool writeDataSizeToSnapshot
1314713147

1314813148
totalCharsRead = (startOffsetByteCount >> 1);
1314913149
charsLeft -= totalCharsRead;
13150-
offst = totalCharsRead;
13151-
13150+
offst += totalCharsRead;
1315213151

1315313152
while (charsLeft > 0)
1315413153
{
@@ -13166,7 +13165,10 @@ bool writeDataSizeToSnapshot
1316613165
}
1316713166
else
1316813167
{
13169-
newbuf = new char[offst + charsRead];
13168+
// grow by an arbitrary number of packets to avoid needing to reallocate
13169+
// the newbuf on each loop iteration of long packet sequences which causes
13170+
// a performance problem as we spend large amounts of time copying and in gc
13171+
newbuf = new char[offst + charsRead + (stateObj.GetPacketSize() * 8)];
1317013172
rentedBuff = false;
1317113173
}
1317213174

‎src/Microsoft.Data.SqlClient/netfx/src/Microsoft/Data/SqlClient/TdsParser.cs

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13265,7 +13265,7 @@ bool writeDataSizeToSnapshot
1326513265
{
1326613266
int charsRead = 0;
1326713267
int charsLeft = 0;
13268-
13268+
1326913269
if (stateObj._longlen == 0)
1327013270
{
1327113271
Debug.Assert(stateObj._longlenleft == 0);
@@ -13275,21 +13275,21 @@ bool writeDataSizeToSnapshot
1327513275

1327613276
Debug.Assert((ulong)stateObj._longlen != TdsEnums.SQL_PLP_NULL, "Out of sync plp read request");
1327713277
Debug.Assert(
13278-
(buff == null && offst == 0)
13279-
||
13278+
(buff == null && offst == 0)
13279+
||
1328013280
(buff.Length >= offst + len)
1328113281
||
13282-
(buff.Length == (startOffsetByteCount >> 1) + 1),
13282+
(buff.Length >= (startOffsetByteCount >> 1) + 1),
1328313283
"Invalid length sent to ReadPlpUnicodeChars()!"
1328413284
);
1328513285
charsLeft = len;
1328613286

1328713287
// If total length is known up front, the length isn't specified as unknown
1328813288
// and the caller doesn't pass int.max/2 indicating that it doesn't know the length
1328913289
// allocate the whole buffer in one shot instead of realloc'ing and copying over each time
13290-
if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && len < (int.MaxValue >> 1))
13290+
if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && stateObj._longlen < (int.MaxValue >> 1))
1329113291
{
13292-
if (supportRentedBuff && len < 1073741824) // 1 Gib
13292+
if (supportRentedBuff && stateObj._longlen < 1073741824) // 1 Gib
1329313293
{
1329413294
buff = ArrayPool<char>.Shared.Rent((int)Math.Min((int)stateObj._longlen, len));
1329513295
rentedBuff = true;
@@ -13324,9 +13324,9 @@ bool writeDataSizeToSnapshot
1332413324

1332513325
totalCharsRead = (startOffsetByteCount >> 1);
1332613326
charsLeft -= totalCharsRead;
13327-
offst = totalCharsRead;
13328-
13329-
13327+
offst += totalCharsRead;
13328+
13329+
1333013330
while (charsLeft > 0)
1333113331
{
1333213332
if (!partialReadInProgress)
@@ -13343,7 +13343,10 @@ bool writeDataSizeToSnapshot
1334313343
}
1334413344
else
1334513345
{
13346-
newbuf = new char[offst + charsRead];
13346+
// grow by an arbitrary number of packets to avoid needing to reallocate
13347+
// the newbuf on each loop iteration of long packet sequences which causes
13348+
// a performance problem as we spend large amounts of time copying and in gc
13349+
newbuf = new char[offst + charsRead + (stateObj.GetPacketSize() * 8)];
1334713350
rentedBuff = false;
1334813351
}
1334913352

@@ -13383,7 +13386,7 @@ bool writeDataSizeToSnapshot
1338313386
&& (charsLeft > 0)
1338413387
)
1338513388
{
13386-
byte b1 = 0;
13389+
byte b1 = 0;
1338713390
byte b2 = 0;
1338813391
if (partialReadInProgress)
1338913392
{

‎src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStateObject.cs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1360,6 +1360,11 @@ internal bool SetPacketSize(int size)
13601360
return false;
13611361
}
13621362

1363+
internal int GetPacketSize()
1364+
{
1365+
return _inBuff.Length;
1366+
}
1367+
13631368
///////////////////////////////////////
13641369
// Buffer read methods - data values //
13651370
///////////////////////////////////////
@@ -4027,6 +4032,7 @@ internal void CheckStack(string trace)
40274032
Debug.Assert(_stateObj._permitReplayStackTraceToDiffer || prev.Stack == trace, "The stack trace on subsequent replays should be the same");
40284033
}
40294034
}
4035+
40304036
#endif
40314037
public bool ContinueEnabled => !LocalAppContextSwitches.UseCompatibilityAsyncBehaviour;
40324038

‎src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataReaderTest/DataReaderTest.cs

Lines changed: 90 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
// See the LICENSE file in the project root for more information.
44

55
using System;
6+
using System.Buffers;
67
using System.Collections.Generic;
78
using System.Data;
89
using System.Data.SqlTypes;
@@ -268,7 +269,7 @@ first_name varchar(100) null,
268269
}
269270

270271
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
271-
public static void CheckNullRowVersionIsBDNull()
272+
public static void CheckNullRowVersionIsDBNull()
272273
{
273274
lock (s_rowVersionLock)
274275
{
@@ -713,6 +714,94 @@ DROP TABLE IF EXISTS [{tableName}]
713714
}
714715
}
715716

717+
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup))]
718+
public static async Task CanGetCharsSequentially()
719+
{
720+
const CommandBehavior commandBehavior = CommandBehavior.SequentialAccess | CommandBehavior.SingleResult;
721+
const int length = 32000;
722+
const string sqlCharWithArg = "SELECT CONVERT(BIGINT, 1) AS [Id], CONVERT(NVARCHAR(MAX), @input) AS [Value];";
723+
724+
using (var sqlConnection = new SqlConnection(DataTestUtility.TCPConnectionString))
725+
{
726+
await sqlConnection.OpenAsync();
727+
728+
StringBuilder inputBuilder = new StringBuilder(length);
729+
Random random = new Random();
730+
for (int i = 0; i < length; i++)
731+
{
732+
inputBuilder.Append((char)random.Next(0x30, 0x5A));
733+
}
734+
string input = inputBuilder.ToString();
735+
736+
using (var sqlCommand = new SqlCommand())
737+
{
738+
sqlCommand.Connection = sqlConnection;
739+
sqlCommand.CommandTimeout = 0;
740+
sqlCommand.CommandText = sqlCharWithArg;
741+
sqlCommand.Parameters.Add(new SqlParameter("@input", SqlDbType.NVarChar, -1) { Value = input });
742+
743+
using (var sqlReader = await sqlCommand.ExecuteReaderAsync(commandBehavior))
744+
{
745+
if (await sqlReader.ReadAsync())
746+
{
747+
long id = sqlReader.GetInt64(0);
748+
if (id != 1)
749+
{
750+
Assert.Fail("Id not 1");
751+
}
752+
753+
var sliced = GetPooledChars(sqlReader, 1, input);
754+
if (!sliced.SequenceEqual(input.ToCharArray()))
755+
{
756+
Assert.Fail("sliced != input");
757+
}
758+
}
759+
}
760+
}
761+
}
762+
763+
static char[] GetPooledChars(SqlDataReader sqlDataReader, int ordinal, string input)
764+
{
765+
var buffer = ArrayPool<char>.Shared.Rent(8192);
766+
int offset = 0;
767+
while (true)
768+
{
769+
int read = (int)sqlDataReader.GetChars(ordinal, offset, buffer, offset, buffer.Length - offset);
770+
if (read == 0)
771+
{
772+
break;
773+
}
774+
775+
ReadOnlySpan<char> fetched = buffer.AsSpan(offset, read);
776+
ReadOnlySpan<char> origin = input.AsSpan(offset, read);
777+
778+
if (!fetched.Equals(origin, StringComparison.Ordinal))
779+
{
780+
Assert.Fail($"chunk (start:{offset}, for:{read}), is not the same as the input");
781+
}
782+
783+
offset += read;
784+
785+
if (buffer.Length - offset < 128)
786+
{
787+
buffer = Resize(buffer);
788+
}
789+
}
790+
791+
var sliced = buffer.AsSpan(0, offset).ToArray();
792+
ArrayPool<char>.Shared.Return(buffer);
793+
return sliced;
794+
795+
static char[] Resize(char[] buffer)
796+
{
797+
var newBuffer = ArrayPool<char>.Shared.Rent(buffer.Length * 2);
798+
Array.Copy(buffer, newBuffer, buffer.Length);
799+
ArrayPool<char>.Shared.Return(buffer);
800+
return newBuffer;
801+
}
802+
}
803+
}
804+
716805
// Synapse: Cannot find data type 'rowversion'.
717806
[ConditionalFact(typeof(DataTestUtility), nameof(DataTestUtility.AreConnStringsSetup), nameof(DataTestUtility.IsNotAzureSynapse))]
718807
public static void CheckLegacyNullRowVersionIsEmptyArray()

0 commit comments

Comments
 (0)