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

Benchmark for strategy creation #1426

Merged
merged 1 commit into from
Jul 24, 2023
Merged

Benchmark for strategy creation #1426

merged 1 commit into from
Jul 24, 2023

Conversation

martintmk
Copy link
Contributor

@martintmk martintmk commented Jul 24, 2023

Details on the issue fix or feature implementation

Benchmark that demonstrates the creation of strategies (V7 vs V8).

Confirm the following

  • I started this PR by branching from the head of the default branch
  • I have targeted the PR to merge into the default branch
  • I have included unit tests for the issue/feature
  • I have successfully run a local build

@martintmk martintmk added the v8 Issues related to the new version 8 of the Polly library. label Jul 24, 2023
@martintmk martintmk added this to the v8.0.0 milestone Jul 24, 2023
| Method | Mean | Error | StdDev | Gen0 | Allocated |
|------------ |-----------:|---------:|---------:|-------:|----------:|
| Fallback_V7 | 114.8 ns | 1.84 ns | 2.70 ns | 0.0191 | 480 B |
| Fallback_V8 | 4,324.7 ns | 21.92 ns | 31.43 ns | 0.2518 | 6504 B |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The creation in V8 is indeed more expensive as it involves a lot more infra. For this reason the recreation of strategies on hot path should be discouraged.

Copy link
Member

Choose a reason for hiding this comment

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

What would your generic recommendation be?

In the sandbox app the fallback behaviour is trivial and could be cached, but in the code on which it was based there's dynamic per-invocation behaviour going on to select the fallback.

It feels that to support the same behaviour the more performant option would be to drop Polly fallbacks entirely and deal with it at the call site like any "normal" code, which if true doesn't feel great that the performance for this scenario has actually gone backwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think I have seen a scenario where you are "forced" to recreate a strategy on a hot path.

At worst you need to fallback to some dynamic value, but that can be archived by passing such value to the fallback action using ResilienceContext.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In you code the caching could look like:

image

Copy link
Member

Choose a reason for hiding this comment

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

Could you paste the code rather than a picture of it? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I tried to create a draft PR to your repo and got rejected.

public ResilienceStrategy<TResult> GetStrategy<TResult>(ApiEndpointOption endpoint, bool handleExecutionFaults, string resource)
{
    return _registry.GetOrAddStrategy<TResult>($"fallback/{resource}/{handleExecutionFaults}", builder =>
    {
        var shouldHandle = new PredicateBuilder<TResult>()
            .Handle<ApiException>()
            .HandleHttpRequestFault()
            .Handle<TaskCanceledException>();

        if (handleExecutionFaults)
        {
            shouldHandle = shouldHandle
                .Handle<BrokenCircuitException>()
                .Handle<IsolatedCircuitException>()
                .Handle<TimeoutRejectedException>();
        }

        builder
            .AddStrategy(GetStrategy(endpoint, resource))
            .AddFallback(new FallbackStrategyOptions<TResult>()
            {
                FallbackAction = context =>
                {
                    if (context.Context.Properties.TryGetValue(FallbackKeys<TResult>.FallbackValue, out var value) && value != null)
                    {
                        return Outcome.FromResultAsTask(value());
                    }

                    return Outcome.FromResultAsTask<TResult>(default);
                },
                ShouldHandle = shouldHandle,
                StrategyName = $"{endpoint.Name} Fallback",
            });
    });
}

Copy link
Member

@martincostello martincostello Jul 24, 2023

Choose a reason for hiding this comment

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

Do you have the code for FallbackKeys?

Nevermind, I can see it in the screenshot.

Comment on lines +14 to +15
| Fallback_V7 | 114.8 ns | 1.84 ns | 2.70 ns | 0.0191 | 480 B |
| Fallback_V8 | 4,324.7 ns | 21.92 ns | 31.43 ns | 0.2518 | 6504 B |
Copy link
Member

Choose a reason for hiding this comment

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

Wow that's a lot more 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think validation eats a lot of that as it involves reflection.

@martintmk martintmk enabled auto-merge (squash) July 24, 2023 08:56
@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #1426 (7ff337a) into main (b9ce232) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1426   +/-   ##
=======================================
  Coverage   83.92%   83.92%           
=======================================
  Files         275      275           
  Lines        6526     6526           
  Branches     1007     1007           
=======================================
  Hits         5477     5477           
  Misses        840      840           
  Partials      209      209           
Flag Coverage Δ
macos 83.92% <ø> (ø)
windows 83.92% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@martintmk martintmk merged commit 707686b into main Jul 24, 2023
12 checks passed
@martintmk martintmk deleted the mtomka/creation-bench branch July 24, 2023 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 Issues related to the new version 8 of the Polly library.
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants