Added ability to specify custom ArrayPool

[sakno]

Proposed Changes

This change is aimed to fix inefficient memory pooling as described and discussed in #1184. It allows to specify custom ArrayPool<byte> through ConnectionFactory instead of ArrayPool<byte>.Shared.

Types of Changes

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

• 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

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

Unfortunately, 6.x branch is based on .NET Standard 2.0. This target framework doesn't support ReadAsync/WriteAsync stream overloads supporting ReadOnlyMemory<byte> type. Thus, MemoryPool<byte> is not applicable here.

[sakno]

@lukebakken , can we have this patch for 6.x ? For 7.x, it's better to use MemoryPool<byte> and truly async BasicPublish.

[lukebakken]

Whoops, wrong milestone. Fixed. I'm keeping an eye out for when this is ready to review.

[sakno]

@sungam3r , should I add a new property to APIApproval.verified.txt file?

[sungam3r]

Yes, API approval tests work so.

[lukebakken]

Pinging @bollhals @danielmarbach and @stebet to see if they have any input. This is a change that would go into version 6.3.0 only.

[danielmarbach]

On vacation. Can only look next to week. Otherwise feel free to proceed

[bollhals]

Pinging @bollhals @danielmarbach and @stebet to see if they have any input. This is a change that would go into version 6.3.0 only.

See my comment in the issue, to which I didn't get an answer yet.

=> I don't see a real world use case where the selection of the memory pool helps solving an issue. So I wouldn't take this unless it is understood why and when.

Just being able to switch the memory manager, doesn't solve anything on its own, as we're still copying the data to another buffer than the one you provide in the publish.

[danielmarbach]

For reference

#1184 (comment)

And I agree with the comment

[lukebakken]

Thanks everyone for the feedback. I need to more carefully re-review the #1184 discussion.

[sakno]

@bollhals , changing ArrayPool from Shared to customized version using ArrayPool<byte>.Create factory method really solves our issue with high memory pressure caused by very optimistic cache inside of the shared pool and increased bucket size in .NET 6.

[bollhals]

So your issue therefore is more about the arrays being retained too long inside the shared pool, right?

[sakno]

@bollhals , not only this. There are few reasons causing OOM:

• Shared array pool is optimized to call Rent and Return on the same thread
• Returning array in the different thread (as RabbitMQ client does) causes to fill all buckets within the pool (and TLS stacks). The number of buckets is equal to the processor count, resulting in a huge memory consumption even for relatively small arrays (1-2 Megabytes)
• Large arrays placed in LOH
• Shared array pool releases strong references to them only in case of high memory pressure through GC callback (since .NET 6)
• This behavior leads to very high fragmentation of LOH (~ 90%)

Custom ArrayPool allows to customize bucket size. In corner cases, we can use our own implementation of the pool (to use POH instead of LOH + remove optimizations for thread-local caching).

[lukebakken]

It sounds like the code in this PR solves an issue that few users experience, maybe only @sakno.

@sakno - is your optimized implementation of ArrayPool<byte> open-source? We could point users of this library to it.

[sakno]

@lukebakken , we are using ArrayPool<T>.Create factory method from .NET to achieve necessary performance optimizations (still not the best option, but acceptable). For now, we see no reason to provide custom implementation of the pool. However, in 7.x we would like to see MemoryPool<T> instead of ArrayPool<T>. In that case we will definitely switch to custom implementation which is already exist: UnmanagedMemoryPool<T>.

[bollhals]

@bollhals , not only this. There are few reasons causing OOM:

• Shared array pool is optimized to call Rent and Return on the same thread
• Returning array in the different thread (as RabbitMQ client does) causes to fill all buckets within the pool (and TLS stacks). The number of buckets is equal to the processor count, resulting in a huge memory consumption even for relatively small arrays (1-2 Megabytes)
• Large arrays placed in LOH
• Shared array pool releases strong references to them only in case of high memory pressure through GC callback (since .NET 6)
• This behavior leads to very high fragmentation of LOH (~ 90%)

Custom ArrayPool allows to customize bucket size. In corner cases, we can use our own implementation of the pool (to use POH instead of LOH + remove optimizations for thread-local caching).

Thanks for commenting.
I can see the benefits for your case. Especially if you run it in a high core scenario with limited memory.

So for me this PR is acceptable if

• The maintainers think this is a scenario that they want to better support
• The performance impact is negligible for the default case

[stebet]

Just as a comment, Json.net has a very similar extension point for the same reasons: http://james.newtonking.com/archive/2015/12/20/json-net-8-0-release-1-allocations-and-bug-fixes

[michaelklishin]

How common is the scenario with a lot of cores but little memory available to the runtime?

[sakno]

How common is the scenario with a lot of cores but little memory available to the runtime?

k8s containers, for instance. We have 1 Gb limit per container. Due to LOH fragmentation, even 2 Gb can't help.

[michaelklishin]

@sakno OK but how many CPU resources do your pods have? We mostly see environments that are limited in every kind of resource, or those that have plenty of both CPU and memory. I'm
not saying that "lots of CPU cores, little memory" is not a common enough scenario but
I just don't see it in the wild amongst RabbitMQ users, on Kubernetes on or.

[sakno]

@michaelklishin , CPU limit causes throttling on k8s for .NET application (for instance, dotnet/aspnetcore#29150) sometimes. So we use limits for RAM only. 8 cores are visible to the container.

[michaelklishin]

Fair enough. Then if it's worth the time investment and the default scenario is marginally affected, as @bollhals suggests, merging this would be a net positive for .NET on Kubernetes users :)

[CrazyMushu]

Dear colleagues, have you faced any troubles with this pull-request? I am really waiting for the new library version with these changes.

[lukebakken]

Dear colleagues, have you faced any troubles with this pull-request?

No, I just haven't had time to run any benchmarks to ensure these changes don't affect the normal case.

[sakno]

Dear colleagues, have you faced any troubles with this pull-request?

No, I just haven't had time to run any benchmarks to ensure these changes don't affect the normal case.

Technically, access to the static field (ArrayPool.Shared) is replaced by instance field. From assembler point of view, both types of access are mov instruction.

[lukebakken]

@sakno thanks!

[lukebakken]

https://github.com/rabbitmq/rabbitmq-dotnet-client/releases/tag/v6.3.0