Code cleanups and performance optimizations.

[stebet]

Proposed Changes

Removing some redundant code and some performance optimizations.

Types of Changes

• Bug fix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)

Checklist

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

[stebet]

@bollhals might want to take a look as well.

[bollhals]

Looking good to me. Question is though, what will remain in Protocol class? It seems to slowly die out.

[bollhals]

can we switch the common case first?

[stebet]

That'll involve doing a lot more checks really. This just does the first check on if _closing which is almost always false and then just continues to the transmit. This usually compiles to no code jumping unless the if matches which is usually faster.

[michaelklishin]

I expect this to be cheaper than acquiring a lock either way ;)

[bollhals]

This for sure =)
A quick "is null" check might be as fast as a enum compare 🤔

[stebet]

Looking good to me. Question is though, what will remain in Protocol class? It seems to slowly die out.

Yes. The Protocol class seems redundant to me. I can see what the point of it was but extending the client with a new protocol other than AMQP 0.9.8 is a much bigger task than just adding a new protocol.

Is there a reason to keep the protocol abstraction around @michaelklishin? Seems like it'd be easier to write a new client for a new protocol than to support more than one in the current client.

[bollhals]

Looking good to me. Question is though, what will remain in Protocol class? It seems to slowly die out.

Yes. The Protocol class seems redundant to me. I can see what the point of it was but extending the client with a new protocol other than AMQP 0.9.8 is a much bigger task than just adding a new protocol.

Is there a reason to keep the protocol abstraction around @michaelklishin? Seems like it'd be easier to write a new client for a new protocol than to support more than one in the current client.

Sounds reasonable to me.

[michaelklishin]

This looks reasonable, thank you. Any benchmarks that demonstrate the improvement for some key workloads?

[stebet]

This looks reasonable, thank you. Any benchmarks that demonstrate the improvement for some key workloads?

I'll see if I can do some benchmarks. They are a bit harder to do here since they require actual usages of Connection/Model objects.

[danielmarbach]

In general it is not a good practice to lock on this since this can lead to deadlocks because your are locking the whole instance on which someone external could also lock on. Either bring the lock instance object back or use a compare exchange spin approach?

[bollhals]

Sessions are internal objects not exposed in any way to the public. While I agree with the general statement, I think one does not always have to follow it if the lock from outside is not possible.

[stebet]

Yeah, I know locking on the instance is not recommended but in this case we can be sure that no one external will lock on it, unless through reflection gymnastics but all bets are off at that point anyway.

[danielmarbach]

I'm a fan of least surprises. Even for internal usage. It doesn't necessarily have to be user code. Sometimes over time things slip in. Having a dedicated lock is the safer choice. Yet I'm not hung up on it. Feel free to ignore

[michaelklishin]

We can add a new field just for locking. One more object per connection is not going to make much of a difference.

[michaelklishin]

Should we do this?

[stebet]

Rudimentary benchmarks for unthrottled publishing with 8 connections:
Before:
Sent 18663500 messages in 60044 ms. 311058 messages per sec.

Calls to ManualResetEventSlim.Wait():

After:
Sent 18683500 messages in 60036 ms. 311391 messages per sec.

Calls to ManualResetEventSlim.Wait():

[michaelklishin]

Given that the gain is 0,1% or so, do we want to proceed with this solely for code simplification (if any) or just close it? We obviously appreciate the time @stebet and the reviewers have put into this PR. But if an optimization PR doesn't make any difference to benchmarks in the end, is it worth adopting?

[stebet]

Given that the gain is 0,1% or so, do we want to proceed with this solely for code simplification (if any) or just close it? We obviously appreciate the time @stebet and the reviewers have put into this PR. But if an optimization PR doesn't make any difference to benchmarks in the end, is it worth adopting?

I'm biased, this being my PR and all, but from a code cleaning perspective I'd say yes. The performance optimization was purely a bonus and not the main point of the PR, although less locking is always better too. Although throughput gains were minimal, CPU usage also dropped a bit as a result.

[michaelklishin]

The Protocol class seems redundant to me

At this point, we can just drop it and leave only 0-9-1 as supported. 0-8 and 0-9 share a lot with 0-9-1 (unlike 1.0, which is a completely different protocol), so years ago it made sense to support them all.

0-9-1 is available in RabbitMQ 2.0+, which has been out for more than a decade. So we can remove all 0-8 and 0-9 bits (I don't expect there to be many) in a separate PR.