Split body memory into distinct parts for better clarity and avoiding to copy for the send path when a single ROM is passed

[danielmarbach]

Follow up for #19996

All SDK Contribution checklist:

This checklist is used to make sure that common guidelines for a pull request are followed.

• Please open PR in Draft mode if it is:
  • Work in progress or not intended to be merged.
  • Encountering multiple pipeline failures and working on fixes.
• If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
• I have read the contribution guidelines.
• The pull request does not introduce breaking changes.

General Guidelines and Best Practices

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

SDK Generation Guidelines

• The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code. (Track 2 only)
• The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK. Please double check nuget.org current release version.

Additional management plane SDK specific contribution checklist:

Note: Only applies to Microsoft.Azure.Management.[RP] or Azure.ResourceManager.[RP]

• Include updated management metadata.
• Update AzureRP.props to add/remove version info to maintain up to date API versions.

Management plane SDK Troubleshooting

• If this is very first SDK for a services and you are adding new service folders directly under /SDK, please add new service label and/or contact assigned reviewer.
• If the check fails at the Verify Code Generation step, please ensure:
  • Do not modify any code in generated folders.
  • Do not selectively include/remove generated files in the PR.
  • Do use generate.ps1/cmd to generate this PR instead of calling autorest directly.
    Please pay attention to the @microsoft.csharp version output after running generate.ps1. If it is lower than current released version (2.3.82), please run it again as it should pull down the latest version,

Old outstanding PR cleanup

Please note:
If PRs (including draft) has been out for more than 60 days and there are no responses from our query or followups, they will be closed to maintain a concise list for our reviewers.

[ghost]

Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon.

[danielmarbach]

@JoshLove-msft I wanted to see if it would be worthwhile to split the send and the receive path a bit in terms of BodyMemory in order to verify if it is possible to avoid copying the single ROM on the send path. Here is the result. What do you think?

[danielmarbach]

I used the same test suite as the previous PR but did access the body during send

            await using var sender = serviceBusClient.CreateSender(destination);
            var messages = new List<ServiceBusMessage>(10000);
            for (int i = 0; i < 10000; i++)
            {
                var message = new ServiceBusMessage(UTF8.GetBytes($"Deep Dive {i} Deep Dive {i} Deep Dive {i} Deep Dive {i} Deep Dive {i} Deep Dive {i}"));
                messages.Add(message);
                Console.WriteLine(message.Body);

                if (i % 1000 == 0)
                {
                    await sender.SendMessagesAsync(messages);
                    messages.Clear();
                }
            }

Before

After

[jsquire]

Interesting, I think that I get what you're up to. If you don't mind, I'm going to state my understanding and ask you to keep me honest:

• The receive path needs to copy memory eagerly, because we're going to dispose the source AMQP message, so the constructor copies the segments into the writer, capturing each copied segment as a slice to an array so that we can see what the original structure was.

• The send path can assume that the read-only memory segments remain valid, as they're held by the caller until the message is sent. In this case, a reference to the source segments is captured and just held until something asks for them to be flattened into the writer. At that point, the copied segments are sliced so that we can continue to see what the original structure was in the case that the caller no longer holds the source memory for us.

• The constructor and body setter for Service Bus Message assume the send path.

• I'd guess that the constructor for the Service Bus Receive Message will assume the receive path.

How am I doing so far?

[jsquire]

The Body Memory approach gives us a construct that knows to only materialize the value once, no matter how many times the GetBody extension is called. If a caller manipulates the AmqpAnnotatedMessage directly, then we won't have an existing BodyMemory to return and we'll create a new one on the fly, essentially giving us the same caching behavior.

Nice.... assuming that I'm close, I think we've got the end-to-end story. Awesome!

[JoshLove-msft]

The Body Memory approach gives us a construct that knows to only materialize the value once, no matter how many times the GetBody extension is called. If a caller manipulates the AmqpAnnotatedMessage directly, then we won't have an existing BodyMemory to return and we'll create a new one on the fly, essentially giving us the same caching behavior.

Nice.... assuming that I'm close, I think we've got the end-to-end story. Awesome!

Yes, that is my understanding. This PR makes the send "happy path" more efficient by avoiding the ArrayBufferWriter usage when we know there is a single ReadOnlyMemory.

[JoshLove-msft]

@danielmarbach thanks for doing this - it looks great! Any chance we can add a few unit tests that validate that the Body is correct at several points in the lifetime of a message, e.g. whether setting with the property or through the AmqpAnnotatedMessage.

[danielmarbach]

@danielmarbach thanks for doing this - it looks great! Any chance we can add a few unit tests that validate that the Body is correct at several points in the lifetime of a message, e.g. whether setting with the property or through the AmqpAnnotatedMessage.

I can see what I can do

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JoshLove-msft]

/azp run net - servicebus - tests

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JoshLove-msft]

Will merge this after releasing the new beta as we are past the code complete date.