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

Add array rental capability in TryReadPlpUnicodeChars #1866

Merged
merged 3 commits into from Feb 1, 2023
Merged

Add array rental capability in TryReadPlpUnicodeChars #1866

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
53d87e3
add array rental capability in RryReadPlpUnicodeChars
Wraith2 Dec 9, 2022
debe78b
remove unneeded variable
Wraith2 Dec 12, 2022
e845b1d
remove uneeded rental buffer return check
Wraith2 Dec 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
58 changes: 47 additions & 11 deletions src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
View file
Open in desktop
Expand Up @@ -5684,8 +5684,8 @@ private bool TryReadSqlStringValue(SqlBuffer value, byte type, int length, Encod
if (isPlp)
{
char[] cc = null;

if (!TryReadPlpUnicodeChars(ref cc, 0, length >> 1, stateObj, out length))
bool buffIsRented = false;
if (!TryReadPlpUnicodeChars(ref cc, 0, length >> 1, stateObj, out length, supportRentedBuff: true, rentedBuff: ref buffIsRented))
{
return false;
}
Expand All @@ -5697,6 +5697,16 @@ private bool TryReadSqlStringValue(SqlBuffer value, byte type, int length, Encod
{
s = "";
}
if (buffIsRented)
{
// do not use clearArray:true on the rented array because it can be massively larger
// than the space we've used and we would incur performance clearing memory that
// we haven't used and can't leak out information.
// clear only the length that we know we have used.
cc.AsSpan(0, length).Clear();
ArrayPool<char>.Shared.Return(cc, clearArray: false);
cc = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line necessary? Isn't cc about to go out of scope anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't strictly necessary in the current configuration. Imagine a future refactoring which hoists cc to an outer scope, with this in place it'll be cleared correctly in future. It costs very little.

}
}
else
{
Expand Down Expand Up @@ -12512,8 +12522,9 @@ private bool TryReadPlpUnicodeCharsChunk(char[] buff, int offst, int len, TdsPar
internal int ReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserStateObject stateObj)
{
int charsRead;
bool rentedBuff = false;
Debug.Assert(stateObj._syncOverAsync, "Should not attempt pends in a synchronous call");
bool result = TryReadPlpUnicodeChars(ref buff, offst, len, stateObj, out charsRead);
bool result = TryReadPlpUnicodeChars(ref buff, offst, len, stateObj, out charsRead, supportRentedBuff: false, ref rentedBuff);
if (!result)
{
throw SQL.SynchronousCallMayNotPend();
Expand All @@ -12525,7 +12536,7 @@ internal int ReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserS
// requested length is -1 or larger than the actual length of data. First call to this method
// should be preceeded by a call to ReadPlpLength or ReadDataLength.
// Returns the actual chars read.
internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserStateObject stateObj, out int totalCharsRead)
internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsParserStateObject stateObj, out int totalCharsRead, bool supportRentedBuff, ref bool rentedBuff)
{
int charsRead = 0;
int charsLeft = 0;
Expand All @@ -12538,16 +12549,26 @@ internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsPar
return true; // No data
}

Debug.Assert(((ulong)stateObj._longlen != TdsEnums.SQL_PLP_NULL),
"Out of sync plp read request");
Debug.Assert(((ulong)stateObj._longlen != TdsEnums.SQL_PLP_NULL),"Out of sync plp read request");

Debug.Assert((buff == null && offst == 0) || (buff.Length >= offst + len), "Invalid length sent to ReadPlpUnicodeChars()!");
charsLeft = len;

// If total length is known up front, allocate the whole buffer in one shot instead of realloc'ing and copying over each time
if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN)
// If total length is known up front, the length isn't specified as unknown
// and the caller doesn't pass int.max/2 indicating that it doesn't know the length
// allocate the whole buffer in one shot instead of realloc'ing and copying over each time
if (buff == null && stateObj._longlen != TdsEnums.SQL_PLP_UNKNOWNLEN && len < (int.MaxValue >> 1))
{
buff = new char[(int)Math.Min((int)stateObj._longlen, len)];
if (supportRentedBuff && len < 1073741824) // 1 Gib
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there ever a case where the buffer will be so short that renting won't be worthwhile?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you determine the answer to this question? Is it ever cheaper to new and GC an array than the overhead of renting? Possibly, I'd expect it to be a very low difference and almost impossible to quantify without a specific weird use case that is pathalogical along that route.

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding from other articles/comments about ArrayPool is that it isn't necessarily worthwhile for small arrays (on my machine it takes ~3x longer for arrays of length 10 for example).

Not sure if that matters here or not. You're probably correct that it won't be the bottleneck.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In terms of speed you're right. Speed isn't really a limiting factor for this library at the moment, network delay is. In general this will has a small speed penalty that would be quantifiable in a microbenchmark but very unlikely to become a bottleneck, it'll have an overall improvement in reducing GC'ed objects which will allow more time to be spend in user code then GC.

If a pathalogical case turns up then we can determine a more complex solution to avoid it.

{
buff = ArrayPool<char>.Shared.Rent((int)Math.Min((int)stateObj._longlen, len));
rentedBuff = true;
}
else
{
buff = new char[(int)Math.Min((int)stateObj._longlen, len)];
rentedBuff = false;
}
}

if (stateObj._longlenleft == 0)
Expand All @@ -12572,11 +12593,26 @@ internal bool TryReadPlpUnicodeChars(ref char[] buff, int offst, int len, TdsPar
charsRead = (int)Math.Min((stateObj._longlenleft + 1) >> 1, (ulong)charsLeft);
if ((buff == null) || (buff.Length < (offst + charsRead)))
{
// Grow the array
newbuf = new char[offst + charsRead];
bool returnRentedBufferAfterCopy = rentedBuff;
if (supportRentedBuff && (offst + charsRead) < 1073741824) // 1 Gib
{
newbuf = ArrayPool<char>.Shared.Rent(offst + charsRead);
rentedBuff = true;
}
else
{
newbuf = new char[offst + charsRead];
rentedBuff = false;
}

if (buff != null)
{
Buffer.BlockCopy(buff, 0, newbuf, 0, offst * 2);
if (returnRentedBufferAfterCopy)
{
buff.AsSpan(0, offst).Clear();
ArrayPool<char>.Shared.Return(buff, clearArray: false);
}
}
buff = newbuf;
}
Expand Down