Structify the Framing.Impl methods

[bollhals]

Proposed Changes

The main focus of this change was to change all classes of Framing.Impl to be structs.

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

Allocation before (Master)

Allocations now

Side effect is a reduction of the dll size of 11KB

Performance is hard to measure, the test app we otherwise use won't show a meaningful difference, so I used to benchmarks provided by @JanEggers:

2nd Line is always this PR

New Connection with 10000 messages

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
Publish_Hello_World	190.6 ms	3.71 ms	6.49 ms	1.00	1000.0000	333.3333	-	4.97 MB
Publish_Hello_World	187.8 ms	3.75 ms	3.86 ms	1.00	666.6667	-	-	4.12 MB

Existing Connection but with 1000 messages

Method	Mean	Error	StdDev	Ratio	Gen 0	Gen 1	Gen 2	Allocated
Publish_Hello_World	22.37 ms	0.429 ms	0.511 ms	1.00	93.7500	-	-	508.24 KB
Publish_Hello_World	21.41 ms	0.408 ms	0.362 ms	1.00	62.5000	-	-	420.11 KB

[stebet]

I really like the changes in this PR :) It simplifies a lot of code.

[bollhals]

I really like the changes in this PR :) It simplifies a lot of code.

Thanks for your review!

[bollhals]

My plan would be to merge this one before the first 8.0 PRs, as this might be also relevant for backporting to 6.x which will be simpler before 8.0 PR hitting master.

I'll update the code in the next days and trying to get this in a mergable state.

[bollhals]

My plan would be to merge this one before the first 8.0 PRs, as this might be also relevant for backporting to 6.x which will be simpler before 8.0 PR hitting master.

I'll update the code in the next days and trying to get this in a mergable state.

Done, rebased to master and fixed all failing tests.

[stebet]

Will try to review this soon. A bit busy with a newborn a.t.m :)

[bollhals]

Will try to review this soon. A bit busy with a newborn a.t.m :)

congrats =)

[bollhals]

rebased to master. this is ready for review

[bollhals]

Updated with latest master and ran new benchmarks & updated initial post.

[bollhals]

@stebet @michaelklishin Can we get this reviewed? Otherwise this is going to end up in another conflicting state with master due to the next PR (like #1024).

[bollhals]

Rebased again against master and resolved all conflicts. Would you mind taking a look so we finally can get this done? @stebet @michaelklishin

[stebet]

Rebased again against master and resolved all conflicts. Would you mind taking a look so we finally can get this done? @stebet @michaelklishin

Will do

[bollhals]

rebased to master again. ready for final reviews

[bollhals]

ping

[bollhals]

@michaelklishin Can we finish this PR please?

[bollhals]

🚨 Anything I can help to get this done? 🚨

PS: What is the plan for 7.0?

[stebet]

After the latest reviews I'm all good for my part :)

[bollhals]

Who could I ping to get this done? :) feeling a bit left out here

[michaelklishin]

@bollhals sorry, there were significant changes on our team in the last few months (mostly positive) and .NET client has slipped under the radar. Nearly all focus is on shipping RabbitMQ 3.9 and a commercial product based on it.