Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor improvements and optimizations #842

Merged
merged 4 commits into from
May 26, 2020

Conversation

bollhals
Copy link
Contributor

Proposed Changes

This PR contains various minor improvements I found while getting to know the code. The changes are in the areas of

  • reduce allocations (Enumerators mainly)
  • improve performance by using the type directly instead of the interface
  • some random tweaks here and there

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

Further Comments

Publicly visible API is not touched at all. All I touched are internals, which I hope is fine.
Please let me know if this breaches some guideline or something.

@lukebakken lukebakken self-assigned this May 21, 2020
@lukebakken
Copy link
Contributor

Hello, thanks for taking the time to do this. What is the best way to prove that these changes have a positive effect on performance or other metric?

@bollhals
Copy link
Contributor Author

Excellent question, difficult to answer. If there isn't a performance benchmark suite we can run, best I can do is some micro benchmarks:

For Interface vs Class

    [ShortRunJob]
    [MemoryDiagnoser]
    public class DictionaryBenchmark
    {
        public DictionaryBenchmark()
        {
            this.Real = new Dictionary<string, string>();
            this.Interface = new Dictionary<string, string>();
        }

        public Dictionary<string, string> Real { get; }
        public IDictionary<string, string> Interface { get; }

        [Benchmark]
        public void AsDictionary()
        {
            this.Real.Add("hi", "ho");
            this.Real.Remove("hi");
        }

        [Benchmark]
        public void AsInterface()
        {
            this.Interface.Add("hi", "ho");
            this.Interface.Remove("hi");
        }

    }
Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
AsDictionary 25.75 ns 2.487 ns 0.136 ns - - - -
AsInterface 27.16 ns 1.210 ns 0.066 ns - - - -

@bollhals
Copy link
Contributor Author

