Skip to content

Reduce Rpc Continuation Queue Locking#450

Merged
michaelklishin merged 3 commits intorabbitmq:masterfrom
YulerB:master
Sep 10, 2018
Merged

Reduce Rpc Continuation Queue Locking#450
michaelklishin merged 3 commits intorabbitmq:masterfrom
YulerB:master

Conversation

@YulerB
Copy link
Contributor

@YulerB YulerB commented Sep 9, 2018

Proposed Changes

  • Updated RpcContinuationQueue.cs.
  • Removed locking and replaced with an atomic operation.
  • This reduces memory and mean time to execute.
  • Also making the variable that is being locked private.
  • Also removes the chance of a null exception in code, where the output of Next() is not checked.
  • Added tests.
  • Benchmark test code and outcome added to comments below.

Types of Changes

What types of changes does your code introduce to this project?

  • Bugfix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (correction or otherwise)
  • Cosmetics (whitespace, appearance)
  • Performance Update

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

Further Comments

BenchmarkDotNet=v0.10.14, OS=Windows 10.0.17134
Intel Core i5-6200U CPU 2.30GHz (Skylake), 1 CPU, 4 logical and 2 physical cores
Frequency=2343750 Hz, Resolution=426.6667 ns, Timer=TSC
  [Host]     : .NET Framework 4.6.1 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.3132.0  [AttachedDebugger]
  Job-JWVBYN : .NET Framework 4.6.1 (CLR 4.0.30319.42000), 32bit LegacyJIT-v4.7.3132.0
Runtime=Clr  IsBaseline=True  
Method     Mean        Error           StdDev       Scaled      Rank       Gen 0        Allocated
RpcNew    26.94 ns    0.3565 ns    0.3334 ns   1.00          1             0.0076      12 B
RpcOld      73.05 ns    2.7595 ns    3.1779 ns   1.00          1             0.0178      28 B

Bench mark code:
bench.cs.txt

@kjnilsson
Copy link
Contributor

This looks good to me but as usual with these type of changes it would be good to get a couple of eyes on it.

namespace RabbitMQ.Client.Unit
{
[TestFixture]
public class TestRpcContinuationQueue {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ensure that the code formatting follows the rest of the project's style by placing the opening curly on a new line.

queue.Enqueue(inputContinuation1);
});
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove extra blank lines.

@michaelklishin michaelklishin merged commit ab0a022 into rabbitmq:master Sep 10, 2018
@michaelklishin
Copy link
Contributor

Thank you @YulerB.

@michaelklishin michaelklishin added this to the 5.2.0 milestone Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants