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

[QUERY] ServiceBusRetryPolicy anonymous class allocation #19126

Closed
danielmarbach opened this issue Mar 2, 2021 · 3 comments · Fixed by #19522
Closed

[QUERY] ServiceBusRetryPolicy anonymous class allocation #19126

danielmarbach opened this issue Mar 2, 2021 · 3 comments · Fixed by #19522
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Service Bus

Comments

@danielmarbach
Copy link
Contributor

I've looked at the usage of ServiceBusRetryPolicy.RunOperation and creates a lot of display class allocations. By providing overloads of RunOperation like

        internal async Task<TResult> RunOperation<T1, T2, T3, TResult>(
            Func<T1, T2, T3, TimeSpan, CancellationToken, Task<TResult>> operation,
            TransportConnectionScope scope,
            T1 t1,
            T2 t2,
            T3 t3,
            CancellationToken cancellationToken)
        {

it would be possible to get rid of those. Example

            this.\u003C\u003E8__1 = new AmqpReceiver.\u003C\u003Ec__DisplayClass34_0();
            this.\u003C\u003E8__1.\u003C\u003E4__this = this.\u003C\u003E4__this;
            this.\u003C\u003E8__1.maxMessages = this.maxMessages;
            this.\u003C\u003E8__1.maxWaitTime = this.maxWaitTime;
            this.\u003C\u003E8__1.cancellationToken = this.cancellationToken;
            this.\u003C\u003E8__1.messages = (IReadOnlyList<ServiceBusReceivedMessage>) null;
            // ISSUE: method pointer
            awaiter = amqpReceiver._retryPolicy.RunOperation(new Func<TimeSpan, Task>((object) this.\u003C\u003E8__1, __methodptr(\u003CReceiveMessagesAsync\u003Eb__0)), (TransportConnectionScope) amqpReceiver._connectionScope, this.\u003C\u003E8__1.cancellationToken).ConfigureAwait(false).GetAwaiter();
            if (!awaiter.IsCompleted)
            {
              this.\u003C\u003E1__state = num2 = 0;
              this.\u003C\u003Eu__1 = awaiter;
              this.\u003C\u003Et__builder.AwaitUnsafeOnCompleted<ConfiguredTaskAwaitable.ConfiguredTaskAwaiter, AmqpReceiver.\u003CReceiveMessagesAsync\u003Ed__34>(ref awaiter, ref this);
              return;
            }

vs

          ConfiguredTaskAwaitable<IReadOnlyList<ServiceBusReceivedMessage>>.ConfiguredTaskAwaiter awaiter;
          int num2;
          if (num1 != 0)
          {
            // ISSUE: method pointer
            awaiter = amqpReceiver._retryPolicy.RunOperation2<int, TimeSpan?, IReadOnlyList<ServiceBusReceivedMessage>>(new Func<TimeSpan, int, TimeSpan?, CancellationToken, Task<IReadOnlyList<ServiceBusReceivedMessage>>>((object) amqpReceiver, __methodptr(\u003CReceiveMessagesAsync\u003Eb__34_0)), (TransportConnectionScope) amqpReceiver._connectionScope, this.maxMessages, this.maxWaitTime, this.cancellationToken).ConfigureAwait(false).GetAwaiter();
            if (!awaiter.IsCompleted)
            {
              this.\u003C\u003E1__state = num2 = 0;
              this.\u003C\u003Eu__1 = awaiter;
              this.\u003C\u003Et__builder.AwaitUnsafeOnCompleted<ConfiguredTaskAwaitable<IReadOnlyList<ServiceBusReceivedMessage>>.ConfiguredTaskAwaiter, AmqpReceiver.\u003CReceiveMessagesAsync\u003Ed__34>(ref awaiter, ref this);
              return;
            }

is that something worth pursuing or not worth the complexity?

There would still be a Func allocation though.

@ghost ghost added needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that labels Mar 2, 2021
@jsquire jsquire added Client This issue points to a problem in the data-plane of the library. needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team Service Bus labels Mar 2, 2021
@ghost ghost removed the needs-triage Workflow: This is a new issue that needs to be triaged to the appropriate team. label Mar 2, 2021
@jsquire
Copy link
Member

jsquire commented Mar 2, 2021

Hi @danielmarbach. Thanks for your feedback and the suggestion. I think there's merit to looking into this as an optimization. Looping Josh in for his thoughts.

@JoshLove-msft
Copy link
Member

Sure, as long it still all funnels through one core method (and the tests still pass 😄). Thanks @danielmarbach!

@JoshLove-msft JoshLove-msft removed their assignment Mar 3, 2021
@JoshLove-msft JoshLove-msft removed the needs-team-attention Workflow: This issue needs attention from Azure service team or SDK team label Mar 3, 2021
@danielmarbach
Copy link
Contributor Author

Fingers crossed ;) #19522

@github-actions github-actions bot locked and limited conversation to collaborators Mar 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. customer-reported Issues that are reported by GitHub users external to the Azure organization. question The issue doesn't require a change to the product in order to be resolved. Most issues start as that Service Bus
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants