Avoid extra casts in SocketAsyncContext

[benaadams]

Store ManualResetEventSlim and Actions as strongly defined types.

They are pooled so isn't much advantage for saving one pointer field in exchange for 2 class casts.

Removes them from 3 such paths for Json TE

/cc @adamsitnik @stephentoub

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[adamsitnik]

I like this idea! Let me run the benchmarks

[adamsitnik]

I've confirmed that it is giving a +-0.2% boost.

Thank you @benaadams !

[stephentoub]

I've confirmed that it is giving a +-0.2% boost.

I'm curious, how many runs does one do to "confirm" such an increase? Isn't that within normal variance on these benchmarks?

[stephentoub]

They are pooled

Don't we only pool for a few specific kinds of operations? And even then only at most one? Did this make concurrent accepts more expensive, for example? What about connects, receive froms, etc.?

[benaadams]

The cast in the Event property meant for async previously it would have a 4 level hierarchy check cast when checking Event == null in StartAsyncOperation, ProcessSyncEventOrGetAsyncEvent (Action<...> -> MulticastDelegate -> Delegate -> Object) as well as the cast to Action<...> for executing the callback; so 3 casts for async.

runtime/src/coreclr/src/System.Private.CoreLib/src/System/Runtime/CompilerServices/CastHelpers.cs

Lines 257 to 293 in 92760b5

	private static object? IsInstanceOfClass(void* toTypeHnd, object? obj)
	{
	if (obj == null || RuntimeHelpers.GetMethodTable(obj) == toTypeHnd)
	return obj;
	MethodTable* mt = RuntimeHelpers.GetMethodTable(obj)->ParentMethodTable;
	for (; ; )
	{
	if (mt == toTypeHnd)
	goto done;
	if (mt == null)
	break;
	mt = mt->ParentMethodTable;
	if (mt == toTypeHnd)
	goto done;
	if (mt == null)
	break;
	mt = mt->ParentMethodTable;
	if (mt == toTypeHnd)
	goto done;
	if (mt == null)
	break;
	mt = mt->ParentMethodTable;
	if (mt == toTypeHnd)
	goto done;
	if (mt == null)
	break;
	mt = mt->ParentMethodTable;
	}

For sync you'd still get a cast in ProcessSyncEventOrGetAsyncEvent (though it would early exit being true being the actual type ManualResetEventSlim)

[stephentoub]

I'm not arguing with the expense of the cast. I'm saying the justification for why it's ok to increase the objects by a field in size was incomplete. It may very well still be the right trade-off, but from the data presented thus far it's not clear.

[stephentoub]

(And from the PR being opened to merged was the five hours I was rudely sleeping over night ;-)

[adamsitnik]

I'm curious, how many runs does one do to "confirm" such an increase? Isn't that within normal variance on these benchmarks?

I've changed the way I run the benchmarks. Previously I had more things to test and I was typically measuring the base once per day and then just running the benchmarks for given change and comparing them. So it was sth like the "base of the day" vs given change.

A few days ago I've changed my program that runs the benchmarks for me and now it runs them always in pairs: the base and diff right after it and the differences are smaller. I also run them for all machines I have access to (5) and for 4 benchmarks (plaintext, json, fortunes, fortunes with dll from Shay that implements batching). In this particular case there were no regressions for the 40 samples I got (2x5x4), but a 0.02% gain and since it was very small I decided not to post the numbers and just merge it. I also remember this particular cast from many profiles and have been thinking about very similar PR for a while.

[adamsitnik]

Don't we only pool for a few specific kinds of operations? And even then only at most one? Did this make concurrent accepts more expensive, for example? What about connects, receive froms, etc.?

We pool the following types:

runtime/src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs

Lines 41 to 45 in c5b55f0

	private AcceptOperation? _cachedAcceptOperation;
	private BufferMemoryReceiveOperation? _cachedBufferMemoryReceiveOperation;
	private BufferListReceiveOperation? _cachedBufferListReceiveOperation;
	private BufferMemorySendOperation? _cachedBufferMemorySendOperation;
	private BufferListSendOperation? _cachedBufferListSendOperation;

The cache is not static. As far as I understand every socket has a dedicated async context with dedicated poolable operations and dedicated send and receive queues. Personally I am surprised that we cache so many types. From the limited TechEmpower perspective we reuse only the BufferMemoryReceiveOperation instance.

[stephentoub]

Personally I am surprised that we cache so many types. From the limited TechEmpower perspective we reuse only the BufferMemoryReceiveOperation instance.

The world doesn't revolve around TechEmpower ;-)

We pool the following types

Right, which means we don't pool for the others, yet this change also added a field to the others. On top of that, we only pool one instance (per Socket as you point out), so if for example a Socket is used as a listener and multiple accepts are always pending at a time, the majority of those will have also grown by a field. That's what I'm calling out, that the explanation for why this change is ok didn't take that into account, or at least didn't highlight it as a downside.

[tmds]

Could the cost of the cast be removed by using Unsafe.As, after doing an explicit type check for typeof(ManualResetEventSlim)?

[benaadams]

Could the cost of the cast be removed by using Unsafe.As, after doing an explicit type check for typeof(ManualResetEventSlim)?

No, because the type is 8 different action variant types, so there is nothing specific to cast to

[tmds]

Can't we cast to the appropriate Action variant?

public Action<int, byte[]?, int, SocketFlags, SocketError>? Callback
{
   get => CallbackOrEvent?.GetType() != typeof(ManualResetEventSlim) ? Unsafe.As<Action<int, byte[]?, int, SocketFlags, SocketError>>(CallbackOrEvent) : null;
   set => CallbackOrEvent = value;
}

[benaadams]

Ah, see what you are saying. Not sure if @stephentoub would be more comfortable with that?