Rent byte buffers in SqlSequentialTextReader

[TrayanZapryanov]

While profiling our code I saw a lot of allocations when using reader.GetTextReader() over big string column when reader is opened with CommandBehavior.SequentialAccess.

This PR tries to fix this by renting byte[] from shared pool.

[codecov]

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 72.58%. Comparing base (ca5e3e8) to head (6e93c9b).
Report is 34 commits behind head on main.

Files	Patch %	Lines
...icrosoft/Data/SqlClient/SqlSequentialTextReader.cs	81.25%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2356      +/-   ##
==========================================
- Coverage   72.64%   72.58%   -0.06%     
==========================================
  Files         310      310              
  Lines       61875    61881       +6     
==========================================
- Hits        44949    44918      -31     
- Misses      16926    16963      +37     

Flag	Coverage Δ
addons	92.88% <ø> (ø)
netcore	76.78% <81.25%> (-0.07%)	⬇️
netfx	70.16% <81.25%> (-0.07%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[JRahnama]

failures seems related to changes:

[xUnit.net 00:01:02.24]     Microsoft.Data.SqlClient.ManualTesting.Tests.DataStreamTest.RunAllTestsForSingleServer_NP [FAIL]
  [xUnit.net 00:01:02.25]       System.ArgumentException : Offset and length were out of bounds for the array or count is greater than the number of elements from index to the end of the source collection.
  [xUnit.net 00:01:02.25]       Stack Trace:
  [xUnit.net 00:01:02.25]            at System.Buffer.BlockCopy(Array src, Int32 srcOffset, Array dst, Int32 dstOffset, Int32 count)
  [xUnit.net 00:01:02.25]         /_/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialTextReader.cs(410,0): at Microsoft.Data.SqlClient.SqlSequentialTextReader.DecodeBytesToChars(Byte[] inBuffer, Int32 inBufferCount, Char[] outBuffer, Int32 outBufferOffset, Int32 outBufferCount)
  [xUnit.net 00:01:02.25]         /_/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialTextReader.cs(323,0): at Microsoft.Data.SqlClient.SqlSequentialTextReader.InternalRead(Char[] buffer, Int32 index, Int32 count)
  [xUnit.net 00:01:02.25]         /_/src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlSequentialTextReader.cs(88,0): at Microsoft.Data.SqlClient.SqlSequentialTextReader.Read()
  [xUnit.net 00:01:02.25]         /_/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs(1699,0): at Microsoft.Data.SqlClient.ManualTesting.Tests.DataStreamTest.ReadTextReader(String connectionString)
  [xUnit.net 00:01:02.25]         /_/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs(178,0): at Microsoft.Data.SqlClient.ManualTesting.Tests.DataStreamTest.RunAllTestsForSingleServer(String connectionString, Boolean usingNamePipes)
  [xUnit.net 00:01:02.25]         /_/src/Microsoft.Data.SqlClient/tests/ManualTests/SQL/DataStreamTest/DataStreamTest.cs(30,0): at Microsoft.Data.SqlClient.ManualTesting.Tests.DataStreamTest.RunAllTestsForSingleServer_NP()

[TrayanZapryanov]

Yes, I saw it and I have idea why. Rent method might return bigger array and calculations have to be changed to not use array limit. I will change it tomorrow.

[Wraith2]

The flow between the three methods is confusing but the code seems right overall. Can you remove the use of array.Empty? If you do that then you'll know that any non-null array is rented and can be returned, at the moment there could be way to try and return array.Empty to a pool which would either be bad or an exception.

In terms of style can you add braces to the single line conditionals please.

[TrayanZapryanov]

@Wraith2 I think empty array is used to progress reader only.
Check comment for 0 byte reads .
It doesn't look safe to remove if - just a check when returning should be enough to not give it back to pool.

[TrayanZapryanov]

@Wraith2 Ready for review. Build and tests are green

[TrayanZapryanov]

@JRahnama Is this PR acceptable?

[David-Engel]

LGTM

[TrayanZapryanov]

When is expected this one to be merged?

[David-Engel]

When is expected this one to be merged?

Just waiting on a second reviewer to pick it up then we'll merge it.