Add SocketTransportOption to enable/disable WaitForData

[tmds]

This allows to opt-out of the zero-byte read that is performed to reduce memory usage for idle connections.
This read has a measurable impact on TE JSON benchmark.

cc @halter73 @davidfowl @sebastienros @adamsitnik

[adamsitnik]

LGTM.

I confirm that making this optional can improve throughput and latency. Moreover, it's mandatory to make batching with Linux AIO work (it does not support 0 bytes reads)

[Tratcher]

The ref assemblies need updating.
https://github.com/dotnet/aspnetcore/blob/master/docs/ReferenceAssemblies.md

What are the actual benchmark numbers?

[Tratcher]

Setting this to false can increase throughput at the cost of increased memory usage.

[tmds]

What are the actual benchmark numbers?

Can we run a benchmark using the Citrine infrastructure?

Should I make a change to some files to make the benchmark run with WaitForDataBeforeAllocatingBuffer=false?

[davidfowl]

/azp run pr-benchmark

[azure-pipelines]

No pipelines are associated with this pull request.

[analogrelay]

FYI, this will need API review (should be quick and easy though) as it adds a new public API.

[tmds]

I set the default to false to ensure that is what used by benchmarks.
I'll change it back to true before this is merged.

[adamsitnik]

Benchmark results using Citrine:

JSON

WaitForData enabled:

--jobs "..\Benchmarks\benchmarks.json.json" --scenario "Json"

RequestsPerSecond:           840,803
Max CPU (%):                 100
WorkingSet (MB):             417
Avg. Latency (ms):           0.86
Startup (ms):                504
First Request (ms):          44.64
Latency (ms):                0.11
Total Requests:              12,696,202
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             3,001
Published Size (KB):         26,065
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.2.20125.16
ASP.NET Core:                5.0.0-preview.2.20126.7

WaitForData disabled:

--jobs "..\Benchmarks\benchmarks.json.json" --scenario "Json"

RequestsPerSecond:           861,744
Max CPU (%):                 100
WorkingSet (MB):             436
Avg. Latency (ms):           0.83
Startup (ms):                303
First Request (ms):          51.51
Latency (ms):                0.1
Total Requests:              13,012,363
Duration: (ms)               15,100
Socket Errors:               0
Bad Responses:               0
Build Time (ms):             4,001
Published Size (KB):         119,145
SDK:                         5.0.100-preview.2.20120.3
Runtime:                     5.0.0-preview.2.20125.16
ASP.NET Core:                5.0.0-preview.2.20126.7

For Plaintext there is no visible difference - both settings give me 86XYk RPS

[sebastienros]

Did you set --self-contained on both runs, or use a custom build on each run to ensure that self-contained is implicitly set? If not then the displayed versions are misleading as the sdk's shared runtimes would prevail.

[adamsitnik]

@sebastienros For the disabled case, I've compiled this dll locally and sent it using --output-file option. For the enabled, I've not provided any extra arguments

[sebastienros]

In the case you don't provide any argument then you need to use --self-contained. I force it on all the CI runs, and it's also a default on the new driver. It might not change anything as we updated the sdk recently in the aspnet repos.

[tmds]

@sebastienros @halter73 where can we set WaitForDataBeforeAllocatingBuffer to false for the benchmarks?

[tmds]

@sebastienros @halter73 where can we set WaitForDataBeforeAllocatingBuffer to false for the benchmarks?

I guess it is in https://github.com/aspnet/Benchmarks once this change has rippled through.

@anurse @halter73 is this good to merge?

[halter73]

Normally our API reviews are on Monday, but it got canceled this week. I'll try to get it through next Monday. Sorry for the delay.

[ghost]

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

• The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
• The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
• Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

[sebastienros]

If the goal is only for benchmarking, you can do a local build that would use an ENV, or hard code the switch, and share the lib to use it instead.

[halter73]

This got approved via email, so no need to wait 😄 Thanks!

[benaadams]

As follow up to this call GetMemory() prior to calling FlushAsync() as otherwise there is contention on the Pipeline between reader and writer #22201