Reader / Writer improvements

[bollhals]

Proposed Changes

Simplified a few things around frames and reader / writer for MethodArgument & Properties. This results in a small gain on the hot paths and prepares the code for full "spanification" on the read / write actions. (See comment)

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

[bollhals]

MethodArgumentReader/Writer
WriteArgument => BasicPublish operation with new BasicPublish(0, "Exchange", "RoutingKey", false, true);
ReadArgument => new BasicDeliver("ConsumerTag", 4000, true, "Exchange", "RoutingKey");

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
WriteArgumentsTo	77.89 ns	0.543 ns	0.030 ns	-	-	-	-
WriteArgumentsTo_Old	83.49 ns	3.107 ns	0.170 ns	-	-	-	-
ReadArgumentsFrom	144.33 ns	7.857 ns	0.431 ns	0.0288	-	-	136 B
ReadArgumentsFrom_Old	145.81 ns	27.038 ns	1.482 ns	0.0288	-	-	136 B

ContentHeaderPropertyReader/Writer
Read/Write BasicProperties =>

new BasicProperties
            {
                ContentType = "application/json",
                CorrelationId = Guid.NewGuid().ToString(),
            };

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
WritePropertiesTo	105.9 ns	1.26 ns	0.07 ns	-	-	-	-
WritePropertiesTo_Old	106.0 ns	0.61 ns	0.03 ns	-	-	-	-
ReadPropertiesFrom	146.5 ns	6.57 ns	0.36 ns	0.0322	-	-	152 B
ReadPropertiesFrom_Old	146.0 ns	5.73 ns	0.31 ns	0.0322	-	-	152 B

[bollhals]

The property case doesn't improve due to the small amount of "non bit" writes. But at least it isn't worse.

At the moment most of the time is spent in the encoding of strings / special handling due to the required array in the Full Framework and .Net Standard 2.0.
We could use unsafe code here with spans by using this method.
Question is do we already want to go full span for all read / write methods and therefore use this method or do we want to wait for .Net Standard 2.1?

[bollhals]

The full span does also improve the performance quite a bit for these paths (and simplify the code) but I can't share numbers as I did only a small test. And before doing all the work, I'd like to know whether this is something that would be accepted.

[michaelklishin]

@bollhals what does "going full span" mean for the supported .NET version matrix?

[bollhals]

@bollhals what does "going full span" mean for the supported .NET version matrix?

