Make array rental return after TryReadPlpUnicodeChars unconditional

[Wraith2]

fixes #2120

In PR #1866 I introduced the ability for TryReadPlpUnicodeChars to use rented buffers to store the intermediate char[]'s that are needed before a string can be created. That change works well in sync calls but has a problem in async calls.

When used in async mode with a string that spans multiple packets the async replay mechanism causes the TryReadPlpUnicodeChars to be called repeatedly until it has all the data available in the buffered packets. An oversight in the method means that in the fail case a rented buffer is dropped to the GC. The rented buffers' size converge on the size of the requested string which means that as more packets are buffered into the snapshot larger and larger arrays will be allocated through the arraypool and then not returned.
This leads to performance which is no better than using newly allocated arrays and which contributes GC pressure (through the arrays it is dropping being on the LOH) leading to Gen2 GC which causes arraypools to be cleaned up which works against the use of rented buffers.

This change causes the caller of TryReadPlpUnicodeChars to return the rented array on failed paths as well as successful ones.

The test program is derived from the one provided in the issue. It uses the profiler api to get accurate results for small periods where trying to manually getting time snapshot would be error prone:

using Microsoft.EntityFrameworkCore;
using MsSqlDriverBug;

using (var context = new GarbageDbContext())
{
    JetBrains.Profiler.Api.MemoryProfiler.GetSnapshot();

    await context.Garbages.AsNoTracking().FirstOrDefaultAsync();

    JetBrains.Profiler.Api.MemoryProfiler.GetSnapshot();

    for (int i = 0; i < 10; i++)
    {
        await context.Garbages.AsNoTracking().FirstOrDefaultAsync(x => x.ID == i);
    }

    JetBrains.Profiler.Api.MemoryProfiler.GetSnapshot();
}

before this change:

after:

The GC time and run frequency are vastly reduced. This makes the overall repro faster.
The change is very simple. The difficulty was in finding and understanding it.

/cc @Havunen , @roji

[Havunen]

I tested this change in our application and the network request response times (end to end API calls) have improved a lot!

BEFORE (Microsoft.Data.SqlClient v5.1.1)

AFTER (Wraith2:fix-async-readsqlstring-rentals) -branch

Awesome work @Wraith2 !

This change deserves a release!

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.12% ⚠️

Comparison is base (930133e) 70.76% compared to head (8323175) 70.65%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2121      +/-   ##
==========================================
- Coverage   70.76%   70.65%   -0.12%     
==========================================
  Files         306      306              
  Lines       62051    62053       +2     
==========================================
- Hits        43911    43841      -70     
- Misses      18140    18212      +72     

Flag	Coverage Δ
addons	92.88% <ø> (ø)
netcore	73.42% <100.00%> (-0.13%)	⬇️
netfx	69.28% <ø> (-0.07%)	⬇️

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

Files Changed	Coverage Δ
.../netcore/src/Microsoft/Data/SqlClient/TdsParser.cs	68.18% <100.00%> (-0.50%)	⬇️

... and 12 files with indirect coverage changes

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

[David-Engel]

Nice! LGTM.

Thanks, Wraith!