Optimizing memory usage even further. #824
Conversation
|
Example of allocation reductions (using my benchmark app, sending and reciving 50.000 events with 512 byte payloads). Before: After: |
|
I didn't notice #816 which makes the same change to the contract. I'll make changes to accomodate. I do think adding a method to the contracts should not require a major change as additions are normally not considered breaking, unless they might affect certain overloads, so we could add the ReadOnlyMemory overload to the batch publishing interface and still keep the byte[] version as well. What do you think @bording? |
|
I see a bunch of low hanging fruit:
|
| @@ -259,6 +259,7 @@ namespace RabbitMQ.Client | |||
| public interface IBasicPublishBatch | |||
| { | |||
| void Add(string exchange, string routingKey, bool mandatory, RabbitMQ.Client.IBasicProperties properties, byte[] body); | |||
| void Add(string exchange, string routingKey, bool mandatory, RabbitMQ.Client.IBasicProperties properties, System.ReadOnlyMemory<byte> body); | |||
There was a problem hiding this comment.
Unfortunately since this is a change to an interface, this means this is a breaking change. If you're wanting to get this in to a release before 7.0, you can't do this.
There was a problem hiding this comment.
I think this should go into 7.0 which we can release fairly soon. @lukebakken WDYT?
There was a problem hiding this comment.
As far as I know there are quite a few things that have been discussed for 7.0, one of which is a fully-async client. I think @stebet has done some work on that.
If "fairly soon" means within 6 months... maybe???
There was a problem hiding this comment.
Yeah, I've done some research and experiments. It'd probably require quite a lot of changes to the public interface, all of which are good i.m.o, like departing from Event based notifications (can be replaced with a pluggable interface), but I'll write up a proposal soon for that. Reverting any interface changes for now :)
That is only true when you're talking about a class. Any changes to an interface are breaking because any existing implementations of that interface break. That's why default interface methods were added to C# 8. |
|
I'll take a closer look and follow up later tonight. Thanks for this @davidfowl and @bording. It makes sense to skip the interface change as most of this is very applicable to 6.X. |
|
@stebet I'm going to test and review this today. Since there are no interface changes, I assume this is OK for |
…asicPublishBatch interface.
…wing us to use "stackalloc".
lukebakken
force-pushed
the
arrayPoolOptimizations
branch
from
28082c8 to
f67908b
Compare
Proposed Changes
After writing a blog on the allocation fixes and publishing last weekend, I got a few pointers from @davidfowl on saving even more allocations by using ArrayPool instead of MemoryPool. I also cleaned up some related code while I was at it.
Types of Changes
Checklist
CONTRIBUTING.mddocument