-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
Remove byte array allocations from AmqpReceiver DisposeMessagesAsync #20425
Conversation
Thank you for your contribution @danielmarbach! We will review the pull request and get back to you soon. |
b207908
to
87fad19
Compare
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs
Outdated
Show resolved
Hide resolved
… revealing design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once again, @danielmarbach - Thanks! The approach looks good to me.
sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpReceiver.cs
Outdated
Show resolved
Hide resolved
@@ -467,7 +472,8 @@ private static void CloseLink(RequestResponseAmqpLink link) | |||
var i = 0; | |||
foreach (ArraySegment<byte> deliveryTag in deliveryTags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we ever actually call with multiple lock tokens? Unless I'm missing something, there seems to be only a single one wrapped in an array at each call site. This seems to be something that we call frequently as part of the core flow. Should we consider an overload optimized for a single token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We never actually call this with multiple tokens. Considering this functionality would no longer be needed the best way would be to remove the multiple token approach. This would also get rid of a lot of List and Array allocs entirely. I've pointed this out in the issue linked in the description.
The code only ever passes one lock token in so by getting rid of all the enumeration we could stackalloc the required memory for the guid and then marshal it into. This would also get rid of all the task lists etc and make the code in general more efficient or if the multiple lock token path needs to be preserved
But since nobody answered there, and I didn't know if the code is still needed or not I figured it is best to assume it is and went ahead with this approach. I've also considered having overloads but then I thought the redundancy involved might also be questioned during the reviews so I stuck to this one for now. Happy to change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I don't have good insight into some of the nuances around the lock management nor potential future scenarios. @JoshLove-msft would definitely be the better option to make the call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing the alternative took me less than 20 min so I just went ahead and sent in a second proposal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative approach looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So close this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think so.
Solves #20166 assuming you want to preserve the capability of dispose multiple messages at the same time. The before/after comparison is in the issue including perf comparison about the different approaches considered
All SDK Contribution checklist:
This checklist is used to make sure that common guidelines for a pull request are followed.
Draft
mode if it is:General Guidelines and Best Practices
Testing Guidelines
SDK Generation Guidelines
*.csproj
andAssemblyInfo.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]
orAzure.ResourceManager.[RP]
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:generate.ps1/cmd
to generate this PR instead of callingautorest
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.Note: We have recently updated the PSH module called by
generate.ps1
to emit additional data. This would help reduce/eliminate the Code Verification check error. Please run following command: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.