AnonymousPipeStreams.Linux, Process.Unix Streams: perform async I/O by using Socket implementation by tmds · Pull Request #44647 · dotnet/runtime

tmds · 2020-11-13T15:28:46Z

This change is to see if there are functional, or performance changes when using read/write instead of recvmsg/sendmsg.

By using read/write we could look into leveraging the existing Socket (and SocketAsyncEngine) for performing async operations on pipe file descriptors, see #44329.

@adamsitnik @sebastienros can you benchmark this using the techempower benchmarks?

cc @stephentoub @wfurt

ghost · 2020-11-13T15:28:51Z

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

Details

Issue Details

Description:	This change is to see if there are functional, or performance changes when using read/write instead of recvmsg/sendmsg. By using read/write we could look into leveraging the existing Socket (and SocketAsyncEngine) for performing async operations on pipe file descriptors, see #44329. @adamsitnik @sebastienros can you benchmark this using the techempower benchmarks? cc @stephentoub @wfurt
Author:	tmds
Assignees:	-
Labels:	`area-System.Net`
Milestone:	-

stephentoub · 2020-11-13T15:34:48Z

Nice. Thanks for trying this. Did you notice any impact in local microbenchmarks?

sebastienros · 2020-11-13T22:03:24Z

TE benchmarks are currently broken on net6.0. But I am trying hard to find why ...

adamsitnik · 2020-11-16T09:37:25Z

cc @geoffkizer

tmds · 2020-11-16T13:45:51Z

I worked on this further to the point where AnonymousPipeStream uses Socket under the hood.

I've added an bool IsPipe to SafeSocketHandle that tracks the handle is actually a pipe. I still need a way to make it non-public and be able to set it from System.IO.Pipes.

antonfirsov · 2020-11-16T18:07:45Z

/azp run runtime-libraries-coreclr outerloop

azure-pipelines · 2020-11-16T18:08:02Z

Azure Pipelines successfully started running 1 pipeline(s).

geoffkizer · 2020-11-16T18:12:57Z

Why not create new operation types in SocketAsyncContext? If you do that, you shouldn't have to touch the existing send/recv paths at all.

tmds · 2020-11-16T20:23:00Z

Why not create new operation types in SocketAsyncContext? If you do that, you shouldn't have to touch the existing send/recv paths at all.

I think it will result in code duplication until we meet the SafeSocketHandle.IsPipe check at a 'higher level'.

stephentoub · 2020-11-25T18:18:40Z

TE benchmarks are currently broken on net6.0. But I am trying hard to find why ...

@sebastienros, were you able to get these working again? Thanks!

sebastienros · 2020-11-25T19:07:45Z

It's working again (since yesterday).

crank --config https://raw.githubusercontent.com/aspnet/Benchmarks/master/scenarios/platform.benchmarks.yml --scenario json --profile aspnet-citrine-lin --application.framework net6.0

In case I can't find the time to build the PR, to use a custom dll, add:

--application.options.outputFiles c:\release\*.dll

adamsitnik · 2020-11-27T14:00:23Z

@tmds I've built your fork and run the JSON platform benchmarks in 4 configruations:

00 without your changes, DOTNET_SYSTEM_NET_SOCKETS_INLINE_COMPLETIONS=0
01 without your changes, DOTNET_SYSTEM_NET_SOCKETS_INLINE_COMPLETIONS=1
10 with your changes, DOTNET_SYSTEM_NET_SOCKETS_INLINE_COMPLETIONS=0
11 with your changes, DOTNET_SYSTEM_NET_SOCKETS_INLINE_COMPLETIONS=1

The results:

application
.NET Core SDK Version	6.0.100-alpha.1.20568.5
.NET Runtime Version	6.0.0-alpha.1.20577.3+17f51...
ASP.NET Core Version	6.0.0-alpha.1.20576.2+2c175...

load	00	10	01	11
CPU Usage (%)	74	74	77	77
Cores usage (%)	2,075	2,080	2,153	2,164
Working Set (MB)	48	48	48	48
Build Time (ms)	3,803	3,722	3,688	3,731
Start Time (ms)	0	0	0	0
Published Size (KB)	76,401	76,401	76,401	76,401
First Request (ms)	67	74	64	72
Requests/sec	1,187,853	1,189,332	1,235,426	1,242,720
Requests	17,936,371	17,957,897	18,654,912	18,764,486
Mean latency (ms)	0.67	0.66	0.42	0.41
Max latency (ms)	40.41	47.82	33.49	32.74
Bad responses	0	0	0	0
Socket errors	0	0	0	0
Read throughput (MB/s)	165.39	165.60	172.02	173.03
Latency 50th (ms)	0.39	0.39	0.40	0.40
Latency 75th (ms)	0.44	0.45	0.43	0.43
Latency 90th (ms)	0.54	0.53	0.46	0.46
Latency 99th (ms)	9.40	9.30	0.59	0.60

the trace files are available here

