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.

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

Make array rental return after TryReadPlpUnicodeChars unconditional #2121

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

Wraith2
Copy link
Contributor

@Wraith2 Wraith2 commented Aug 12, 2023

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:
before

after:
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
Copy link

Havunen commented Aug 12, 2023

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)
before

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

Awesome work @Wraith2 !

This change deserves a release!

@codecov
Copy link

codecov bot commented Aug 12, 2023

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.

@JRahnama JRahnama added this to the 5.2.0-preview4 milestone Aug 13, 2023
@JRahnama JRahnama added the 📈 Performance Issues that are targeted to performance improvements. label Aug 13, 2023
Copy link
Contributor

@David-Engel David-Engel left a comment

Choose a reason for hiding this comment

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

Nice! LGTM.

Thanks, Wraith!

@David-Engel David-Engel merged commit 359c4ba into dotnet:main Aug 15, 2023
132 checks passed
@Wraith2 Wraith2 deleted the fix-async-readsqlstring-rentals branch August 15, 2023 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📈 Performance Issues that are targeted to performance improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Heavy GC thrashing in TdsParser
4 participants