Add array rental capability in TryReadPlpUnicodeChars #1866
Conversation
Codecov ReportBase: 70.71% // Head: 70.21% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1866 +/- ##
==========================================
- Coverage 70.71% 70.21% -0.50%
==========================================
Files 292 292
Lines 61727 61745 +18
==========================================
- Hits 43648 43355 -293
- Misses 18079 18390 +311
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/TdsParser.cs
Outdated
Show resolved
Hide resolved
| { | ||
| buff = new char[(int)Math.Min((int)stateObj._longlen, len)]; | ||
| if (supportRentedBuff && len < 1073741824) // 1 Gib |
There was a problem hiding this comment.
Is there ever a case where the buffer will be so short that renting won't be worthwhile?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // clear only the length that we know we have used. | ||
| cc.AsSpan(0, length).Clear(); | ||
| ArrayPool<char>.Shared.Return(cc, clearArray: false); | ||
| cc = null; |
There was a problem hiding this comment.
Is this line necessary? Isn't cc about to go out of scope anyway?
There was a problem hiding this comment.
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.
fixes #1864
This changes TryReadPlpUnicodeChars in two ways.
The outcome of this is that in general use the arraypool will be used no intermediate buffers will be dropped to the GC. This does some with a minor performance loss in CPU time spend dealing with the array pool rental mechanics and clearing. See the fixed thread for the details on that. The perf loss is small enough that I think #1544 will probably make up for it.
/cc @masonwheeler