Skip to content

Commit

Permalink
Potentially resolve AOT warning
Browse files Browse the repository at this point in the history 
Attempt to resolve the AOT warning in `DelegatingComponent`.
  • Loading branch information
@martincostello
martincostello committed Oct 27, 2023
1 parent a620b56 commit 393b2db
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 3 deletions.
1 change: 1 addition & 0 deletions src/Polly.Core/Polly.Core.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

<ItemGroup>
<Using Include="Polly.Utils" />
<InternalsVisibleToTest Include="Polly.AotTest" />
<InternalsVisibleToTest Include="Polly.Core.Tests" />
<InternalsVisibleToTest Include="Polly.Testing" />
<InternalsVisibleToTest Include="Polly.TestUtils" />
Expand Down
19 changes: 16 additions & 3 deletions src/Polly.Core/Utils/Pipeline/CompositeComponent.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,20 +133,33 @@ private sealed class DelegatingComponent : PipelineComponent
ResilienceContext context,
TState state)
{
// Custom state object is used to cast the callback and state to prevent infinite
// generic type recursion warning IL3054 when referenced in a native AoT application.
// See https://github.com/App-vNext/Polly/issues/1732 for further context.
return _component.ExecuteCore(
static (context, state) =>
static (context, wrapper) =>
{
var callback = (Func<ResilienceContext, TState, ValueTask<Outcome<TResult>>>)wrapper.Callback;
var state = (TState)wrapper.State;
if (context.CancellationToken.IsCancellationRequested)
{
return Outcome.FromExceptionAsValueTask<TResult>(new OperationCanceledException(context.CancellationToken).TrySetStackTrace());
}
return state.Next!.ExecuteCore(state.callback, context, state.state);
return wrapper.Next.ExecuteCore(callback, context, state);
},
context,
(Next, callback, state));
new StateWrapper(Next!, callback, state!));
}

public override ValueTask DisposeAsync() => default;

