Minor WireFormatting code cleanups and optimizations. #981

stebet · 2020-11-26T12:40:19Z

Proposed Changes

Code cleanups to simplify/optimize code for WireFormatting.

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

A simple change, performance benchmarks shows considerable improvements to serialization times for empty/null strings and objects. Getting rid of "unsafe" code for the netcoreapp target.

Before:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.20262
Intel Core i7-10700 CPU 2.90GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=5.0.100
  [Host]   : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
  ShortRun : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT

Job=ShortRun  IterationCount=3  LaunchCount=1  
WarmupCount=3

Type	Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
WireFormatting_Read_BasicAck	ReadFromSpan	3.905 ns	0.4919 ns	0.0270 ns	1.00	0.00	0.0038	-	-	32 B
WireFormatting_Read_BasicDeliver	ReadFromSpan	11.831 ns	1.2428 ns	0.0681 ns	3.03	0.03	0.0067	-	-	56 B
WireFormatting_Read_BasicProperties	ReadFromSpan	63.368 ns	4.6002 ns	0.2522 ns	16.23	0.10	0.0229	-	-	192 B
WireFormatting_Read_ChannelClose	ReadFromSpan	7.086 ns	0.9426 ns	0.0517 ns	1.81	0.03	0.0038	-	-	32 B
WireFormatting_Write_BasicAck	WriteArgumentsTo	1.899 ns	0.0449 ns	0.0025 ns	0.49	0.00	-	-	-	-
WireFormatting_Write_BasicDeliver	WriteArgumentsTo	34.764 ns	3.9156 ns	0.2146 ns	8.90	0.08	-	-	-	-
WireFormatting_Write_BasicProperties	WritePropertiesToSpan	32.445 ns	0.2154 ns	0.0118 ns	8.31	0.06	-	-	-	-
WireFormatting_Write_ChannelClose	WriteArgumentsTo	13.923 ns	1.5404 ns	0.0844 ns	3.57	0.03	-	-	-	-

After:

BenchmarkDotNet=v0.12.1, OS=Windows 10.0.20262
Intel Core i7-10700 CPU 2.90GHz, 1 CPU, 16 logical and 8 physical cores
.NET Core SDK=5.0.100
  [Host]   : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT
  ShortRun : .NET Core 3.1.9 (CoreCLR 4.700.20.47201, CoreFX 4.700.20.47203), X64 RyuJIT

Job=ShortRun  IterationCount=3  LaunchCount=1  
WarmupCount=3

Type	Method	Mean	Error	StdDev	Ratio	RatioSD	Gen 0	Gen 1	Gen 2	Allocated
WireFormatting_Read_BasicAck	ReadFromSpan	3.889 ns	0.5254 ns	0.0288 ns	1.00	0.00	0.0038	-	-	32 B
WireFormatting_Read_BasicDeliver	ReadFromSpan	11.002 ns	0.4028 ns	0.0221 ns	2.83	0.02	0.0067	-	-	56 B
WireFormatting_Read_BasicProperties	ReadFromSpan	58.720 ns	11.1702 ns	0.6123 ns	15.10	0.24	0.0229	-	-	192 B
WireFormatting_Read_ChannelClose	ReadFromSpan	6.678 ns	0.1788 ns	0.0098 ns	1.72	0.02	0.0038	-	-	32 B
WireFormatting_Write_BasicAck	WriteArgumentsTo	1.820 ns	0.0432 ns	0.0024 ns	0.47	0.00	-	-	-	-
WireFormatting_Write_BasicDeliver	WriteArgumentsTo	4.797 ns	0.3201 ns	0.0175 ns	1.23	0.00	-	-	-	-
WireFormatting_Write_BasicProperties	WritePropertiesToSpan	34.957 ns	0.8850 ns	0.0485 ns	8.99	0.06	-	-	-	-
WireFormatting_Write_ChannelClose	WriteArgumentsTo	3.786 ns	0.1707 ns	0.0094 ns	0.97	0.01	-	-	-	-

projects/RabbitMQ.Client/client/impl/WireFormatting.cs

stebet · 2020-11-26T15:24:23Z

Updating comments after rebase.

michaelklishin

I get a test failure in RabbitMQ.Client.Unit.TestFieldTableFormatting on this branch:

RABBITMQ_RABBITMQCTL_PATH=rabbitmqctl dotnet test projects/Unit --filter "FullyQualifiedName~RabbitMQ.Client.Unit.TestFieldTableFormatting

