[WIP] Simple version of PooledMemoryStream #452

Pliner · 2018-09-13T08:11:18Z

Proposed Changes

This PR continues work on allocations(see #443). Its goal is to provide the pool of MemoryStream to reduce allocations, which were caused by resize of the internal buffer of MemoryStream.

The idea of PR is not to pool MemoryStream, but to pool internal buffers. So, PooledMemoryStream is introduced, which is the same as MemoryStream, but it also pools internal buffers with help of System.Buffers.

A bit more info about System.Buffers:

It has default shared implementation(ArrayPool<T>.Shared), which can be reused not only by rabbitmq client.
The default implementation has the following allocation strategy: it allocates arrays of size 2^i, where i from 4 to 20. If the size of renting array is larger than 2^20, it will allocate an array outside of the pool.
Also it has some trimming strategy, which helps to free memory in case of low memory.
This package is referenced transitively by MongoDB Client(DnsClient references System.Buffers), so it may be not so awfull to have this package as dependency.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

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)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

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

Producer.cs

    public static class EntryPoint
    {
        public static void Main(string[] args)
        {
            var connectionFactory = new ConnectionFactory();
            using (var connection = connectionFactory.CreateConnection())
            {
                using (var model = connection.CreateModel())
                {
                    model.ExchangeDeclare("Exchange", "direct", true);
                    for (var i = 0; i < 10000; ++i)
                    {
                        model.BasicPublish("Exchange", "#", body: new byte[100000]);
                    }
                }
            }   

            Console.ReadKey();
        }
    }

Before:

Before.zip

After:

After.zip

As you can see, there are no more huge allocations of byte arrays except Producer. We can expect the same behaviour when size of body is less then 1 megabytes, because max size of pooled array is 2^20.

One more non-proven observation: after this fix Producer.cs(I recompiled it in Release mode) works significantly faster(before - 3.7s , after - 2.5s).

PS It's an experiment. Please, do not merge it, because PooledMemoryStream is not properly tested. Also I'm sure we need to measure the performance too.

michaelklishin · 2018-09-13T18:13:38Z

Almost a 4-fold peak heap usage reduction, good job @Pliner!

lukebakken · 2018-09-13T18:32:42Z

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

@@ -75,7 +75,7 @@ public Command(MethodBase method, ContentHeaderBase header, byte[] body)
        }

        public byte[] Body { get; private set; }
-
+        


Please be aware of and remove whitespace changes like these.

Yeap, sorry, missed it 😞

lukebakken · 2018-09-13T18:33:45Z

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

@@ -67,10 +67,12 @@ public static async Task TimeoutAfter(this Task task, int millisecondsTimeout)
        }
    }

+


Was this newline added by an automatic tool of some sort?

Don't know, actually. Fixed it.

kjnilsson · 2018-09-13T18:40:55Z

Looks very promising. :)

paulomorgado · 2018-09-14T13:51:37Z

Have you looked at pipelines?

https://blog.marcgravell.com/search/label/pipelines

michaelklishin · 2018-09-15T21:56:28Z

@Pliner the title still says WIP. Should we wait for more changes, e.g. are you going to adopt pipelines? And what'd be the minimum required .NET version be if so? Thanks.

bording · 2018-09-16T06:34:41Z

@michaelklishin Using pipelines would be a fundamental re-architecture of the entire client. It's what my experimental client, Angora, is using.

Pliner · 2018-09-17T09:18:58Z

@bording @paulomorgado Thanks for information about pipelines 😉

@michaelklishin I will push some cosmetic changes today. Is it ok to add System.Buffer as a dependency to client?

PS Also today I will try to benchmark client at least on EasyNetQ.

kjnilsson · 2018-09-17T10:15:01Z

@Pliner generally we haven't had a very good experience with adding dependencies to the client. That said System.Buffers looks pretty safe. Does it work with net451?

Pliner · 2018-09-17T10:45:26Z

@Pliner generally we haven't had a very good experience with adding dependencies to the client. That said System.Buffers looks pretty safe. Does it work with net451?