stephentoub · 2020-11-27T18:50:43Z

The results

Thanks for checking, @adamsitnik. That's good, that means this has basically zero impact, which is as good as we could hope for.

The only number that jumps out at me is the First Request ms value. Is that difference just noise, or is that meaningful?

tmds · 2020-12-03T09:46:00Z

Thanks for running those benchmarks Adam.

Since the benchmarks show no regression, I've worked out the implementation further. These new changes won't impact the benchmarks.

I'm struggling to make the Socket creation for AnonymousPipeStreams internal, and not have it removed by the linker.

There are a couple of ways this could be implemented. I've avoided the work and risk of refactoring SocketAsyncEngine out of System.Net.Sockets, and kept the handle (which is likely passed between different processes) as a pipe.

ghost · 2020-12-03T12:36:15Z

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

Issue Details

This change is to see if there are functional, or performance changes when using read/write instead of recvmsg/sendmsg.

By using read/write we could look into leveraging the existing Socket (and SocketAsyncEngine) for performing async operations on pipe file descriptors, see #44329.

@adamsitnik @sebastienros can you benchmark this using the techempower benchmarks?

cc @stephentoub @wfurt

Author:	tmds
Assignees:	-
Labels:	`area-System.Net.Sockets`
Milestone:	-

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs

davidfowl · 2021-01-29T15:09:31Z

Would it be better to move this polling into corelib and decouple it from sockets so other things can take advantage?

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs

stephentoub

Looking good. Thanks for working on this. Are there any cancellation-related tests we can enable now or add for pipe streams on unix?

stephentoub · 2021-03-04T03:27:08Z

@geoffkizer, can you take a look at the socket parts?

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

src/libraries/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.cs

geoffkizer · 2021-03-12T03:24:22Z

I don't have any major concerns -- a couple small issues above.

geoffkizer · 2021-03-15T20:20:07Z

My concerns have all been addressed -- @stephentoub is this ready to merge?

stephentoub · 2021-03-16T03:23:15Z

/azp run

azure-pipelines · 2021-03-16T03:23:20Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

stephentoub · 2021-03-16T03:23:32Z

/azp list

azure-pipelines · 2021-03-16T03:23:37Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr clrinterpreter runtime-coreclr crossgen2-composite gcstress runtime-libraries-mono outerloop runtime-staging

stephentoub · 2021-03-16T03:23:50Z

/azp run runtime

azure-pipelines · 2021-03-16T03:24:16Z

Azure Pipelines successfully started running 1 pipeline(s).

stephentoub · 2021-03-17T02:03:31Z

Thanks, @tmds!

benaadams · 2021-03-17T05:20:17Z

src/libraries/System.IO.Pipes/src/System/IO/Pipes/PipeStream.Windows.cs

