Reduce lock contention for frame writes (with extra tests) #354

michaelklishin · 2017-09-19T20:09:13Z

This is #350 by @YulerB and @vendre21 with some additional tests from our team. See #350 for the original discussion.

The idea is the following:

Frames and frame sets (for frames that carry content and need, ahem, content framing) are written in a single operation to a memory-backed stream
BufferedStream is no longer used (it was not efficiently used for writes anyway)
The above seeks to guarantee that socket stream writes cannot be interleaved in a way that will confuse RabbitMQ protocol parser
As such, locking is no longer necessary

This PR was created to simplify the final rounds of QA:

Extra tests if our team comes up with any
A fresh discussion without a dozen of comments about line endings :)

Details discussed on Google Groups https://groups.google.com/forum/?nomobile=true#!topic/rabbitmq-users/OyoYTkAyCSk

Updated code to remove lock contention

Updating to configure await on connectasync as per trunk

This reverts commit 3aecf44.

…ient into YulerB-master

bording · 2017-09-19T20:21:41Z

@michaelklishin I've been meaning to try out these changes, but haven't gotten to them yet.

The only way I can think to make trying it out easier would be for your to publish some sort of prerelease package, perhaps to a custom MyGet feed instead of nuget.org.

Did you see @danielmarbach's request on the other PR about adding tests with several different payload sizes? Seems like a good idea to validate this continues to be better as message size increases.

michaelklishin · 2017-09-19T20:53:15Z

@bording OK, we had an AppVeyor feed for development releases but I'm afraid that thing hasn't been updated in a while.

I am going to add more tests with different payload sizes, very good idea @danielmarbach. Any particular values you have in mind?

michaelklishin · 2017-09-20T04:41:16Z

I did some benchmarking with 20 runs of the concurrent connection access test suite on two machines (from 2017 and 2013).

In one case, median went down from 1.35s (master) to 1.04s (this PR). In the other, from 1.41s to 1.06s. While that's not a lot of data points, on a CPU (32 threads, together with RabbitMQ completely max out 8 hyperthreads on both machines) + loopback I/O heavy workload, removing the lock makes a nice difference.

bording · 2017-09-20T23:47:33Z

@michaelklishin That sounds really good! I spent some time today getting some of my testing setup ready to try out the changes here, so I should have something to report back tomorrow.

kjnilsson

Ok this generally looks good. Just a few formatting, tidy up changes. Naturally it is almost impossible to say this is definitely safe but it looks ok to me.

kjnilsson · 2017-09-21T08:36:35Z

projects/client/RabbitMQ.Client/src/client/impl/Connection.cs