We always use spans for any type -> byte conversions (e.g. NetworkOrderDeserializer / NetworkOrderSerializer) except for strings, due to the fact that there is no framework encoding method for spans below .Net Standard 2.1 (Here's the one for NS2.1).

By "going full span" I mean that we'd not be using Memory anymore in the conversion methods. But this is only possible if we can use Spans for strings.

One possibility to acheive this for < NS2.1 is to use code like this

fixed (char* chars = theStringValue)
fixed (byte* bytes = &span.GetPinnableReference())
{
    Encoding.GetBytes(chars, StringSize, bytes, span.length);
}

but it requires the "unsafe" switch enabled.

Why would we want to do this?
Currently we use the workaround to retrieve the backing array through MemoryMarshal.TryGetArray(stringSlice, out ArraySegment<byte> segment) which is ~20% of the cost of short strings to byte encoding. (dependens on string size, content, ...)
So by getting rid of this workaround, there is considerably less overhead for all (de)serialization methods that use strings (almost of all of them do)

[michaelklishin]

Thanks for the detailed explanation. My question was about the .NET [Framework | Core] versions we would have to require if we adopt .NET Standard 2.1.

Per this .NET standard implementation matrix, there is no .NET Framework version implementing .NET Standard 2.1, and the minimum .NET Core version supporting it is 3.0.

.NET Standard 2.1 announcement post suggests that .NET Framework may never support this version. If that's the case, this is likely a no-go for most libraries to require it at this time.

[michaelklishin]

From the above post:

Library authors who need to support .NET Framework customers should stay on .NET Standard 2.0

[bollhals]

You're correct about the frameworks. NS2.1 would target only .Net Core 3.0+, while NS2.0 supports (still officially supported) .Net 4.61 & .Net Core 2.1.

I was under the impression that for 7.0 the support for .NET Framework might be dropped?

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.
Link

Also the posted workaround also works for NS2.0 (but requires the unsafe switch), so this would still be possible, but whether or not to use the unsafe switch, is something that's needs to be decided upon.

[bollhals]

Why would we want to do this?
Currently we use the workaround to retrieve the backing array through MemoryMarshal.TryGetArray(stringSlice, out ArraySegment<byte> segment) which is ~20% of the cost of short strings to byte encoding. (dependens on string size, content, ...)
So by getting rid of this workaround, there is considerably less overhead for all (de)serialization methods that use strings (almost of all of them do)

I have made some tests locally with the described workaround for the current supported platforms.
It looks like the benchmarks shown in this PR could be improved by about 30-50% (depending on how many strings were used).

[michaelklishin]

Fair enough, if the community believes such a release would have enough users to justify, we can consider it.

[stebet]

Considering that .NET Framework will probably not see any updates except for security updates, and that MS is pushing full force for .NET 5 as it's next release, my personal opinion is that 7.0 should target .NET Standard 2.1, and 6.X should be the last .NET Framework supported release. This is of course always a debatable thing, but given that for example some C# 8.0 language features can only be used in .NET Core it's obvious that .NET Framework is just on life support until it's support lifetime runs out, it seems like a good cutoff point to leave behind legacy code and move the codebase forward.

At that point it'd make sense to create a separate 6.X branch for reliability/security updates and possibly minor features.

My full-async PR (which I'm hoping to submit for your review in the next few days) targets only .NET Standard 2.1 and the latest .NET Core LTS version (3.1) is already .NET Standard 2.1 ready.

[stebet]

Also, regarding strings, there is work ongoing in the .NET Core runtime to make a native UTF8 String type which is ideal for a library like this. I don't know exactly what the progress is there or if it'll make it for .NET 5. Perhaps @GrabYourPitchforks has an update on the progress there.

dotnet/corefxlab#2350

[bollhals]

Updated benchmarks:

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
WriteArgumentsTo	41.12 ns	1.031 ns	0.056 ns	-	-	-	-
WriteArgumentsTo_Old	83.49 ns	3.107 ns	0.170 ns	-	-	-	-
ReadArgumentsFrom	100.87 ns	22.954 ns	1.258 ns	0.0288	-	-	136 B
ReadArgumentsFrom_Old	145.81 ns	27.038 ns	1.482 ns	0.0288	-	-	136 B

Method	Mean	Error	StdDev	Gen 0	Gen 1	Gen 2	Allocated
WritePropertiesTo	75.38 ns	1.237 ns	0.068 ns	-	-	-	-
WritePropertiesTo_Old	106.0 ns	0.61 ns	0.03 ns	-	-	-	-
ReadPropertiesFrom	109.99 ns	11.165 ns	0.612 ns	0.0323	-	-	152 B
ReadPropertiesFrom_Old	146.0 ns	5.73 ns	0.31 ns	0.0322	-	-	152 B

[lukebakken]

There is some extra whitespace that needs to be removed. Also, is this PR 100% complete? I remember reading an earlier comment that suggested this is only part of the work, but I can't seem to find it 🤔

[bollhals]

There is some extra whitespace that needs to be removed. Also, is this PR 100% complete? I remember reading an earlier comment that suggested this is only part of the work, but I can't seem to find it 🤔

The "only part of the work"-thing was the part about whether or not use the unsafe switch and thus use the span everywhere in the wireformatting. I've done that in d0d3032. So this PR is now complete and ready for review.

[danielmarbach]

Considering that .NET Framework will probably not see any updates except for security updates, and that MS is pushing full force for .NET 5 as it's next release, my personal opinion is that 7.0 should target .NET Standard 2.1, and 6.X should be the last .NET Framework supported release. This is of course always a debatable thing, but given that for example some C# 8.0 language features can only be used in .NET Core it's obvious that .NET Framework is just on life support until it's support lifetime runs out, it seems like a good cutoff point to leave behind legacy code and move the codebase forward.

At that point it'd make sense to create a separate 6.X branch for reliability/security updates and possibly minor features.

what is the benefit of targeting .NET Standard 2.1? As it stands my understanding is that .NET Standard 2.1 is basically more or less a dead end. With the introduction of .NET 5 the standards have been merged into .NET 5 TFM. But that being said .NET 5 will not even be the LTS version. Only .NET 6 will be the LTS version. So from a consumer perspective nobody but bleeding edge contributors really care in what language version the rabbitmq client is written or do they?

I would guess from a Pivotal / VMWare perspective given the broad adoption of the rabbitmq client having a broader set of compatibility makes more sense instead of too early corner the client in "just because performance" (of course this is drastically and potentially unfairly simplified for the sake of my argument ;) ). Yes .NET 4.8 is basically the last version and only .NET 4.7.2 and higher properly supports .NET Standard 2.0. Even though .NET Standard 2.0 is a little bit of a disaster and the System packages used can cause headaches and don't offer the true perf one would expect (and sometimes even cause binding redirect issues) wouldn't it from an adoption perspective as well as long tail perspective make sense to stay a bit longer on the .NET Standard 2.0 train (with or without dual targeting to .NET Framework)?

[bollhals]

I've created an issue #867 to separate the discussion about the supported frameworks and the content of this PR.

[bollhals]

squashed and rebased against master

[lukebakken]

Superseded by #878. Thanks @bollhals