+        {
+            if (_isAsync)
+            {
+                return ReadAsync(buffer, offset, count, CancellationToken.None).GetAwaiter().GetResult();


I know this was just copied from before; but why is this ReadAsync(...).GetAwaiter().GetResult() and Read(Span) is base.Read(buffer)?

Couldn't this also be

return base.Read(new Span(buffer, offset, count));

Or is the other one wrong?

@benaadams the Stream.Read(Span) method has an additional overhead of renting an array from the ArrayPool and copying the memory:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 659 to 677 in 0f64b26

public virtual int Read(Span<byte> buffer)

{

byte[] sharedBuffer = ArrayPool<byte>.Shared.Rent(buffer.Length);

try

{

int numRead = Read(sharedBuffer, 0, buffer.Length);

if ((uint)numRead > (uint)buffer.Length)

{

throw new IOException(SR.IO_StreamTooLong);

}

new ReadOnlySpan<byte>(sharedBuffer, 0, numRead).CopyTo(buffer);

return numRead;

}

finally

{

ArrayPool<byte>.Shared.Return(sharedBuffer);

}

}

this is why we use Read(Array) here and in few other places like FileStream

Ah, and then it calls into the above overload using the array

Dotnet-GitSync-Bot added the area-System.Net label Nov 13, 2020

tmds changed the title ~~[NO MERGE] experiment: use read/write for socket operations instead of recvmsg/sendmsg~~ [NO MERGE] experiment: use read/write for socket operations instead of recv/send Nov 13, 2020

runfoapp bot mentioned this pull request Nov 13, 2020

Inability to unzip assets during build on Unix x64 #32805

Closed

This comment has been minimized.

Sign in to view

tmds changed the title ~~[NO MERGE] experiment: use read/write for socket operations instead of recv/send~~ [NO MERGE] experiment: use Socket for async I/O on pipe Nov 16, 2020

tmds changed the title ~~[NO MERGE] experiment: use Socket for async I/O on pipe~~ AnonymousPipeStreams.Linux: perform async I/O by using Socket implementation Dec 3, 2020

karelz added area-System.Net.Sockets and removed area-System.Net labels Dec 3, 2020

tmds commented Dec 9, 2020

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.Unix.cs Outdated Show resolved Hide resolved

karelz assigned stephentoub Jan 12, 2021

stephentoub mentioned this pull request Jan 15, 2021

Same code starting wkhtmltopdf process works on netcoreapp3.1 but fails on net5.0 on Linux #46469

Closed

stephentoub mentioned this pull request Jan 25, 2021

Introduce FileStreamStrategy as a first step of FileStream rewrite #47128

Merged

eiriktsarpalis mentioned this pull request Jan 26, 2021

Revert "Use socketpair to implement Process.Start redirection" #47461

Merged

stephentoub reviewed Mar 4, 2021

View reviewed changes

src/libraries/System.Diagnostics.Process/src/System/Diagnostics/Process.Unix.cs Outdated Show resolved Hide resolved

stephentoub reviewed Mar 4, 2021

View reviewed changes

tmds added 2 commits March 4, 2021 10:36

PR feedback

f4e6480

PipeStream.Unix: enable cancelation tests

382323d

tmds force-pushed the socket_read_write branch from 10fa0a7 to 382323d Compare March 5, 2021 07:17

stephentoub requested a review from geoffkizer March 8, 2021 03:02

karelz assigned geoffkizer Mar 9, 2021

tmds changed the title ~~AnonymousPipeStreams.Linux: perform async I/O by using Socket implementation~~ AnonymousPipeStreams.Linux, Process.Unix Streams: perform async I/O by using Socket implementation Mar 10, 2021

geoffkizer reviewed Mar 12, 2021

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs Show resolved Hide resolved

geoffkizer reviewed Mar 12, 2021

View reviewed changes

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs Show resolved Hide resolved

geoffkizer reviewed Mar 12, 2021

View reviewed changes

src/libraries/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.cs Show resolved Hide resolved

geoffkizer reviewed Mar 12, 2021

View reviewed changes

src/libraries/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.cs Show resolved Hide resolved

tmds added 2 commits March 12, 2021 14:33

Add back Ctor_SafeHandle_Invalid_ThrowsException test

42a32da

SysRead/SysWrite: Assert not used for socket-type handle

94583f3

stephentoub approved these changes Mar 16, 2021

View reviewed changes

runfoapp bot mentioned this pull request Mar 16, 2021

Mono System.Text.Json.Tests on Windows timing out #42677

Open

stephentoub merged commit 2606d82 into dotnet:main Mar 17, 2021

benaadams reviewed Mar 17, 2021

View reviewed changes

ghost locked as resolved and limited conversation to collaborators Apr 16, 2021

karelz added this to the 6.0.0 milestone May 20, 2021

	public virtual int Read(Span<byte> buffer)
	{
	byte[] sharedBuffer = ArrayPool<byte>.Shared.Rent(buffer.Length);
	try
	{
	int numRead = Read(sharedBuffer, 0, buffer.Length);
	if ((uint)numRead > (uint)buffer.Length)
	{
	throw new IOException(SR.IO_StreamTooLong);
	}

	new ReadOnlySpan<byte>(sharedBuffer, 0, numRead).CopyTo(buffer);
	return numRead;
	}
	finally
	{
	ArrayPool<byte>.Shared.Return(sharedBuffer);
	}
	}

Conversation

tmds commented Nov 13, 2020

Uh oh!

ghost commented Nov 13, 2020

Uh oh!

stephentoub commented Nov 13, 2020

Uh oh!

sebastienros commented Nov 13, 2020

Uh oh!

adamsitnik commented Nov 16, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

tmds commented Nov 16, 2020

Uh oh!

antonfirsov commented Nov 16, 2020

Uh oh!

azure-pipelines bot commented Nov 16, 2020

Uh oh!

geoffkizer commented Nov 16, 2020

Uh oh!

tmds commented Nov 16, 2020

Uh oh!

stephentoub commented Nov 25, 2020

Uh oh!

sebastienros commented Nov 25, 2020

Uh oh!

adamsitnik commented Nov 27, 2020

Uh oh!

stephentoub commented Nov 27, 2020

Uh oh!

tmds commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 3, 2020

Uh oh!

Uh oh!

davidfowl commented Jan 29, 2021

Uh oh!

Uh oh!

stephentoub left a comment

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Mar 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

geoffkizer commented Mar 12, 2021

Uh oh!

geoffkizer commented Mar 15, 2021

Uh oh!

stephentoub commented Mar 16, 2021

Uh oh!

azure-pipelines bot commented Mar 16, 2021

Uh oh!

stephentoub commented Mar 16, 2021

Uh oh!

azure-pipelines bot commented Mar 16, 2021

Uh oh!

stephentoub commented Mar 16, 2021

Uh oh!

azure-pipelines bot commented Mar 16, 2021

Uh oh!

stephentoub commented Mar 17, 2021

Uh oh!

benaadams Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

adamsitnik Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

benaadams Mar 17, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

tmds commented Dec 3, 2020 •

edited

Loading