[QUIC] Copy managed memory instead of pinning #72746

ManickaP · 2022-07-24T20:35:38Z

Copies data into native memory instead of just pinning so that we can unblock the pending ValueTask before SEND_COMPLETE.

Note that this is draft to see if it helps, I'm pretty certain there are better ways to do this. Feel free to suggest anything.

Also adds logging of S.N.Quic traces for server.

Contributes to #72739

ghost · 2022-07-24T20:35:54Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Copies data into native memory instead of just pinning so that we can unblock the pending ValueTask before SEND_COMPLETE.

Note that this is draft to see if it helps, I'm pretty certain there are better ways to do this. Feel free to suggest anything.

Also adds logging of S.N.Quic traces for server.

Fixes #72739

Author:	ManickaP
Assignees:	-
Labels:	`area-System.Net.Http`
Milestone:	-

ManickaP · 2022-07-24T20:36:11Z

/azp run runtime-libraries stress-http

azure-pipelines · 2022-07-24T20:36:19Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2022-07-25T08:22:09Z

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Copies data into native memory instead of just pinning so that we can unblock the pending ValueTask before SEND_COMPLETE.

Note that this is draft to see if it helps, I'm pretty certain there are better ways to do this. Feel free to suggest anything.

Also adds logging of S.N.Quic traces for server.

Fixes #72739

Author:	ManickaP
Assignees:	ManickaP
Labels:	`area-System.Net.Http`, `area-System.Net.Quic`
Milestone:	-

rzikm · 2022-07-25T08:39:09Z

src/libraries/System.Net.Quic/src/System/Net/Quic/Internal/MsQuicBuffers.cs

-        {
-            _handles = new MemoryHandle[count];
-            FreeNativeMemory();
-            _buffers = (QUIC_BUFFER*)NativeMemory.Alloc((nuint)count, (nuint)sizeof(QUIC_BUFFER));
-        }
-
+        _buffers = (QUIC_BUFFER*)NativeMemory.AllocZeroed((nuint)count, (nuint)sizeof(QUIC_BUFFER));


Before this change, Reserve allocated only if _buffers was too small to hold all count buffers. Now it allocates every time. Is it necessary? Is the previous _buffers array deallocated?

This is just wrong, it also needs to free in case it "reallocates". I'll fix it, thanks for catching this.

ManickaP · 2022-07-25T08:56:47Z

/azp run runtime-libraries stress-http

azure-pipelines · 2022-07-25T08:56:55Z

Azure Pipelines successfully started running 1 pipeline(s).

CarnaViire · 2022-08-01T20:15:57Z

I don't see any perf impact of this change (whaaaat 🤯) (I haven't run extensive benchmarks though, only some of them)

Some results

Get 8K (10 threads): *no regression* (~11K RPS on aspnet-perf-win)

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net7.0 --client.channel latest --server.framework net7.0 --server.channel latest --profile aspnet-perf-win --scenario httpclient-kestrel-get --variable useHttps=true --variable httpVersion="3.0" --variable responseSize=8192 --variable concurrencyPerHttpClient=10 --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 --client.options.outputFiles "<path-to-dll>\Release\net7.0-windows\System.Net.Quic.dll" --server.options.outputFiles "<path-to-dll>\Release\net7.0-linux\System.Net.Quic.dll"

Get 8K (1000 threads): *no regression* (~25K RPS)

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net7.0 --client.channel latest --server.framework net7.0 --server.channel latest --profile aspnet-perf-win --scenario httpclient-kestrel-get --variable useHttps=true --variable httpVersion="3.0" --variable responseSize=8192 --variable concurrencyPerHttpClient=1000 --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 --client.options.outputFiles "<path-to-dll>\Release\net7.0-windows\System.Net.Quic.dll" --server.options.outputFiles "<path-to-dll>\Release\net7.0-linux\System.Net.Quic.dll"

Post 8K (1000 threads): *no regression* (~26K RPS)

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net7.0 --client.channel latest --server.framework net7.0 --server.channel latest --profile aspnet-perf-win --scenario httpclient-kestrel-post --variable useHttps=true --variable httpVersion="3.0" --variable requestContentSize=8192 --variable concurrencyPerHttpClient=1000 --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 --client.options.outputFiles "<path-to-dll>\Release\net7.0-windows\System.Net.Quic.dll" --server.options.outputFiles "<path-to-dll>\Release\net7.0-linux\System.Net.Quic.dll"

Get 1M (1000 threads): *no regression* (~310 RPS)

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net7.0 --client.channel latest --server.framework net7.0 --server.channel latest --profile aspnet-perf-win --scenario httpclient-kestrel-get --variable useHttps=true --variable httpVersion="3.0" --variable responseSize=1048576 --variable concurrencyPerHttpClient=1000 --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 --client.options.outputFiles "<path-to-dll>\Release\net7.0-windows\System.Net.Quic.dll" --server.options.outputFiles "<path-to-dll>\Release\net7.0-linux\System.Net.Quic.dll"

Post 1M (1000 threads): *no regression* (~240 RPS)

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net7.0 --client.channel latest --server.framework net7.0 --server.channel latest --profile aspnet-perf-win --scenario httpclient-kestrel-post --variable useHttps=true --variable httpVersion="3.0" --variable requestContentSize=1048576 --variable requestContentWriteSize=1048576 --variable concurrencyPerHttpClient=1000 --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 --client.options.outputFiles "<path-to-dll>\Release\net7.0-windows\System.Net.Quic.dll" --server.options.outputFiles "<path-to-dll>\Release\net7.0-linux\System.Net.Quic.dll"

Post 1K 10-bytes-at-a-time (1000 threads): *no regression* (~4K RPS)

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net7.0 --client.channel latest --server.framework net7.0 --server.channel latest --profile aspnet-perf-win --scenario httpclient-kestrel-post --variable useHttps=true --variable httpVersion="3.0" --variable requestContentSize=1024 --variable requestContentWriteSize=10 --variable requestContentFlushAfterWrite=true --variable requestContentUnknownLength=true --variable concurrencyPerHttpClient=1000 --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 --client.options.outputFiles "<path-to-dll>\Release\net7.0-windows\System.Net.Quic.dll" --server.options.outputFiles "<path-to-dll>\Release\net7.0-linux\System.Net.Quic.dll"

Post 1M 1K-at-a-time (100 threads): *no regression* (~220 RPS)

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/main/scenarios/httpclient.benchmarks.yml --client.framework net7.0 --client.channel latest --server.framework net7.0 --server.channel latest --profile aspnet-perf-win --scenario httpclient-kestrel-post --variable useHttps=true --variable httpVersion="3.0" --variable requestContentSize=1048576 --variable requestContentWriteSize=1024 --variable requestContentFlushAfterWrite=true --variable requestContentUnknownLength=true --variable concurrencyPerHttpClient=100 --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 --client.options.outputFiles "<path-to-dll>\Release\net7.0-windows\System.Net.Quic.dll" --server.options.outputFiles "<path-to-dll>\Release\net7.0-linux\System.Net.Quic.dll"

Note: perf runs were fixed for --client.sdkVersion 7.0.100-rc.1.22381.2 --client.aspNetCoreVersion 7.0.0-rc.1.22401.1 --client.runtimeVersion 7.0.0-rc.1.22401.1 --server.sdkVersion 7.0.100-rc.1.22377.4 --server.aspNetCoreVersion 7.0.0-rc.1.22376.12 --server.runtimeVersion 7.0.0-rc.1.22401.1 because of #73177

