-
Notifications
You must be signed in to change notification settings - Fork 580
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
[WIP] Simple version of PooledMemoryStream #452
Conversation
Almost a 4-fold peak heap usage reduction, good job @Pliner! |
@@ -75,7 +75,7 @@ public Command(MethodBase method, ContentHeaderBase header, byte[] body) | |||
} | |||
|
|||
public byte[] Body { get; private set; } | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please be aware of and remove whitespace changes like these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeap, sorry, missed it 😞
@@ -67,10 +67,12 @@ public static async Task TimeoutAfter(this Task task, int millisecondsTimeout) | |||
} | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this newline added by an automatic tool of some sort?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't know, actually. Fixed it.
Looks very promising. :) |
Have you looked at pipelines? |
@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. |
@michaelklishin Using pipelines would be a fundamental re-architecture of the entire client. It's what my experimental client, Angora, is using. |
@bording @paulomorgado Thanks for information about pipelines 😉 @michaelklishin I will push some cosmetic changes today. Is it ok to add PS Also today I will try to benchmark client at least on EasyNetQ. |
@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. |
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 |
One reason that a reference to System.Buffers might not be a good idea is that it only ships a |
And it's not trivial to everyone to get out of that mess. |
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. |
@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? |
I see that the |
Yes. I need a bit more time. I hope today I will push the new pool and remove |
0. Formatting 1. Rename GetSegment to GetBufferSegment 2. Exceptions of PooledMemoryStream match to exceptions of MemoryStream
fd8f061
to
c76d796
Compare
aff754d
to
08264e6
Compare
I pushed the new pool. Several considerations:
|
If it's the same API, consider, for targets that can depend on |
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) |
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. |
Closed because of #694 |
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 ofMemoryStream
.The idea of PR is not to pool
MemoryStream
, but to pool internal buffers. So,PooledMemoryStream
is introduced, which is the same asMemoryStream
, but it also pools internal buffers with help of System.Buffers.A bit more info about
System.Buffers
:ArrayPool<T>.Shared
), which can be reused not only by rabbitmq client.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 applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creatingthe 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.
CONTRIBUTING.md
documentFurther Comments
Producer.cs
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.