Pipelines implementation and allocation improvements

[stebet]

Proposed Changes

This pull-request includes a WIP of several big changes to allocations and will most likely include some breaking API changes (pending approval).

Here is the gist of it (some improvements are still left to do):

• Use System.IO.Pipelines to handle the socket reading/writing and to handle parsing and writing frames/commands
• Use ArrayPool to reduce allocations of byte arrays.
• Use RecyclableMemoryStream to reduce allocation of streams when writing temporary data.

This reduces allocations considerably on my machine, as can be seen in the table below (will update as this PR progresses). I have yet to do throughput measurements, but my guess is those increase quite a lot as well. I will update with data on that as I go on.

All tests run, except for the APIApproval test as I have some suggestions on changes to make on the API side of things to simplify and modernize the client, and would love to have a discussion on that.

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

To bring the library up to modern .NET standards and to utilize some of the great work that has gone into performance improvements since .NET Core came along, some fundamental changes are needed on the API level.

Here is what I'd love to discuss as well:

• Generate *Async versions (and possibly remove non-Async methods where thread blocking is involved) of most operations, even though pipelining is not used, since the library can still take advantage of it to free up threads while waiting for IO.
• Replace the use of byte[] as parameters in most places (BasicPublish, Command, Frame for example) and replace them with [ReadOnly]Memory or [ReadOnly]Span where appropriate. Using that, allocations can be almost completely avoided as consumers will just be receiving windows into memory that can later be returned to a pool when work is completed.

[michaelklishin]

Thanks you! Those breaking public API changes, whatever we decide to proceed with, will have to wait until 7.0.

[stebet]

B.t.w, there are some bugs I've yet to iron out, and to do some code cleanups like remove the redundant test project I use to run the stress tests, and to do some more measurements, so don't hit that Merge button just yet.

Remaining bugs

• Code cleanups (remove redundant project)
• Make sure the APIApproval test runs.
• Deadlocks when running a publisher and consumer on the same connection.

[stebet]

Thanks you! Those breaking public API changes, whatever we decide to proceed with, will have to wait until 7.0.

Makes sense. Most of these changes should work fine with no changes to the API. Would it make sense to try to get *Async methods where appropriate into 6.0 though? What's your timeline for 6.0?

[michaelklishin]

There is no strict timeline but we have had delayed 6.0 several times already. I'd say March sounds acceptable.

[lukebakken]

@michaelklishin version 6.0 already has breaking API changes. If additional changes are ready to be merged and tested we should consider doing that now.

[lukebakken]

@stebet this needs to be rebased on master. Would you like me to do that?

[stebet]

Sure thing.

[bording]

This should not be conditioned.

[lukebakken]

That's part of the "rebase on master" work 😄

[stebet]

True, but there's no reason to include it for netstandard builds either as it's a noop there anyway. It's simpler to skip the condition though :)

[lukebakken]

FWIW with the condition the build fails in my Linux and OS X environment 🤷‍♂

[bording]

While I do think that moving forward to more modern stuff, like pipelines, spans, etc. is where this should go long-term, I'm not sure that bringing this into 6.0 is a good idea.

While the packages that have been added claim to support .NET Framework in various ways, a lot of these don't really work well on .NET Framework at all. The packages can often lead into binding redirect hell, sometimes so obscure they are unsolvable. Things like Span can be brought into .NET Framework in limited ways, but they don't have a lot of the perf benefits that they do when used on .NET Core.

Based on a lot of those things, I'm thinking it might be better to ship 6.0, and then for 7.0 if you want to bring this stuff in, drop support for .NET Framework entirely.

[lukebakken]

Interesting points @bording, thanks

[stebet]

Binding redirect issues were one of the reasons I pointed at 461 as minimum for 6.0. I know @mgravell has a lot of experience with maintaining a library using these packages for older versions of the .NET Framework (Pipelines.Sockets.Unofficial and StackExchange.Redis) so he might have some pointers there. Issues should be able to be ironed out in prerelease versions i.m.o.

B.t.w Span/Memory is slower on .NET Framework than .NET Core but it is by no means slow in general. The slicing/windowing functionality makes working with buffers so much easier and maintainable, and Pipelines, which makes heavy use of them, has already been very much battletested in StackExchange.Redis (which was the inspiration for making these changes here).

I'm happy to be discussing these things b.t.w, it's exactly what I wanted so pros/cons can be thought of to make decisions.

[bording]

Binding redirect issues were one of the reasons I pointed at 461 as minimum for 6.0.

Unfortunately, going to net461 doesn't solve any of those. Even going to net48 doesn't 100% solve them.

[stebet]

Binding redirect issues were one of the reasons I pointed at 461 as minimum for 6.0.

Unfortunately, going to net461 doesn't solve any of those. Even going to net48 doesn't 100% solve them.

I know, but it does make them easier. Here is post with links to many of the discussions had to resolve those issues (although not perfect): https://stu.dev/dotnet-framework-support-for-dotnetnet-standard-2-0/

[michaelklishin]

Let's ship 6.0 and drop classic .NET framework support in 7.0. That means we will have to maintain two major branches for a pretty long period of time but that's fine. We've been doing this for a while anyway.

[lukebakken]

@stebet if you have a second to check your changes rebased on master, I have them pushed to this branch.

[michaelklishin]

Now that 6.0 is out and there's a 6.x branch we proceed with this PR. @stebet may I ask you to rebase it on top of latest master?

[stebet]

Now that 6.0 is out and there's a 6.x branch we proceed with this PR. @stebet may I ask you to rebase it on top of latest master?

Yeah, I might even just abandon this PR and start a new one as a lot of the original work in this PR has been improved in the 6.0 work, sans the Pipelines stuff. I'll be looking at that soon, have some work stuff to finish these days :)

[stebet]

Replaced by a lot of previous work and finally with #949