Yep.

I'm not sure too, but I will help to reimplement this pool inside the client as was suggested by @Scooletz in #443 in case of any troubles due to dependency.

kjnilsson · 2018-09-17T10:52:18Z

The troubles with dependencies are hard to predict without a proper release that people actually use. For now I would prefer if we tried to avoid taking dependencies unless you think there is a substantial risk of severe bugs re-implementing the pool?

Pliner · 2018-09-17T15:21:02Z

The troubles with dependencies are hard to predict without a proper release that people actually use. For now I would prefer if we tried to avoid taking dependencies unless you think there is a substantial risk of severe bugs re-implementing the pool?

Deal. I will implement simple pool and compare it to ArrayPool from System.Buffers. I hope it will help to decide is it worth to depend on it or not 😄

bording · 2018-09-17T21:46:37Z

One reason that a reference to System.Buffers might not be a good idea is that it only ships a netstandard1.1 and netstandard2.0 version of the lib, so if you're using net461 through net471 you can end up in some weird pain because of how the .NET Standard 2.0 support for those versions works. Lots of difficult to solve binding redirect errors, etc.

paulomorgado · 2018-09-17T22:34:54Z

And it's not trivial to everyone to get out of that mess.

paulomorgado · 2018-09-17T22:36:18Z

The beauty of it is that we don't need to have the same implementation for all targets. Targets the benefit from better libraries should take advantage of it.

michaelklishin · 2018-09-19T08:05:48Z

@Pliner there's some interest in a 5.2.0 release in the next few days. Is this still a WIP? Should we QA it?

michaelklishin · 2018-09-19T08:06:26Z

I see that the System.Buffers dependency is still there. So maybe this can go into 5.3.0 then.

Pliner · 2018-09-19T08:08:42Z

I see that the System.Buffers dependency is still there. So maybe this can go into 5.3.0 then.

Yes. I need a bit more time. I hope today I will push the new pool and remove System.Buffers dependency.

michaelklishin · 2018-09-19T08:47:59Z

Currently we have #441 scheduled for 5.2.0 since it is completely backwards-compatible w.r.t. to the public API. and #442, #443 and this PR will have to potentially wait till 6.0.

0. Formatting 1. Rename GetSegment to GetBufferSegment 2. Exceptions of PooledMemoryStream match to exceptions of MemoryStream

Pliner · 2018-09-20T11:55:54Z

I pushed the new pool. Several considerations:

ArrayPool allocates lazily ~16mb of arrays.
Pool is a simple version of Roslyn pools. Merely the same approach is implemented in StackExchange.Redis.
ArrayPool was not tested or compared with ArrayPool from System.Buffers. I will do it soon.

paulomorgado · 2018-09-20T15:03:48Z

@Pliner,

If it's the same API, consider, for targets that can depend on System.Buffers, use System.Buffers instead of replicating it.

rwkarg · 2018-11-12T20:10:52Z

