simplify IncomingCommand.ReturnBuffers handling

[bollhals]

Proposed Changes

Simplifies the handling of the ReturnBuffers by calling it at one central place, instead of many places.
-> This gets rid of the try-finally(return_buffer) pattern, as it is no longer needed. If there is an exception, the buffer will not be returned, but no real harm is done. It is not an uncommon pattern.

In addition, this commit seals the Continuation types (in accordance to CA1852)

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)
• Refactoring / performance change

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

[paulomorgado]

Simplifies the handling of the ReturnBuffers by calling it at one central place, instead of many places. -> This gets rid of the try-finally(return_buffer) pattern, as it is no longer needed. If there is an exception, the buffer will not be returned, but no real harm is done. It is not an uncommon pattern.

Why?

[bollhals]

Simplifies the handling of the ReturnBuffers by calling it at one central place, instead of many places. -> This gets rid of the try-finally(return_buffer) pattern, as it is no longer needed. If there is an exception, the buffer will not be returned, but no real harm is done. It is not an uncommon pattern.

Why?

what exactly do you mean?

[paulomorgado]

Simplifies the handling of the ReturnBuffers by calling it at one central place, instead of many places. -> This gets rid of the try-finally(return_buffer) pattern, as it is no longer needed. If there is an exception, the buffer will not be returned, but no real harm is done. It is not an uncommon pattern.

Why?

what exactly do you mean?

Why is this simplification required?

[bollhals]

Why is this simplification required?

It is not "required", but as with many simplifications, it's less likely to make a mistake (e.g. not returning the buffer), is less code to maintain and easier to read an understand.

In addition, not needing the try-finally potentially enables more inlining / slightly less work needing to be done.

Then the move of the starttime property means that only the continuation that needs this property will pay for setting the value, not all others as well that never used the value.

And last but not least, the sealing of internal types is another small performance related change, that a Analyzer for the newer frameworks would flag.

[paulomorgado]

In addition, not needing the try-finally potentially enables more inlining / slightly less work needing to be done.

I'm just curious. Do you have any traces or benchmarks that show this?

And last but not least, the sealing of internal types is another small performance related change, that a Analyzer for the newer frameworks would flag.

This is unrelated to try/finally.

[bollhals]

In addition, not needing the try-finally potentially enables more inlining / slightly less work needing to be done.

I'm just curious. Do you have any traces or benchmarks that show this?

Try-catch blocking inlining can be seen here or here.

Whether or not it actually improves things based on this changes is a bit harder to prove, which is why I didn't even try. Improvements will be minimal compared to the time the other things take and to my knowledge, there's no isolated benchmark in this area (CommandAssembler until Dispatcher)

[lukebakken]

Thanks @bollhals, I will review this today or tomorrow.

[lukebakken]

@bollhals thanks, this is a nice improvement.

[lukebakken]

I will merge once CI passes. Thanks!