P.S.: I can see the additional copying being present on the traces (so it's not like I somehow failed in substituting the dll) -- it takes ~22% of the time for "Post 1M (1000 threads)" scenario. Still the results above sound extremely strange to me.

ManickaP · 2022-08-02T17:29:15Z

If we're limited by msquic thread then I guess adding more work on user thread doesn't affect the overall result. Note that this change only frees one pointer on msquic thread, otherwise the copy happens within WriteAsync before calling msquic.

CarnaViire · 2022-08-02T18:43:01Z

@ManickaP FYI we just discussed on a meeting that we want this change to be merged for now while we are working on a "proper" fix (which we might not have in time for 7.0)

CarnaViire · 2022-08-03T12:18:31Z

/azp run runtime-libraries stress-http

azure-pipelines · 2022-08-03T12:18:40Z

Azure Pipelines successfully started running 1 pipeline(s).

CarnaViire · 2022-08-09T17:41:08Z

Stress failures are "Completed unexpectedly" from #72619

CarnaViire

LGTM

dotnet-issue-labeler bot added the area-System.Net.Http label Jul 24, 2022

ghost assigned ManickaP Jul 24, 2022

ManickaP added area-System.Net.Quic and removed area-System.Net.Http labels Jul 25, 2022

rzikm reviewed Jul 25, 2022

View reviewed changes

runfoapp bot mentioned this pull request Jul 25, 2022

CopyToNanosecondsPresent_LowTempRes #48717

Closed

Improved server logging to include S.N.Quic traces as well.

6d54852

ManickaP force-pushed the mapichov/quic-write-copy branch from 6e91d7e to 6d54852 Compare July 26, 2022 18:07

MsQuicBuffers copy given memory into native instead of pinning

59ff693

CarnaViire mentioned this pull request Aug 1, 2022

[HTTP/3] Elevated exception rate after Kestrel update #73177

Closed

CarnaViire marked this pull request as ready for review August 9, 2022 17:39

CarnaViire approved these changes Aug 9, 2022

View reviewed changes

CarnaViire merged commit 8e407e3 into dotnet:main Aug 9, 2022

CarnaViire mentioned this pull request Aug 9, 2022

[HTTP/3] Stress QUIC mangled data #72739

Closed

ManickaP deleted the mapichov/quic-write-copy branch August 9, 2022 18:51

This was referenced Aug 10, 2022

[QUIC] Abort on cancellation throws QUIC_STATUS_INVALID_PARAMETER #73688

Closed

[QUIC] Consider implementing send buffering on .NET side #73691

Closed

karelz added this to the 7.0.0 milestone Aug 15, 2022

ghost locked as resolved and limited conversation to collaborators Sep 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[QUIC] Copy managed memory instead of pinning #72746

[QUIC] Copy managed memory instead of pinning #72746

ManickaP commented Jul 24, 2022 •

edited by CarnaViire

Loading

ghost commented Jul 24, 2022

ManickaP commented Jul 24, 2022

azure-pipelines bot commented Jul 24, 2022

ghost commented Jul 25, 2022

rzikm Jul 25, 2022

ManickaP Jul 25, 2022

ManickaP commented Jul 25, 2022

azure-pipelines bot commented Jul 25, 2022

CarnaViire commented Aug 1, 2022

ManickaP commented Aug 2, 2022

CarnaViire commented Aug 2, 2022

CarnaViire commented Aug 3, 2022

azure-pipelines bot commented Aug 3, 2022

CarnaViire commented Aug 9, 2022

CarnaViire left a comment

[QUIC] Copy managed memory instead of pinning #72746

[QUIC] Copy managed memory instead of pinning #72746

Conversation

ManickaP commented Jul 24, 2022 • edited by CarnaViire Loading

ghost commented Jul 24, 2022

ManickaP commented Jul 24, 2022

azure-pipelines bot commented Jul 24, 2022

ghost commented Jul 25, 2022

rzikm Jul 25, 2022

Choose a reason for hiding this comment

ManickaP Jul 25, 2022

Choose a reason for hiding this comment

ManickaP commented Jul 25, 2022

azure-pipelines bot commented Jul 25, 2022

CarnaViire commented Aug 1, 2022

ManickaP commented Aug 2, 2022

CarnaViire commented Aug 2, 2022

CarnaViire commented Aug 3, 2022

azure-pipelines bot commented Aug 3, 2022

CarnaViire commented Aug 9, 2022

CarnaViire left a comment

Choose a reason for hiding this comment

ManickaP commented Jul 24, 2022 •

edited by CarnaViire

Loading