And for IList

    [ShortRunJob]
    [MemoryDiagnoser]
    public class IListBenchmark
    {
        public IListBenchmark()
        {
            this.Interface = Enumerable.Range(0, 10).Select(i => i.ToString()).ToList();
        }

        public IList<string> Interface { get; }

        [Benchmark]
        public void ForEach()
        {
            foreach (var s in this.Interface)
            {
                UseString(s);
            }
        }

        [Benchmark]
        public void For()
        {
            for (var i = 0; i < this.Interface.Count; i++)
            {
                UseString(this.Interface[i]);
            }
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        private static void UseString(string s)
        { }
    }
Method Mean Error StdDev Gen 0 Gen 1 Gen 2 Allocated
ForEach 104.49 ns 17.500 ns 0.959 ns 0.0085 - - 40 B
For 45.49 ns 1.252 ns 0.069 ns - - - -

@lukebakken
Copy link
Contributor

@bollhals thanks for sharing that.

@stebet did you share your benchmark app and I just can't seem to find it now?

@stebet
Copy link
Contributor

stebet commented May 21, 2020

I'm liking this a lot. I have another PR coming as well, which avoid even more allocations as well. In the meantime, here is my oh-so-simple benchmark app.

using System;
using System.Threading;
using System.Threading.Tasks;
using RabbitMQ.Client;
using RabbitMQ.Client.Events;

namespace DeadlockRabbitMQ
{
    class Program
    {
        private static int messagesSent = 0;
        private static int messagesReceived = 0;
        private static int batchesToSend = 100;
        private static int itemsPerBatch = 500;
        static async Task Main(string[] args)
        {
            ThreadPool.SetMinThreads(16 * Environment.ProcessorCount, 16 * Environment.ProcessorCount);
            var connectionString = new Uri("amqp://guest:guest@localhost/");

            var connectionFactory = new ConnectionFactory() { DispatchConsumersAsync = true, Uri = connectionString };
            var connection = connectionFactory.CreateConnection();
            var publisher = connection.CreateModel();
            var subscriber = connection.CreateModel();

            publisher.ConfirmSelect();
            publisher.ExchangeDeclare("test", ExchangeType.Topic, false, false);

            subscriber.QueueDeclare("testqueue", false, false, true);
            var asyncListener = new AsyncEventingBasicConsumer(subscriber);
            asyncListener.Received += AsyncListener_Received;
            subscriber.QueueBind("testqueue", "test", "myawesome.routing.key");
            subscriber.BasicConsume("testqueue", true, "testconsumer", asyncListener);

            byte[] payload = new byte[512];
            var batchPublish = Task.Run(() =>
            {
                while (messagesSent < batchesToSend * itemsPerBatch)
                {
                    var batch = publisher.CreateBasicPublishBatch();
                    for (int i = 0; i < itemsPerBatch; i++)
                    {
                        var properties = publisher.CreateBasicProperties();
                        properties.AppId = "testapp";
                        properties.CorrelationId = Guid.NewGuid().ToString();
                        batch.Add("test", "myawesome.routing.key", false, properties, payload);
                    }
                    batch.Publish();
                    messagesSent += itemsPerBatch;
                    publisher.WaitForConfirmsOrDie();
                }
            });

            var sentTask = Task.Run(async () =>
            {
                while (messagesSent < batchesToSend * itemsPerBatch)
                {
                    Console.WriteLine($"Messages sent: {messagesSent}");

                    await Task.Delay(500);
                }

                Console.WriteLine("Done sending messages!");
            });

            var receivedTask = Task.Run(async () =>
            {
                while (messagesReceived < batchesToSend * itemsPerBatch)
                {
                    Console.WriteLine($"Messages received: {messagesReceived}");

                    await Task.Delay(500);
                }

                Console.WriteLine("Done receiving all messages.");
            });

            await Task.WhenAll(sentTask, receivedTask);

            publisher.Dispose();
            subscriber.Dispose();
            connection.Dispose();
            Console.ReadLine();
        }

        private static Task AsyncListener_Received(object sender, BasicDeliverEventArgs @event)
        {
            Interlocked.Increment(ref messagesReceived);
            return Task.CompletedTask;
        }
    }
}

Edit: fixed a bug in the benchmarking code.

@michaelklishin
Copy link
Member

Some of the changes here optimize methods that are never invoked on the hot code path (connection recovery is one example). Is it a worthwhile investment of our time, trying to squeeze a few nanoseconds in a loop here and a field there in such areas?

@bollhals
Copy link
Contributor Author

Some of the changes here optimize methods that are never invoked on the hot code path (connection recovery is one example). Is it a worthwhile investment of our time, trying to squeeze a few nanoseconds in a loop here and a field there in such areas?

I do not yet know which ones are used where, but then again, if there isn't a downside to it, faster is always better I guess.

@michaelklishin
Copy link
Member

The downside to me is that the intent of a classic (indexed) for loop is rarely immediately clear. I can tell what for each, select/filter and similar higher-order functions try to do just by looking at their name.

Connection recovery runs no more often than once every 5 seconds by default, and quite often a few times in a given process' lifetime.

@bollhals
Copy link
Contributor Author

The intent is certainly clearer in the foreach. Unfortunately for IList<> the foreach allocates. And even if it is only executed every 5 seconds, that sums up in a day to almost 700'000 bytes that are allocated.

@michaelklishin
Copy link
Member

Fair enough, I don't feel too strongly about this.

@michaelklishin
Copy link
Member

@bollhals can you please rebase this branch on top of master? Note that we'd prefer a rebase as it would make backporting (and undoing a backport if we have to one day) easier. Thank you.

@bollhals bollhals force-pushed the minor.improvements branch from a6b0919 to d1f0e45 Compare May 25, 2020 20:00
@bollhals
Copy link
Contributor Author

@bollhals can you please rebase this branch on top of master? Note that we'd prefer a rebase as it would make backporting (and undoing a backport if we have to one day) easier. Thank you.

/done

@michaelklishin michaelklishin changed the title Minor.improvements Minor improvements and optimizations May 26, 2020
@michaelklishin michaelklishin merged commit d1f0e45 into rabbitmq:master May 26, 2020
@michaelklishin
Copy link
Member

Backported to 6.x.

@michaelklishin michaelklishin added this to the 6.1.0 milestone May 26, 2020
@bollhals bollhals deleted the minor.improvements branch May 26, 2020 17:24
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