search

Azure / azure-service-bus-dotnet

☁️ .NET Standard client library for Azure Service Bus
https://azure.microsoft.com/services/service-bus
Other
235 stars 120 forks source link

[SPIKE] Switch MessageHandlers to use ValueTask #611

Closed SeanFeldman closed 5 years ago

SeanFeldman commented 5 years ago

Experimental PR for issue #609 to see if switching MessageHandlers (message pumps) to ValueTask yields any performance benefits.

According to ValueTask documentation

the default choice for any asynchronous method should be to return a Task or Task<TResult>. Only if performance analysis proves it worthwhile should a ValueTask<TResult> be used instead of a Task<TResult>.

ghost commented 5 years ago

I have benched dev branch and PR-611, I've extracted the two .diagsession that I link to this post: https://www.dropbox.com/s/y35tanvdoybkbu3/dev-branch.diagsession?dl=0 https://www.dropbox.com/s/vqjezib4uart9s2/PR-611.diagsession?dl=0

I haven't time yet to compare both profiles but you should be able to do so.

I also made a project repro here: https://github.com/bbougot/PR611 You have only to create a Service Bus Queue and fill the two required values "QUEUE_NAME" and "Endpoint=" to the respective Azure Service Bus queue name and connection string.

Basically, it is made to send batch of 1000 messages every 100 milliseconds and by switching Task/ValueTask depending on the branch we are (dev/PR-611), you will be able to find the differences by profiling the memory using Visual Studio profiler.

Please also notice you may encounter a memory leak issue related to AMQP I've tracked here.

SeanFeldman commented 5 years ago

Doesn't seem like .diagsession are portable. I couldn't load those properly on two different computers. I'll run the sample locally to profile it.

SeanFeldman commented 5 years ago

@bbougot I played with the code a bit - don't really see a significant impact. Interestingly enough, ValueTask based version had slightly more Tasks allocations... Mind to share your results as text/screenshots?

SeanFeldman commented 5 years ago

Closing as it doesn't seem to be worthy of proceeding.