stebet · 2020-11-26T17:51:39Z

Yeah, I'll fix it in a couple hours, know what the problem is :)

projects/RabbitMQ.Client/client/impl/WireFormatting.cs

bollhals · 2020-11-26T20:53:16Z

I like the change 👍 didn't yet think about it yet now that we have the new APIs available.

One question though: How likely is a null / empty string being passed for real world examples?

projects/RabbitMQ.Client/client/impl/WireFormatting.cs

stebet · 2020-11-26T22:18:43Z

I like the change 👍 didn't yet think about it yet now that we have the new APIs available.

One question though: How likely is a null / empty string being passed for real world examples?

Likely given that it's a valid Exchange for example (the default exchange) :) also, people initializing arrays or lists, aiming to put stuff into and sometimes omitting data, so I think it's a valid use-case.

bollhals · 2020-11-27T01:05:41Z

projects/RabbitMQ.Client/client/impl/WireFormatting.cs

+            return WriteShortStrImpl(span, val, maxLength);
+        }
+
+        private static int WriteShortStrImpl(Span<byte> span, string val, int maxLength)


Now I'm hooked =) Was it beneficial to extract this part in it's own method? (I assume this won't be inlined)

A little. Anything containing exception handling won't be inlined. You can use the InliningDiagnoser from benchmarksdotnet to see inlining errors. :)

Example using this attribute [BenchmarkDotNet.Diagnostics.Windows.Configs.InliningDiagnoser(true, true)]

-------------------- WireFormatting_ReadWritePrimitives.WriteDecimal: ShortRun(IterationCount=3, LaunchCount=1, WarmupCount=3) -------------------- Inliner: Benchmarks.WireFormatting.WireFormatting_ReadWritePrimitives.WriteDecimal - instance int32 () Inlinee: System.ThrowHelper.ThrowArgumentOutOfRangeException - void () Fail Reason: does not return -------------------- Inliner: Benchmarks.WireFormatting.WireFormatting_ReadWritePrimitives.WriteDecimal - instance int32 () Inlinee: System.Decimal.GetBits - int32[] (value class System.Decimal) Fail Reason: unprofitable inline -------------------- Inliner: Benchmarks.WireFormatting.WireFormatting_ReadWritePrimitives.WriteDecimal - instance int32 () Inlinee: RabbitMQ.Client.Impl.WireFormatting.ThrowWireFormattingException - void (value class System.Decimal) Fail Reason: unprofitable inline -------------------- Inliner: Benchmarks.WireFormatting.WireFormatting_ReadWritePrimitives.WriteDecimal - instance int32 () Inlinee: System.ThrowHelper.ThrowArgumentOutOfRangeException - void () Fail Reason: does not return -------------------- Inliner: Benchmarks.WireFormatting.WireFormatting_ReadWritePrimitives.WriteDecimal - instance int32 () Inlinee: System.ThrowHelper.ThrowInvalidTypeWithPointersNotSupported - void (class System.Type) Fail Reason: unprofitable inline -------------------- Inliner: Benchmarks.WireFormatting.WireFormatting_ReadWritePrimitives.WriteDecimal - instance int32 () Inlinee: System.ThrowHelper.ThrowArgumentOutOfRangeException - void (value class System.ExceptionArgument) Fail Reason: does not return --------------------

I'm surprised that it it is faster, not quite get why...
WriteShortstr2 is fully inlined without the extra method.

Method Length Mean Error StdDev Ratio RatioSD Code Size

WriteShortstr 0 0.7690 ns 0.0710 ns 0.0039 ns 1.00 0.00 115 B

WriteShortstr2 0 2.6724 ns 0.1118 ns 0.0061 ns 3.48 0.02 405 B

WriteShortstr 1 12.9989 ns 0.0236 ns 0.0013 ns 1.00 0.00 364 B

WriteShortstr2 1 13.3813 ns 0.5709 ns 0.0313 ns 1.03 0.00 405 B

WriteShortstr 10 13.8706 ns 1.0501 ns 0.0576 ns 1.00 0.00 364 B

WriteShortstr2 10 15.4745 ns 0.5192 ns 0.0285 ns 1.12 0.01 405 B

WriteShortstr 100 21.8589 ns 0.5577 ns 0.0306 ns 1.00 0.00 364 B

WriteShortstr2 100 21.0526 ns 0.1686 ns 0.0092 ns 0.96 0.00 405 B

WriteShortstr 255 28.5284 ns 0.9557 ns 0.0524 ns 1.00 0.00 364 B

WriteShortstr2 255 29.3146 ns 4.7681 ns 0.2614 ns 1.03 0.01 405 B

What does WriteShortstr2 look like?

Same as the other but without the call to the impl method

That doesn't inline actually :) It refuses to do so because of the try/catch which never inlines.

Is this still the case in the newer frameworks? (I've not checked the disassembly in that detail.)

stebet · 2020-11-27T10:20:22Z

This is done now I think :)

stebet · 2020-12-10T08:43:22Z

I'm going to abandon this PR in favour of other smaller PRs that are easier to review and compare :)

sungam3r reviewed Nov 26, 2020

View reviewed changes

projects/RabbitMQ.Client/client/impl/WireFormatting.cs Outdated Show resolved Hide resolved

sungam3r reviewed Nov 26, 2020

View reviewed changes

projects/RabbitMQ.Client/client/impl/WireFormatting.cs Outdated Show resolved Hide resolved

stebet force-pushed the code_cleanups branch from ce66c43 to 4ef4266 Compare November 26, 2020 15:14

stebet requested review from danielmarbach, lukebakken and michaelklishin November 26, 2020 15:40

michaelklishin requested changes Nov 26, 2020

View reviewed changes

danielmarbach reviewed Nov 26, 2020

View reviewed changes

projects/RabbitMQ.Client/client/impl/WireFormatting.cs Show resolved Hide resolved

danielmarbach reviewed Nov 26, 2020

View reviewed changes

projects/RabbitMQ.Client/client/impl/WireFormatting.cs Show resolved Hide resolved

bollhals reviewed Nov 26, 2020

View reviewed changes

projects/RabbitMQ.Client/client/impl/WireFormatting.cs Outdated Show resolved Hide resolved

bollhals reviewed Nov 26, 2020

View reviewed changes

projects/RabbitMQ.Client/client/impl/WireFormatting.cs Show resolved Hide resolved

bollhals reviewed Nov 26, 2020

View reviewed changes

projects/RabbitMQ.Client/client/impl/WireFormatting.cs Show resolved Hide resolved

stebet requested a review from michaelklishin November 26, 2020 23:27

WireFormatting code cleanups and optimizations.

d7c5663

stebet force-pushed the code_cleanups branch from ef8720a to d7c5663 Compare November 26, 2020 23:48

Cleaning up code to help with inlining.

e7125ab

bollhals reviewed Nov 27, 2020

View reviewed changes

stebet closed this Dec 10, 2020

Method	Length	Mean	Error	StdDev	Ratio	RatioSD	Code Size
WriteShortstr	0	0.7690 ns	0.0710 ns	0.0039 ns	1.00	0.00	115 B
WriteShortstr2	0	2.6724 ns	0.1118 ns	0.0061 ns	3.48	0.02	405 B

WriteShortstr	1	12.9989 ns	0.0236 ns	0.0013 ns	1.00	0.00	364 B
WriteShortstr2	1	13.3813 ns	0.5709 ns	0.0313 ns	1.03	0.00	405 B

WriteShortstr	10	13.8706 ns	1.0501 ns	0.0576 ns	1.00	0.00	364 B
WriteShortstr2	10	15.4745 ns	0.5192 ns	0.0285 ns	1.12	0.01	405 B

WriteShortstr	100	21.8589 ns	0.5577 ns	0.0306 ns	1.00	0.00	364 B
WriteShortstr2	100	21.0526 ns	0.1686 ns	0.0092 ns	0.96	0.00	405 B

WriteShortstr	255	28.5284 ns	0.9557 ns	0.0524 ns	1.00	0.00	364 B
WriteShortstr2	255	29.3146 ns	4.7681 ns	0.2614 ns	1.03	0.01	405 B

Minor WireFormatting code cleanups and optimizations. #981

Minor WireFormatting code cleanups and optimizations. #981

Uh oh!

Conversation

stebet commented Nov 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Types of Changes

Checklist

Further Comments

Uh oh!

Uh oh!

Uh oh!

stebet commented Nov 26, 2020

Uh oh!

michaelklishin left a comment

Choose a reason for hiding this comment

Uh oh!

stebet commented Nov 26, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bollhals commented Nov 26, 2020

Uh oh!

Uh oh!

stebet commented Nov 26, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stebet commented Nov 27, 2020

Uh oh!

stebet commented Dec 10, 2020

Uh oh!

Uh oh!

stebet commented Nov 26, 2020 •

edited

Loading