Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

introduce struct BasicProperties for basicPublish #1096

Merged
merged 2 commits into from
Dec 27, 2021

Conversation

bollhals
Copy link
Contributor

@bollhals bollhals commented Oct 7, 2021

Proposed Changes

Introduces a struct BasicProperties and opens up the header implementation to a public.

Changes in detail:

  • Replaced IBasicProperties in the receiving area with ReadOnlyBasicProperties
  • Replaced BasicProperties class with a struct version
  • Modified BasicPublish to be generic (TProperties : IBasicProperties, IAmqpHeader)
  • Removed CreateBasicProperties method, replacement is 'new BasicProperties()'
  • Introduce public IAmqpHeader interface. (Can be implemented by anyone and used as header properties)
  • Removed IStreamProperties, IContentHeader (Not needed for anything)
  • Aligned the name for getting the buffer size: GetRequiredPayloadBufferSize -> GetRequiredBufferSize
  • Aligned the name for writing to the buffer: WritePropertiesTo -> WriteTo

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

I'm going to post a few benchmark numbers as soon as I get around to run them. (soon)

@bollhals bollhals force-pushed the feature/basicProperties branch 3 times, most recently from 4b7d077 to fe4a583 Compare October 7, 2021 21:58
@michaelklishin
Copy link
Member

michaelklishin commented Oct 8, 2021

Thank you. Looks like this is still a WIP? I run into compilation errors:

/path/to/dotnet_client.git/projects/Unit/TestConnectionRecovery.cs(783,20): error CS0411: The type arguments for method 'IModel.BasicPublish<TProperties>(string, string, in TProperties, ReadOnlyMemory<byte>, bool)' cannot be inferred from the usage. Try specifying the type arguments explicitly. [/path/to/dotnet_client.git/projects/Unit/Unit.csproj]
/path/to/dotnet_client.git/projects/Unit/TestConnectionRecovery.cs(798,37): error CS1061: 'IModel' does not contain a definition for 'CreateBasicProperties' and no accessible extension method 'CreateBasicProperties' accepting a first argument of type 'IModel' could be found (are you missing a using directive or an assembly reference?) [/path/to/dotnet_client.git/projects/Unit/Unit.csproj]

@bollhals
Copy link
Contributor Author

bollhals commented Oct 8, 2021

Ah probably my last rebase against main... sorry for that. I'll update it when I ran the benchmarks👍🏼

@bollhals bollhals force-pushed the feature/basicProperties branch from fe4a583 to 7fca46f Compare October 8, 2021 21:48
@bollhals bollhals force-pushed the feature/basicProperties branch from 7fca46f to 288344d Compare October 16, 2021 11:39
@bollhals
Copy link
Contributor Author

Method When Runtime Mean Error StdDev Code Size
BasicPublishWriteNonEmpty Before .NET Core 3.1 324.15 ns 2.803 ns 2.622 ns 5,638 B
BasicPublishWriteNonEmpty After .NET Core 3.1 327.46 ns 2.391 ns 2.236 ns 5,697 B
BasicPublishWrite Before .NET Core 3.1 118.49 ns 1.718 ns 1.607 ns 5,638 B
BasicPublishWrite After .NET Core 3.1 86.68 ns 0.507 ns 0.450 ns 2,651 B
BasicPublishMemoryWrite Before .NET Core 3.1 72.80 ns 0.386 ns 0.342 ns 6,127 B
BasicPublishMemoryWrite After .NET Core 3.1 44.78 ns 0.288 ns 0.270 ns 2,594 B
BasicPublishWriteNonEmpty Before .NET Framework 4.8 438.34 ns 3.226 ns 3.018 ns 6,831 B
BasicPublishWriteNonEmpty After .NET Framework 4.8 443.86 ns 4.187 ns 3.712 ns 6,849 B
BasicPublishWrite Before .NET Framework 4.8 220.12 ns 0.528 ns 0.468 ns 6,703 B
BasicPublishWrite After .NET Framework 4.8 182.63 ns 0.969 ns 0.906 ns 3,006 B
BasicPublishMemoryWrite Before .NET Framework 4.8 153.67 ns 1.525 ns 1.427 ns 7,044 B
BasicPublishMemoryWrite After .NET Framework 4.8 119.55 ns 0.652 ns 0.610 ns 3,347 B
Method When Mean Error StdDev Ratio Gen 0 Gen 1 Gen 2 Allocated
Publish_Hello_World Before 149.2 ms 2.78 ms 2.60 ms 1.00 750.0000 - - 4.33 MB
Publish_Hello_World After 148.1 ms 2.94 ms 4.66 ms 1.00 750.0000 - - 4.02 MB

Before
Before

After
after

Summary:
Pure serialization performance is slight worse, but overall the performance is better and there is less memory allocations.
Also performance without properties is significant faster.

Remaining memory allocation for sending and receiving is mostly string creation.

@@ -187,7 +188,8 @@ public interface IModel : IDisposable
/// Routing key must be shorter than 255 bytes.
/// </para>
/// </remarks>
void BasicPublish(string exchange, string routingKey, bool mandatory, IBasicProperties basicProperties, ReadOnlyMemory<byte> body);
void BasicPublish<TProperties>(string exchange, string routingKey, ref TProperties basicProperties, ReadOnlyMemory<byte> body = default, bool mandatory = false)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

having ref in the outer interface is maybe something to discuss, as it forces everyone to write BasicPublish("exchange", "routingkey", ref properties, body)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we switch it to in, it means that we have to copy the struct once.

@bollhals
Copy link
Contributor Author

This is ready for review.
@stebet whats your take on ref in the public interface?

@bollhals bollhals force-pushed the feature/basicProperties branch from 288344d to af8c072 Compare October 21, 2021 20:35
@bollhals
Copy link
Contributor Author

rebased after latest additions in main.

@bollhals
Copy link
Contributor Author

bollhals commented Nov 8, 2021

can this be reviewed?

@bollhals
Copy link
Contributor Author

@michaelklishin can we get this reviewed / approved?

@bollhals
Copy link
Contributor Author

Reminder for review

@michaelklishin
Copy link
Member

I had to update RabbitMQ.Client.Unit.APIApproval expectations, running the rest of the suites.

@michaelklishin michaelklishin merged commit af8c072 into rabbitmq:main Dec 27, 2021
@bollhals
Copy link
Contributor Author

Whoops, my idea was just to review, as this change does have impact on some basic API that will require some turtorials/docu update.

Were you aware of this?

{
if (frame.Type != FrameType.FrameHeader)
{
throw new UnexpectedFrameException(frame.Type);
}

ReadOnlySpan<byte> span = frame.Payload.Span;
_header = _protocol.DecodeContentHeaderFrom(NetworkOrderDeserializer.ReadUInt16(span), span.Slice(12));
var classId = NetworkOrderDeserializer.ReadUInt16(span);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, this is where #1260 was introduced :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants