Elide repeated Action-delegate allocation by caching it #1324

gfoidl · 2022-01-04T14:04:54Z

Here C# compiler has too little knowledge about the usage of the delegate, we know better so cache it on our own to avoid the repeated allocation.

From the profiling session in #1323 we see that there are ~100.000 allocations of the Action-delegate. They come from cancellationToken.Register and the C# compiler can't cache the method-group here.

With the change from this PR these allocations are gone. The following images shows the changes of #1323 combined with this one:

So for the client publishing use case only the allocations for tasks, the publish packet, and the publish result show up.
To avoid these allocations I fear some bigger refactorings are needed, and thats a different story...

Here C# compiler has too little knowledge about the usage of the delegate, we know better so cache it on our own to avoid the repeated allocation.

gfoidl · 2022-01-04T14:07:05Z

Source/MQTTnet/Implementations/MqttTcpChannel.cs

@@ -150,7 +156,7 @@ public async Task<int> ReadAsync(byte[] buffer, int offset, int count, Cancellat
                }

                // Workaround for: https://github.com/dotnet/corefx/issues/24430


Question: starting with .NET Core 3.0 this workaround isn't needed anymore, so the code could be conditionally compiled for a "legacy" target and for new targets.

W/o the workaround perf in general should be better, as the cost of creating, and disposing the cancellationtoken-registration can be avoided.

If it is fixed we should add a compiler switch (NETCOREAPP3_0_OR_GREATER).

chkr1011 · 2022-01-04T18:33:58Z

Source/MQTTnet/Implementations/MqttTcpChannel.cs

@@ -150,7 +156,7 @@ public async Task<int> ReadAsync(byte[] buffer, int offset, int count, Cancellat
                }

                // Workaround for: https://github.com/dotnet/corefx/issues/24430
-                using (cancellationToken.Register(Dispose))


In the above code I agree that the lambda is not required but here we already have a method without wrapping it. Do we really need that optimization here as well?

From my profiling it showed up in WriteAsync (L187), so it's just duplicated here.

But when the conditional compilation is added for .NET Core 3.0 and newer, this won't be hit. So for the legacy code we could

keep the state of master-branch

keep the cached delegate of PR's current state

I'm leaning to the latter, as the change isn't huge and so older targets can benefit from that change too.

Ah, now I see your concern. Yes we need it. Whether it's a delegate created from a method group or a lambda, the delegate won't be cached. Checked the IL for both.

OK then we should keep it.

chkr1011 · 2022-01-05T16:31:03Z

Source/MQTTnet/Implementations/CrossPlatformSocket.cs

@@ -112,7 +118,7 @@ public async Task ConnectAsync(string host, int port, CancellationToken cancella
                _networkStream?.Dispose();

                // Workaround for: https://github.com/dotnet/corefx/issues/24430
-                using (cancellationToken.Register(() => _socket.Dispose()))


Is the bug also fixed in this case so adding the pragma here works as well?

Socket.ConnectAsync(string, int, CancellationToken) exists only starting from .NET 6.
So for the moment we have to stay at the current code.

chkr1011 · 2022-01-08T12:56:12Z

Thank you for your contribution. Good work 👍

commit 27592f1 Author: Christian <[email protected]> Date: Sat Jan 8 15:36:35 2022 +0100 Update MQTTnet.nuspec commit c6f4b59 Author: Christian <[email protected]> Date: Sat Jan 8 15:35:47 2022 +0100 Increase delay for Keep Alive checks to decrease CPU load. commit b362b2a Author: Christian <[email protected]> Date: Sat Jan 8 14:35:25 2022 +0100 Make logger parameters generic. commit 7a72a8d Author: Christian <[email protected]> Date: Sat Jan 8 14:07:57 2022 +0100 Update MQTTnet.nuspec commit bd40e10 Author: Günther Foidl <[email protected]> Date: Sat Jan 8 14:00:39 2022 +0100 Elide repeated Action-delegate allocation by caching it (#1324) * Elide repeated Action-delegate allocation by caching it Here C# compiler has too little knowledge about the usage of the delegate, we know better so cache it on our own to avoid the repeated allocation. * Use conditional compilation commit 4b44a2b Author: Christian <[email protected]> Date: Fri Jan 7 18:01:03 2022 +0100 Remove appveyor. commit 215f24a Author: Christian <[email protected]> Date: Fri Jan 7 17:52:16 2022 +0100 Create CODE-OF-CONDUCT.md

yyjdelete · 2022-01-13T03:31:18Z

@gfoidl
Maybe it's better to use static Action instead of instance ones, and pass _socket or this as state for CancellationToken.Register()

static readonly Action<object> _socketDisposeAction = state => ((Socket)state).Dispose();

using (cancellationToken.Register(_socketDisposeAction, _socket))

gfoidl · 2022-01-14T19:04:31Z

@yyjdelete that's another way to do it, but I don't know if it is actually "better".
Due the static field the instance gets smaller a few bytes, but these objects are intended to live longer, so it won't matter much.
Calling a static delegate from an instance method has more runtime overhead (due to shuffle thunks that the runtime needs to create), which won't matter here too (could have an effect on (very) hot code-pathes).

So for the use here it could be done either way. In this PR I just the most easy to write and read way.
But I'm open to change it to your suggestion, leaving the decision to up to others (or you open a PR and it can be discussed further over there).

Elide repeated Action-delegate allocation by caching it

b7e92e1

Here C# compiler has too little knowledge about the usage of the delegate, we know better so cache it on our own to avoid the repeated allocation.

gfoidl commented Jan 4, 2022

View reviewed changes

gfoidl mentioned this pull request Jan 4, 2022

[Discussion] Cache instances of MqttClientPublishResult #1325

Closed

chkr1011 requested changes Jan 4, 2022

View reviewed changes

Use conditional compilation

74e34e3

chkr1011 requested changes Jan 7, 2022

View reviewed changes

chkr1011 approved these changes Jan 8, 2022

View reviewed changes

chkr1011 merged commit bd40e10 into dotnet:master Jan 8, 2022

gfoidl deleted the elide-action-allocation branch January 9, 2022 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Elide repeated Action-delegate allocation by caching it #1324

Elide repeated Action-delegate allocation by caching it #1324

gfoidl commented Jan 4, 2022

gfoidl Jan 4, 2022

chkr1011 Jan 4, 2022

chkr1011 Jan 4, 2022

gfoidl Jan 4, 2022

gfoidl Jan 4, 2022

chkr1011 Jan 5, 2022

chkr1011 Jan 5, 2022

gfoidl Jan 7, 2022

chkr1011 Jan 8, 2022

chkr1011 commented Jan 8, 2022

yyjdelete commented Jan 13, 2022

gfoidl commented Jan 14, 2022

		@@ -150,7 +156,7 @@ public async Task<int> ReadAsync(byte[] buffer, int offset, int count, Cancellat
		}

		// Workaround for: https://github.com/dotnet/corefx/issues/24430

Elide repeated Action-delegate allocation by caching it #1324

Elide repeated Action-delegate allocation by caching it #1324

Conversation

gfoidl commented Jan 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chkr1011 commented Jan 8, 2022

yyjdelete commented Jan 13, 2022

gfoidl commented Jan 14, 2022