API Proposal: Expose SocketAsyncEventArgs constructor for suppressing the execution context

[davidfowl]

This constructor should be exposed to give callers the control over not capturing and restoring the execution context per operation.

https://github.com/dotnet/corefx/blob/788690a86f37a9b70907d002a367f57fca8ba423/src/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEventArgs.cs#L95

protected SocketAsyncEventArgs(bool flowExecutionContext)
{
}

cc @geoffkizer @stephentoub

[davidfowl]

@karelz why is this in future instead of 3.0? It’s trivial to do.

[karelz]

@davidfowl trivial != critical / high-value. 3.0 milestone means it is something we are fairly sure we can make happen or it is critical for that milestone and we should make it happen.
Future milestone does not mean it cannot happen earlier. It just doesn't make any promises on when it is going to happen. Therefore Future milestone is default for most bugs and API additions without clear high-impact. In Networking area, we are not yet in phase to mark nice-to-have things as 3.0 given the number of 3.0 bugs we have.

If there is impact/value information missing above, please add it - that will help us prioritize it properly.

[davidfowl]

High level goal of mine is to reduce the amount of places we're capturing the restoring the ExecutionContext unnecessarily.

I don't mind contributing this change as long as we agree on the API (which is why this is an API proposal). I'd like to avoid the additional overhead we currently have using SocketAsyncEventArgs in Kestrel similar to how AwaitableSocketAsyncEventArgs does.

I'm currently trying to do this today aspnet/KestrelHttpServer#2974 but I need to measure it, it add unnecessary overhead since I have to suppress the flow per-operation instead of doing it once at construction time.

[stephentoub]

I'd like to avoid the additional overhead we currently have using SocketAsyncEventArgs in Kestrel

Can we measure what the actual impact is? I'd be fine with an approximation just by changing the internal use of this ctor to always pass true, and then seeing how that affects a microbenchmark using the send/receive methods that would otherwise have passed false. I should have gotten concrete measurements for this when I initially made that change internally in corefx (I may have locally, I don't remember, but I didn't include them in the PR details), but the burden of proof was also lower there as no new surface area was being exposed.

If it makes a statistically measurable difference, I'm fine with it being exposed, subject to normal API review/tweaks.

[stephentoub]

ps It'd also be great if Kestrel didn't need its own SAEA usage and could just use the ValueTask-based methods on Sockets. What would it take to make that happen instead, in terms of e.g. other missing APIs or perf gaps?

[davidfowl]

an we measure what the actual impact is? I'd be fine with an approximation just by changing the internal use of this ctor to always pass true, and then seeing how that affects a microbenchmark using the send/receive methods that would otherwise have passed false. I should have gotten concrete measurements for this when I initially made that change internally in corefx (I may have locally, I don't remember, but I didn't include them in the PR details), but the burden of proof was also lower there as no new surface area was being exposed.

If it makes a statistically measurable difference, I'm fine with it being exposed, subject to normal API review/tweaks.

Some performance results are in the PR (it's the PlainTextPlatform). It's improvement and it's repeatable. I ran it multiple times.

ps It'd also be great if Kestrel didn't need its own SAEA usage and could just use the ValueTask-based methods on Sockets. What would it take to make that happen instead, in terms of e.g. other missing APIs or perf gaps?

Yep, that's the first thing I looked at actually and it might be something that we do in 3.0. There's extra overhead because we want to control how continuations are scheduled and we bake that into the awaiter that we have. Using the Task based overloads means we'd have to use a custom task scheduler (which adds overhead) or SyncContext to avoid the extra hop. What we have right now is simpler.

[davidfowl]

Blah, I'm running more performance tests what's there now might be noise.

[davidfowl]

OK latest results here aspnet/KestrelHttpServer#2974 (comment)

[benaadams]

Just came across this as the source of unexpected ExecutionContext Runs I was seeing.

As this flag is currently used via inheritance (for types in corefx) and Kestrel's version is also inherited; would it be better to start with changing it from internal to protected?

protected SocketAsyncEventArgs(bool flowExecutionContext)
{
}

A user could then inherit to suppress and create the derived type; but it wouldn't be as public an api for something that is "unsafe".

Is showing up both via ExecutionContext.Capture

And via ExecutionContext.Run + Thread.GetCurrentThread

When the next action that Kestrel Socket Transport does with the data returned from the callback is to trigger Pipelines which has its own capturing of EC, SynCtx, TaskScheduler, so its next step is to call UnsafeQueueUserWorkItem ditching the context that has been saved and restored by SocketAsyncEventArgs as it uses its own (similarly to many of the derived types in corefx)

[stephentoub]

api-ready-for-review

The latest data that was shared above states (in what was linked to):
"Blah, I'm running more performance tests what's there now might be noise."
"Might be a wash."
That's not particularly conclusive for exposing new API entirely about improving performance. 😉

I'm actually not against the idea of exposing it, but if it's decided it's actually needed, we need to think through how to do so. Every other place I can think of where we suppress ExecutionContext flow is done with "UnsafeXx" methods. Obviously that can't be done with a ctor, but if we switched to a factory pattern, then it would prevent the derivation case that's presumably more important. That then suggests maybe a protected ctor is best for this, as Ben suggested in his comment above.

I'm having a hard time getting excited about this, but if others feel it's important I wouldn't stand in its way; obviously we felt it was worth using it ourselves, albeit as a microoptimization (and the equation is different when considering what's done as an implementation detail internally vs what's exposed publicly).

[benaadams]

That's not particularly conclusive for exposing new API entirely about improving performance. 😉

The example change while removing the EC restore when it returns, does add an EC suppress + restore when it starts; so while its removing work its also adding work.

Whereas the api change; I assume, would just be passing a bool which would drop the capture and restore, so only remove work?

[davidfowl]

The latest data that was shared above states (in what was linked to):
"Blah, I'm running more performance tests what's there now might be noise."
"Might be a wash."
That's not particularly conclusive for exposing new API entirely about improving performance.

Yea, testing this change was hard and I never got conclusive data about whether it moves the needle. It does do unnecessary work and that still bugs me. Hopefully we'll be in a better place to get data soon.

[terrajobst]

• The scenario makes sense, but we'd prefer if we'd use an enum so that we can stick the word unsafe somewhere, for example:

public enum Socket_Please_Give_Me_A_Name
{
    FlowExecutionContext = 0,
    UnsafeDoNotFlowExecutionContext = 1
}

public SocketAsyncEventArgs(Socket_Please_Give_Me_A_Name someName)
{
}

[scalablecory]

triage: we'd like to change the ctor to protected, without exposing a factory or enum.

New proposal:

protected SocketAsyncEventArgs(bool flowExecutionContext)
{
}

[terrajobst]

• We want false to be the safe default
• We should follow existing naming conventions which includes unsafe

public class SocketAsyncEventArgs
{
    public SocketAsyncEventArgs(bool unsafeSuppressExecutionContextFlow);
}

[MarcoRossignoli]

Waiting CI re-run and merge #706