There is also a provided RecyclableMemoryStream that handles internal pooling of buffers (most applicable to this use case) and also using multiple small buffers instead of one large one for large streams to avoid LOH allocation (probably less useful, though there certainly could be messages larger than 85K bytes)
https://github.com/Microsoft/Microsoft.IO.RecyclableMemoryStream
This would be a good battle tested (it's used by the Bing team) option to evaluate.

stebet · 2020-01-25T12:11:49Z

I've made several related/similar changes using the latest libraries. Check out #694 and the associated PR once I submit it (ironing out the last remaining bugs in tests). It includes System.IO.Pipelnes support as well.

Pliner · 2020-01-26T06:41:54Z

Closed because of #694

lukebakken reviewed Sep 13, 2018

View reviewed changes

michaelklishin added this to the 6.0 milestone Sep 19, 2018

Pliner added 4 commits September 20, 2018 16:11

Simple version of PooledMemoryStream

14c65a7

Rollback whitespaces

cde2f52

Remove unused GetSegmentBuffer

fd4d508

Several cosmetic changes

c76d796

0. Formatting 1. Rename GetSegment to GetBufferSegment 2. Exceptions of PooledMemoryStream match to exceptions of MemoryStream

Pliner force-pushed the pool-memory-stream branch from fd8f061 to c76d796 Compare September 20, 2018 11:12

Replace System.Buffers with simple alternative

08264e6

Pliner force-pushed the pool-memory-stream branch from aff754d to 08264e6 Compare September 20, 2018 11:33

lechu445 mentioned this pull request Jan 23, 2020

Reduce allocations by using pooled memory and recycling memory streams #694

Closed

Pliner closed this Jan 26, 2020

Pliner deleted the pool-memory-stream branch January 26, 2020 06:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Simple version of PooledMemoryStream #452

[WIP] Simple version of PooledMemoryStream #452

Pliner commented Sep 13, 2018 •

edited

Loading

michaelklishin commented Sep 13, 2018

lukebakken Sep 13, 2018

Pliner Sep 15, 2018

lukebakken Sep 13, 2018

Pliner Sep 15, 2018

kjnilsson commented Sep 13, 2018

paulomorgado commented Sep 14, 2018

michaelklishin commented Sep 15, 2018

bording commented Sep 16, 2018

Pliner commented Sep 17, 2018

kjnilsson commented Sep 17, 2018

Pliner commented Sep 17, 2018

kjnilsson commented Sep 17, 2018

Pliner commented Sep 17, 2018 •

edited

Loading

bording commented Sep 17, 2018

paulomorgado commented Sep 17, 2018

paulomorgado commented Sep 17, 2018

michaelklishin commented Sep 19, 2018

michaelklishin commented Sep 19, 2018

Pliner commented Sep 19, 2018

michaelklishin commented Sep 19, 2018 •

edited

Loading

Pliner commented Sep 20, 2018

paulomorgado commented Sep 20, 2018

rwkarg commented Nov 12, 2018

stebet commented Jan 25, 2020

Pliner commented Jan 26, 2020

		@@ -75,7 +75,7 @@ public Command(MethodBase method, ContentHeaderBase header, byte[] body)
		}

		public byte[] Body { get; private set; }

		@@ -67,10 +67,12 @@ public static async Task TimeoutAfter(this Task task, int millisecondsTimeout)
		}
		}

[WIP] Simple version of PooledMemoryStream #452

[WIP] Simple version of PooledMemoryStream #452

Conversation

Pliner commented Sep 13, 2018 • edited Loading

Proposed Changes

Types of Changes

Checklist

Further Comments

michaelklishin commented Sep 13, 2018

lukebakken Sep 13, 2018

Choose a reason for hiding this comment

Pliner Sep 15, 2018

Choose a reason for hiding this comment

lukebakken Sep 13, 2018

Choose a reason for hiding this comment

Pliner Sep 15, 2018

Choose a reason for hiding this comment

kjnilsson commented Sep 13, 2018

paulomorgado commented Sep 14, 2018

michaelklishin commented Sep 15, 2018

bording commented Sep 16, 2018

Pliner commented Sep 17, 2018

kjnilsson commented Sep 17, 2018

Pliner commented Sep 17, 2018

kjnilsson commented Sep 17, 2018

Pliner commented Sep 17, 2018 • edited Loading

bording commented Sep 17, 2018

paulomorgado commented Sep 17, 2018

paulomorgado commented Sep 17, 2018

michaelklishin commented Sep 19, 2018

michaelklishin commented Sep 19, 2018

Pliner commented Sep 19, 2018

michaelklishin commented Sep 19, 2018 • edited Loading

Pliner commented Sep 20, 2018

paulomorgado commented Sep 20, 2018

rwkarg commented Nov 12, 2018

stebet commented Jan 25, 2020

Pliner commented Jan 26, 2020

Pliner commented Sep 13, 2018 •

edited

Loading

Pliner commented Sep 17, 2018 •

edited

Loading

michaelklishin commented Sep 19, 2018 •

edited

Loading