8.0: implement BasicPublishMemory

[bollhals]

Proposed Changes

fixes #972

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

Also see perf benchmarks at #972 (comment)

[michaelklishin]

Copying and pasting some benchmark results from @bollhals shared in #972:

Update from a local running version on this:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated	Code Size
BasicPublishWrite	41.3951 ns	0.8661 ns	0.0475 ns	-	-	-	-	386 B
BasicPublishSize	18.5197 ns	0.1749 ns	0.0096 ns	-	-	-	-	178 B
BasicPublishMemoryWrite	14.3104 ns	0.6103 ns	0.0334 ns	-	-	-	-	775 B
BasicPublishMemorySize	0.8075 ns	0.0310 ns	0.0017 ns	-	-	-	-	39 B

Obviously the gain increases the larger the string (Exchange / RoutingKey) is. (Benchmark is with "Exchange_OR_RoutingKey").

Another benchmark to serialize into the 3 frames of a message:

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated	Code Size
BasicPublish	223.9 ns	13.50 ns	0.74 ns	1.00	-	-	-	-	2504 B
BasicPublishMemory	182.1 ns	7.83 ns	0.43 ns	0.81	-	-	-	-	2518 B

It my does not seem huge, but it increases the throughput on the most relevant hotpath.
I'll setup a PR for 7.0 in the next days.

[michaelklishin]

This also cannot be backported to 7.x because of the accumulated delta. We need a separate version of this for 7.x or reduce the delta by backporting earlier PRs.

[danielmarbach]

@bollhals what is the benefit of exposing this as method parameters vs exposing the CLR type ReadonlyMemory? At first glance it looks like only convenience right? So would it make sense to make the public Api just accepting the CLR type instead? I'm asking because even with cached string the caller of the API needs to be concerned with caching the instances so the value of this class is really only abstracting the encoding get bytes which I find not enough to expose a custom type. A middle ground could be to expose the type but make it convertible to readonly memory while still switching the public Api to readonly memory

[bollhals]

I hope my explanation is reasonable and what I wrote is understandable 😄
This was one thing which I was not 100% certain what the best option might be. In the end I've exposed it as CachedString due to:

• You should only use this method if you actually cache the string (Otherwise you have worse performance)
• Exposing ROM enables customer to send non UTF8 values, which might cause more harm than good. (In which case would you have a valid ROM but not a string? (Stackalloc is also not possible since it's Memory not Span))
• In the future (comment from other PR) we could start returning the CachedString in other methods like BasicConsume or QueueDeclare, which then again could directly be used again in follow up methods for better performance.
• CachedString ist more clearly explaining the intent of the method than ROM + comments explaining it has to be a UTF8 sting

Let's switch it around, what impact would you get from exposing just ROM?
Which use cases could you see which taking a ROM is preferable than CachedString?

The intent of this API is better performance due to not having to encode the same string over and over again which is done by caching the encoded value, which implies that the user is caching the value.

[danielmarbach]

Ok got it. These are really the strongest reason to expose a custom type

• You should only use this method if you actually cache the string (Otherwise you have worse performance)
• Exposing ROM enables customer to send non UTF8 values, which might cause more harm than good. (In which case would you have a valid ROM but not a string? (Stackalloc is also not possible since it's Memory not Span))

Which use cases could you see which taking a ROM is preferable than CachedString?

My thoughts were comming from the experience that when you have a .NET type available to expose a .NET type where it makes sense because it is one API less you have to maintain and potentially evolve. But like mentioned above the two major bulllets seem to indicate a strong custom type makes sense.

Regarding

• CachedString ist more clearly explaining the intent of the method than ROM + comments explaining it has to be a UTF8 string

Could we add some validation into the constructor of that type or at least call it CachedUTF8String or something similar?

[bollhals]

CachedString ist more clearly explaining the intent of the method than ROM + comments explaining it has to be a UTF8 string

Could we add some validation into the constructor of that type or at least call it CachedUTF8String or something similar?

What kind of validation?
Sure we can rename it, but I'm not favoring UTF8 in the name as the type kind of abstracting it away so you don't need to care what it is. Adding it in the name makes it clear what it will use, but for what purpose?