Speeding up shortstr/longstr (de)serialization. #985
Conversation
| } | ||
| catch (ArgumentException) | ||
| { | ||
| return ThrowArgumentOutOfRangeException(val, maxLength); |
There was a problem hiding this comment.
Would we loose perf if we preserved the original exception as an inner for more transparency?
There was a problem hiding this comment.
Don't think so. We'd have to replace the maxLength parameter since there's a missing overload for ArgumentOutOfRangeException to accept both an inner exception as well as setting the value.
|
Out of curiosity why are you not using the Benchmark Baseline for the comparison? |
I didn't want to copy the old logic over to the new one since I'm manually comparing runs between different branches so there's no straightforward way to add the baseline parameter. There is a tool that @adamsitnik has mentioned that can compare benchmarkdotnet runs but I haven't tried it yet. See here: dotnet/BenchmarkDotNet#973 Hopefully it'll become a global tool soon to make this easier :) |
We don't have such plans for the near future, but it's available here if someone wants to use it |
|
@stebet any reason not to backport this all the way to |
Don't think so :) You'll just need to remove the |
| using RabbitMQ.Client.Exceptions; | ||
| using RabbitMQ.Util; | ||
|
|
||
| namespace RabbitMQ.Client.Impl | ||
| { | ||
| internal static class WireFormatting | ||
| { | ||
| private static UTF8Encoding UTF8 = new UTF8Encoding(); |
There was a problem hiding this comment.
What made you add this change? I've been wondering for some time now whether it's better to call Encoding.UTF8 vs a static instance. (Btw, should be readonly!)
From source I see they're not even the same instance 🤷♂️
=> https://source.dot.net/#System.Private.CoreLib/Encoding.cs,a10eb90a3d884500
=> https://source.dot.net/#System.Private.CoreLib/UTF8Encoding.Sealed.cs,98c4a50ea9fbaa13,references
There was a problem hiding this comment.
The Encoding.UTF8 returns the base class instead of an UTF8Encoding. Maybe there isn't any difference but this way we're at least pretty certain to avoid any callvirt calls. I saw this method used in other high-perf libraries as well.
There was a problem hiding this comment.
Quick test: (Code at the end)
GetBytes
| Method | value | Mean | Error | StdDev | Ratio | Code Size |
|---|---|---|---|---|---|---|
| Original | MyString | 21.67 ns | 0.887 ns | 0.049 ns | 1.00 | 230 B |
| Static | MyString | 21.40 ns | 0.710 ns | 0.039 ns | 0.99 | 230 B |
| StaticSealed | MyString | 20.85 ns | 0.800 ns | 0.044 ns | 0.96 | 230 B |
GetString
| Method | value | Mean | Error | StdDev | Ratio | Code Size |
|---|---|---|---|---|---|---|
| Original | Byte[8] | 22.11 ns | 2.246 ns | 0.123 ns | 1.00 | 166 B |
| Static | Byte[8] | 22.25 ns | 1.799 ns | 0.099 ns | 1.01 | 166 B |
| StaticSealed | Byte[8] | 22.11 ns | 1.126 ns | 0.062 ns | 1.00 | 166 B |
There's not much difference, only thing I see is a slight benefit for static sealed.
Benchmark code
using System.Collections.Generic;
using System.Text;
using BenchmarkDotNet.Attributes;
namespace Benchmarks.WireFormatting
{
[ShortRunJob]
[DisassemblyDiagnoser]
public class EncodingBenchmarks
{
private static readonly UTF8Encoding UTF8 = new UTF8Encoding();
private static readonly UTF8Encoding UTF8Sealed = (UTF8Encoding)Encoding.UTF8;
public static IEnumerable<byte[]> ByteSource => new[] {Encoding.UTF8.GetBytes("MyString")};
/*
[Benchmark(Baseline = true)]
[Arguments("MyString")]
public byte[] Original(string value)
{
return Encoding.UTF8.GetBytes(value);
}
[Benchmark]
[Arguments("MyString")]
public byte[] Static(string value)
{
return UTF8.GetBytes(value);
}
[Benchmark]
[Arguments("MyString")]
public byte[] StaticSealed(string value)
{
return UTF8Sealed.GetBytes(value);
}
*/
[Benchmark(Baseline = true)]
[ArgumentsSource(nameof(ByteSource))]
public string Original(byte[] value)
{
return Encoding.UTF8.GetString(value);
}
[Benchmark]
[ArgumentsSource(nameof(ByteSource))]
public string Static(byte[] value)
{
return UTF8.GetString(value);
}
[Benchmark]
[ArgumentsSource(nameof(ByteSource))]
public string StaticSealed(byte[] value)
{
return UTF8Sealed.GetString(value);
}
}
}
| #if NETCOREAPP | ||
| public static int WriteLongstr(Span<byte> span, ReadOnlySpan<char> val) | ||
| { | ||
| int bytesWritten = val.IsEmpty ? 0 : UTF8.GetBytes(val, span.Slice(4)); |
There was a problem hiding this comment.
If we special cased the WriteLongstr for empty values, shouldn't we also do it for the read? => https://github.com/rabbitmq/rabbitmq-dotnet-client/pull/981/files#diff-24820bc65c94384792cf645221a57c741a49e59275b2b0eb34f09167de064bb3R143
Proposed Changes
Code cleanups and optimizations for shortstr/longstr (de)serialization.
This provides a pretty hefty improvement to almost all messages sent and some improvements to most messages read as well.
Types of Changes
Checklist
CONTRIBUTING.mddocumentBenchmark results