@@ -1099,7 +1099,7 @@ public void HeartbeatWriteTimerCallback(object state)
                    if (!m_closed)
                    {
                        WriteFrame(m_heartbeatFrame);
-                        m_frameHandler.Flush();
+                        //m_frameHandler.Flush();


remove commented code

kjnilsson · 2017-09-21T08:37:56Z

projects/client/RabbitMQ.Client/src/client/impl/ModelBase.cs

@@ -465,11 +464,11 @@ public void ModelSend(MethodBase method, ContentHeaderBase header, byte[] body)
        {
            if (method.HasContent)
            {
-                lock (m_flowSendLock)
-                {
+                //lock (m_flowSendLock)


remove commented code

kjnilsson · 2017-09-21T08:38:23Z

projects/client/RabbitMQ.Client/src/client/impl/ModelBase.cs

@@ -908,11 +912,11 @@ public void HandleChannelFlow(bool active)
            }
            else
            {
-                lock (m_flowSendLock)
-                {
+                //lock (m_flowSendLock)


remove commented code

kjnilsson · 2017-09-21T08:45:43Z

projects/client/RabbitMQ.Client/src/client/impl/Connection.cs

@@ -767,7 +767,7 @@ public void MainLoopIteration()
            }
        }

-        public void NotifyHeartbeatListener()
+        public  void NotifyHeartbeatListener()


remove double space

kjnilsson · 2017-09-21T08:47:54Z

projects/client/RabbitMQ.Client/src/client/impl/ModelBase.cs

@@ -88,7 +87,7 @@ public abstract class ModelBase : IFullModel, IRecoverable

        private EventHandler<EventArgs> m_recovery;

-        public IConsumerDispatcher ConsumerDispatcher { get; private set; }
+        public  IConsumerDispatcher ConsumerDispatcher { get; private set; }


remove extra space

kjnilsson · 2017-09-21T08:49:31Z

projects/client/RabbitMQ.Client/src/client/impl/CommandAssembler.cs

                    {
                        throw new UnexpectedFrameException(f);
                    }
-                    byte[] fragment = f.Payload;


Is there real value in removing this variable? I would have thought (hoped) the compiler would optimise this away anyway.

YulerB · 2017-09-21T10:19:13Z

Since the frame sizes are calculable, might be wothwhile setting the capacity on the memory streams to avoid resizing and memory fragmentation.

For example,

HEADER var ms = new MemoryStream(8);
FRAME-> var ms = new MemoryStream(frame.Size);
FRAMESET-> var ms = new MemoryStream(frames.Sum(=>.Size));

danielmarbach · 2017-09-21T12:02:11Z

@YulerB if you worry about allocs then better no use Linq ;)

YulerB · 2017-09-21T13:13:50Z

@danielmarbach Linq gets a bad rap, but I know what you mean.

Many of the Framesets with 4k, which would require 2 re-allocs of the byte buffer in the MemoryStream.

Many of the message like Ack are much smaller than the default 1k, so we have over-allocation of memory .

This might avoid some of the memory thrashing.

To avoid linq, here is an extension method.

    public int Sum<T>(this IEnumerable<T> list, Func<T, int> expression)
    {
        int sum = 0;
        foreach (var item in list)
        {
            sum += expression(item);
        }
        return sum;
    }

kjnilsson · 2017-09-25T10:51:16Z

@bording @michaelklishin I've set our ci to push each PR build to myget - would this work for testing this PR? https://www.myget.org/feed/Packages/rabbitmq

bording · 2017-09-25T20:26:58Z

@kjnilsson Yes, having a MyGet feed that the results of each PR will be helpful for the future.

FYI, assuming your MyGet account name is rabbitmq, then the feed URL is https://www.myget.org/F/rabbitmq/api/v3/index.json. It doesn't look like any packages have been pushed to it yet, though!

bording · 2017-09-25T20:32:04Z

I have done some testing of these changes with a locally built copy of the client. I am seeing a small benefit on some tests that specifically ensure that we're publishing messages with multiple channels from the same connection.

I'm also not seeing any performance regressions, so overall it seems like this PR is 👍 from me, with the caveat that I haven't spent a lot of time actually looking at the content of the changes.

michaelklishin · 2017-09-26T02:18:08Z

@bording my personal concerns were about concurrency hazard safety but I assume if there were any failures in your test suites you'd have mentioned it. I'll do some last round naming and test structure changes before merging.

As integration test case subclasses are meant to do. Squashes a warning.

bording · 2017-09-26T02:40:38Z

@michaelklishin I'll try and run a few more rounds of tests to be sure, but I've not seen any problems so far.

There are no "read frames" and "write frames" in the protocol. "Incoming" and "outgoing" frames are also not well established terms but at least they hint at what the difference is and when they are used. There are existing methods with identical names (ReadFrame, WriteFrame) as well, which isn't helping.

Addressed Karl's feedback.

michaelklishin · 2017-09-26T05:41:16Z

In the end, so far we didn't manage to run into any concurrency hazards with this implementation and indeed there's a nice efficiency gain with concurrent writes on a shared connection. Thank you, @YulerB and @vendre21! Your contribution will be availalbe in the 5.1.0 version of the client :)

michaelklishin · 2017-09-26T10:15:36Z

We published a 5.1.0-pre1 to nuget.org for others to try this change out.

YulerB · 2017-09-26T11:03:21Z

Thanks for you help on this @michaelklishin, @bording, @danielmarbach, @kjnilsson, @lukebakken, @vendre21.

when transmitting multiple frames at once. While changes in #350, #354 are supposed to be safe, there's some evidence (#681) that sometimes they are not which leads to incorrect byte stream interleaving on the wire.

when transmitting multiple frames at once. While changes in #350, #354 are supposed to be safe, there's some evidence (#681) that sometimes they are not which leads to incorrect byte stream interleaving on the wire. Update API approval test expectations Re-introduce lock for all socket writes The official `NetworkStream` docs are confusing: https://docs.microsoft.com/en-us/dotnet/api/system.net.sockets.networkstream?view=netframework-4.5.1 > Read and write operations can be performed simultaneously on an instance of the NetworkStream class without the need for synchronization. As long as there is one unique thread for the write operations and one unique thread for the read operations, there will be no cross-interference between read and write threads and no synchronization is required. This is a poorly-written way to say "multiple threads must be synchronized". Fixes #681 Update API approval test expectations

YulerB and others added 24 commits September 12, 2017 20:17

Updated code to remove lock contention

20f9fc8

Details discussed on Google Groups https://groups.google.com/forum/?nomobile=true#!topic/rabbitmq-users/OyoYTkAyCSk

Merge pull request #1 from YulerB/YulerB-RemoveLocking

0907063

Updated code to remove lock contention

Updating frame with extra lines.

4177dbc

Updating casing of reference comment.

ce45f63

Updated with curly braces as requested in review.

3c2caa9

Updating casing of some comments.

36f90b2

Updating back to public

ac5ffba

Updated many elements back to public

3efb320

Updated casing of site address

40153cd

Updated casing of reference

5db2bd1

Updated private to public

5e99b0e

Updated setter back to private setter

22febeb

Re-Adding configureawait

f491885

Updating to configure await on connectasync as per trunk

Updating method back to private

c5d09f9

Updating spacing

9b70fb8

Updating back to public from private readonly

0f37c87

Reverting change

4fbcd56

Fixed brace position

1f10486

Just for check the newlines

3aecf44

Revert "Just for check the newlines"

0a61119

This reverts commit 3aecf44.

Just to check the line endings(CR chars)

3e37cba

Command and Frame Unix(LF) fix

e8a478f

Updated to unix line encoding.

61d9e79

Merge branch 'master' of https://github.com/YulerB/rabbitmq-dotnet-cl…

dc124a4

…ient into YulerB-master

michaelklishin added this to the 5.1.0 milestone Sep 19, 2017

michaelklishin mentioned this pull request Sep 19, 2017

Reduce lock contention for frame writes #350

Closed

michaelklishin added effort-medium enhancement labels Sep 19, 2017

michaelklishin requested review from lukebakken and kjnilsson September 19, 2017 20:10

More concurrent connection access tests

9332cde

kjnilsson previously requested changes Sep 21, 2017

View reviewed changes

Dispose the latch in ReleaseResources instead

260444e

As integration test case subclasses are meant to do. Squashes a warning.

michaelklishin added 2 commits September 26, 2017 06:55

Address dead code and formatting issues in PR review

8dc7ce4

michaelklishin merged commit 7e69311 into master Sep 26, 2017

michaelklishin mentioned this pull request Apr 25, 2018

The Write method cannot be called when another write operation is pending #417

Closed

michaelklishin mentioned this pull request Jan 3, 2020

Change in 5.1 synchronisation behavior when connection is shared between threads #681

Closed

lukebakken deleted the rabbitmq-dotnet-client-350-connection-write-lock-contention-reduction branch January 4, 2020 12:23

michaelklishin mentioned this pull request Jan 7, 2020

DO NOT MERGE Reintroduce locking around socket writes #682

Closed

lukebakken mentioned this pull request Jan 7, 2020

Re-introduce lock for all socket writes on a connection #683

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce lock contention for frame writes (with extra tests) #354

Reduce lock contention for frame writes (with extra tests) #354

michaelklishin commented Sep 19, 2017

bording commented Sep 19, 2017

michaelklishin commented Sep 19, 2017

michaelklishin commented Sep 20, 2017

bording commented Sep 20, 2017

kjnilsson left a comment

kjnilsson Sep 21, 2017

kjnilsson Sep 21, 2017

kjnilsson Sep 21, 2017

kjnilsson Sep 21, 2017

kjnilsson Sep 21, 2017

kjnilsson Sep 21, 2017

YulerB commented Sep 21, 2017

danielmarbach commented Sep 21, 2017

YulerB commented Sep 21, 2017

kjnilsson commented Sep 25, 2017

bording commented Sep 25, 2017

bording commented Sep 25, 2017

michaelklishin commented Sep 26, 2017

bording commented Sep 26, 2017

michaelklishin commented Sep 26, 2017

michaelklishin commented Sep 26, 2017

YulerB commented Sep 26, 2017

Reduce lock contention for frame writes (with extra tests) #354

Reduce lock contention for frame writes (with extra tests) #354

Conversation

michaelklishin commented Sep 19, 2017

bording commented Sep 19, 2017

michaelklishin commented Sep 19, 2017

michaelklishin commented Sep 20, 2017

bording commented Sep 20, 2017

kjnilsson left a comment

Choose a reason for hiding this comment

kjnilsson Sep 21, 2017

Choose a reason for hiding this comment

kjnilsson Sep 21, 2017

Choose a reason for hiding this comment

kjnilsson Sep 21, 2017

Choose a reason for hiding this comment

kjnilsson Sep 21, 2017

Choose a reason for hiding this comment

kjnilsson Sep 21, 2017

Choose a reason for hiding this comment

kjnilsson Sep 21, 2017

Choose a reason for hiding this comment

YulerB commented Sep 21, 2017

danielmarbach commented Sep 21, 2017

YulerB commented Sep 21, 2017

kjnilsson commented Sep 25, 2017

bording commented Sep 25, 2017

bording commented Sep 25, 2017

michaelklishin commented Sep 26, 2017

bording commented Sep 26, 2017

michaelklishin commented Sep 26, 2017

michaelklishin commented Sep 26, 2017

YulerB commented Sep 26, 2017