private struct StateWrapper(PipelineComponent next, object callback, object state)
{
public PipelineComponent Next = next;
public object Callback = callback;
public object State = state;

This comment has been minimized.

Sign in to view

Copy link
@davidfowl

davidfowl Oct 27, 2023

Does this introduce boxing? If yes, maybe only do this in AOT mode?

This comment has been minimized.

Sign in to view

Copy link
@martincostello

martincostello Oct 27, 2023

Author Owner

Yeah that's what I was thinking too. I'm about to write a microbenchmark to see.

Do you have a code snippet to show checking for AOT mode? I've not done that before.

This comment has been minimized.

Sign in to view

Copy link
@davidfowl

davidfowl Oct 27, 2023

There's RuntimeFeature.IsDynamicCodeSupported and RuntimeFeature.IsDynamicCodeCompiled, that are typically used for different reasons (when generating dynamic code using ref emit), but might be usable here as well.

cc @eerhardt @MichalStrehovsky

This comment has been minimized.

Sign in to view

Copy link
@martincostello

martincostello Oct 27, 2023

Author Owner

First attempt (via hacky local build to switch modes) without using RuntimeFeature.*:


BenchmarkDotNet v0.13.9+228a464e8be6c580ad9408e98f18813f6407fb5a, Windows 11 (10.0.22621.2428/22H2/2022Update/SunValley2)
12th Gen Intel Core i7-1270P, 1 CPU, 16 logical and 12 physical cores
.NET SDK 7.0.403
  [Host] : .NET 7.0.13 (7.0.1323.51816), X64 RyuJIT AVX2

Job=MediumRun  Toolchain=InProcessEmitToolchain  IterationCount=15  
LaunchCount=2  WarmupCount=10
Method Mean Error StdDev Ratio RatioSD Gen0 Allocated Alloc Ratio
Unfriendly 64.51 ns 0.909 ns 1.332 ns 1.00 0.00 - - NA
Friendly 65.06 ns 1.618 ns 2.372 ns 1.01 0.04 0.0025 24 B NA

This comment has been minimized.

Sign in to view

Copy link
@martincostello

martincostello Oct 27, 2023

Author Owner

And with a RuntimeFeature.IsDynamicCodeSupported guard to pick old or new (there's definitely some noise in the actual numbers for the execution time):


BenchmarkDotNet v0.13.9+228a464e8be6c580ad9408e98f18813f6407fb5a, Windows 11 (10.0.22621.2428/22H2/2022Update/SunValley2)
12th Gen Intel Core i7-1270P, 1 CPU, 16 logical and 12 physical cores
.NET SDK 7.0.403
  [Host] : .NET 7.0.13 (7.0.1323.51816), X64 RyuJIT AVX2

Job=MediumRun  Toolchain=InProcessEmitToolchain  IterationCount=15  
LaunchCount=2  WarmupCount=10
Method Mean Error StdDev Ratio RatioSD Allocated Alloc Ratio
Unfriendly 63.35 ns 1.702 ns 2.547 ns 1.00 0.00 - NA
Friendly 62.14 ns 2.226 ns 3.333 ns 0.98 0.07 - NA

This comment has been minimized.

Sign in to view

Copy link
@martincostello

martincostello Oct 27, 2023

Author Owner

Rough code from that is here.

I'm going to clean it up a bit and then open a draft PR to see how best we can move forward.

This comment has been minimized.

Sign in to view

Copy link
@MichalStrehovsky

MichalStrehovsky Oct 27, 2023

Does this introduce boxing? If yes, maybe only do this in AOT mode?

Doing things like that makes it impossible to reason about AOT perf. We don't want people to run different code on aot unless unavoidable.

If this is perf critical and the boxing cannot be tolerated, it shouldn't be tolerated on NAOT either. It's it's tolerable, don't increase your test matrix unnecessarily.

This trades the box for less jitting (evenn outside NativeAOT). That means better startup and less working set. It should be evaluated against that. Bdn unfortunately doesn't put much focus on those metrics.

This comment has been minimized.

Sign in to view

Copy link
@martincostello

martincostello Oct 27, 2023

Author Owner

Let's move the conversation to App-vNext#1737.
}
}
}
14 changes: 14 additions & 0 deletions src/Polly.Core/Utils/Pipeline/PipelineComponentFactory.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,24 @@ internal static class PipelineComponentFactory

public static PipelineComponent WithDisposableCallbacks(PipelineComponent component, IEnumerable<Action> callbacks)
{
#if NET6_0_OR_GREATER
if (callbacks.TryGetNonEnumeratedCount(out var count))
{
if (count == 0)
{
return component;
}
}
else if (!callbacks.Any())
{
return component;
}
#else
if (!callbacks.Any())
{
return component;
}
#endif

return new ComponentWithDisposeCallbacks(component, callbacks.ToList());
}
Expand Down
10 changes: 10 additions & 0 deletions test/Polly.AotTest/Program.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,16 @@
p.ExecuteCore<int, int>(state => default, default);
p.ExecuteCore<int, int>(state => default, default);

var p1 = Polly.Utils.Pipeline.PipelineComponent.Empty;
var p2 = Polly.Utils.Pipeline.PipelineComponent.Empty;
var p3 = Polly.Utils.Pipeline.CompositeComponent.Create([p1, p2], null!, null!);
var p4 = new Polly.Utils.Pipeline.BridgeComponent(null!);

await p1.ExecuteCore<int, int>((state, context) => default, default!, default);
await p2.ExecuteCore<int, int>((state, context) => default, default!, default);
await p3.ExecuteCore<int, int>((state, context) => default, default!, default);
await p4.ExecuteCore<int, int>((state, context) => default, default!, default);

var app = builder.Build();

var sampleTodos = new Todo[] {
Expand Down

0 comments on commit 393b2db

Please sign in to comment.