v7 Client Performance Degraded vs v6 with MassTransit

[phatboyg]

I'm sharing this as an initial pass, but from my testing there is a pretty major degradation with my first pass migration to the v7 client. I think it's related specifically to how the confirmation tracking is working. I've looking through the code and the MaybeTrackXxxx methods and it seems fairly straightforward. I also removed all confirmation tracking from MassTransit since it would essentially duplicate what you're doing.

My branch is here: /~https://github.com/phatboyg/MassTransit/tree/rabbitmq-client-v7

If you go into the tests/MassTransit.Benchmark project folder, and have RabbitMQ running accessible to localhost, you can type:

dotnet run -c Release --latency --count=80000 --clients=10 --prefetch=100 --confirm=true

If you compare the develop branch to the rabbitmq-client-v7 branch, you can see some pretty wide variation in the message throughput.

This is the original v6 client:

This is the new v7 client:

I'm open to suggestions/thoughts on where I could look, but I'm reluctant to upgrade MassTransit and impact customers with this drastic of a change.

ALSO, it seems like batch publishing is completely removed in the v7 client, so ignore the batch: enabled label in the test, it's just output but obviously ignored in the v7 codebase.

[phatboyg]

Okay, so, I tried something and I'm curious if it's legit.

Is the channel still considered to be only accessible by a single thread? Because if it remove the serialization of calls into the channel (which was required with the v6 client), the performance is much better.

So, is it valid to use a channel from multiple tasks concurrently?

If I do so, the throughput goes up significantly.

[michaelklishin]

Sharing channels between threads for publishing is explicitly not supported by client libraries as a general rule.

This client certainly was not an exception in 6.x. And I don't think v7 had this as one of the goals, so the safety guarantees (or lack thereof) should still be considered the same (the general rule).

[phatboyg]

Well, that's interesting, because I don't see how it would have any throughput without it (with respect to publish anyway).

Here: /~https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/v7.0.0/projects/RabbitMQ.Client/Impl/Channel.PublisherConfirms.cs#L294

The semaphore is taken, which blocks any other publisher from getting into it.

That's called from here, and at this point the semaphore has not yet been released.
/~https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/Impl/Channel.BasicPublish.cs#L56

And then, later down the line:
/~https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/Impl/Channel.PublisherConfirms.cs#L347

Which is called from:
/~https://github.com/rabbitmq/rabbitmq-dotnet-client/blob/main/projects/RabbitMQ.Client/Impl/Channel.BasicPublish.cs#L97

So, with these details in mind, it seems that BasicPublish can be called without waiting from anywhere and the semaphore will block an entrants (in-order, since the SemaphoreSlim implementation is a Queue) until the previous publish(es) are completed.

So, winning.

[phatboyg]

It also seems like all the DeclareXxx methods and others are blocked by an _rpcSemaphore so they should be callable as well and will queue up behind the semaphore.

[michaelklishin]

The client does not optimize for a high rate declarations and bindings, simply because it's not something Team RabbitMQ would recommend.

You can use multiple channels for different (groups of) operations, although in some cases that might not be safe (e.g. a confirmation can only be acknowledged on the delivery channel, consumer must be cancelled on the same channel it was registered on).

[lukebakken]

Please continue testing with the assumption that IChannel is thread-safe because I'm pretty sure it is in version 7. I have a task to review the code and ensure it, and then I will update our documentation.

#1722

[alrz]

batch publishing is completely removed in the v7

Would love to see this implemented built-in perhaps with some configurable window? since it's already handling confirmations internally.

[alrz]

Just to make sure we're on the same page, what I mean by that involves no new APIs, just configuring a tiny window e.g. 1s and batch all the concurrent publishes within that window in a single call. Does that make sense?

[lukebakken]

/~https://github.com/lukebakken/rabbitmq-dotnet-client-1721

[alrz]

Thanks, looks like this is meant to be implemented by concurrent tasks.. somehow I got the impression that basic.publish supports batching proper with a single roundtrip. Is that incorrect?

[lukebakken]

No, it never has because the AMQP 0.9.1 protocol does not support it. All that "batch publishing" ever did was hide the details of publishing over-and-over.

[lukebakken]

In the future, the protocol reference may be helpful - https://www.rabbitmq.com/amqp-0-9-1-protocol

[phatboyg]

Okay, just to update, the branch I linked above has been updated to no longer serialize anything into the connection or channel since they have been updated to use semaphore to avoid problems with multiple tasks hitting the channel. That seemed to increase the throughput significantly, even 20% above what it was in the previous v6 client.

Will continue testing.

[danielmarbach]

@phatboyg just double-checking. Did you disable the concurrency limit on the channel options for your tests, or did you use the default rate limiter?

[phatboyg]

I'm using the concurrency limit built into the client's thread pool instead of using my own semaphore (which I've removed in the latest develop branch since it's duplicative). I went through the channel-based dispatcher (very similar to my Task Executor in MassTransit) and seems like if you run out of concurrency slots, notifications aren't delivered to the consumer but I think that's the right call at this point.

[lukebakken]

Yes, I'm pretty sure that IChannel is thread-safe in version 7.0.0 of this library, though it wasn't an explicit design goal. I am planning to review and verify this, and update the docs / examples.

[phatboyg]

Thanks, I went through all the entrypoints for the methods used (by me anyway) and saw that all of them were protected either with the rpcSemaphore or the confirmSemaphore. So I'd say good job :)

[lukebakken]

Great, thanks for the initial review.

#1722

[phatboyg]

Also ran a test where 10,000 exchanges are declared, another 10000 bindings, and 100 queues, without the serialization of calls into the channel and it didn't die. so, that's a win.

[danielmarbach]

The channel is thread safe as far as I remember from reviewing the code. The critical paths are guarded by the RPC and the confirms semaphore, and it is OK to concurrently publish on the same channel. No need to further synchronize access to it.

[bollhals]

Even the v6 is threadsafe AS LONG AS you don't use publish confirmation. I've used it for years without thread protection in production.

So i'd specifically look how the counter is used for the publish confirmation when reviewing the code.

[danielmarbach]

Can this discussion be closed or is there anything left to discuss?

[michaelklishin]

Even if there is something else, a new one can always be started, and linked to be easier to spot for anyone who lands here.

[phatboyg]

I've created a master issue that links all issues related to the v7 client upgrade that have been reported.

MassTransit/MassTransit#5705

[lukebakken]

Thanks! Let me know if there's anything you need from this project.

[phatboyg]

@lukebakken if you look at the master issue, the main question is the delivery of "problems" like shutdown and the thread pool limit causing it to wait until consumers are completed (thus kind of conflicting with the nature of a connection shutting down waiting for a consumer).