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

[Feature request]: AOT compatibility #1732

Closed
davidfowl opened this issue Oct 27, 2023 · 12 comments · Fixed by #1737 or dotnet/extensions#4646
Closed

[Feature request]: AOT compatibility #1732

davidfowl opened this issue Oct 27, 2023 · 12 comments · Fixed by #1737 or dotnet/extensions#4646
Assignees
Labels
aot Relates to native AoT support feature suggestion
Milestone

Comments

@davidfowl
Copy link

davidfowl commented Oct 27, 2023

Is your feature request related to a specific problem? Or an existing feature?

Polly is at the core of Microsoft.Extensions.Http.Resilience and we'd like the libraries that depend on this package and polly itself to work in AOT environments.

Describe the solution you'd like

Add <IsAotCompatible>true</IsAotCompatible> to the project files and work through the AOT issues. I've run a project that has a dependency and saw this:

  ILC: Method '[Polly.Core]Polly.Utils.Pipeline.CompositeComponent+DelegatingComponent.ExecuteCore<__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>>(Func`3<Resi
  lienceContext,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>,ValueTask`1<Outcome`1<__Canon>>>,ResilienceContext,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__
  Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>)' will always throw because: Failed to load type 'Polly.Utils.Pipeline.PipelineComponent' from assembly 'Polly.Core, Version=8.0.0.0, Culture=neutral, PublicKeyToken=c8a
  3ffc3f8f825cc'
ILC : AOT analysis warning IL3054: Polly.Utils.Pipeline.CompositeComponent.DelegatingComponent.ExecuteCore<__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>>(Fun
c`3<ResilienceContext,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>,ValueTask`1<Outcome`1<__Canon>>>,ResilienceContext,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Ca
non,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>): Generic expansion to 'Polly.Utils.Pipeline.PipelineComponent.ExecuteCore<__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__C
anon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>>>(Func`3<ResilienceContext,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,
ValueTuple`2<__Canon,__Canon>>>>>>,ValueTask`1<Outcome`1<__Canon>>>,ResilienceContext,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`3<__Canon,__Canon,ValueTuple`2<__Canon,__Canon>>>>>>)' w
as aborted due to generic recursion. An exception will be thrown at runtime if this codepath is ever reached. Generic recursion also negatively affects compilation speed and the size of the compilation output. It is advisable to remove the source of the generic recursi
on by restructuring the program around the source of recursion. The source of generic recursion might include: 'Polly.Utils.Pipeline.PipelineComponent.ExecuteCore<TResult,TState>(Func`3<ResilienceContext,TState,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)',
'Polly.Utils.Pipeline.ComponentWithDisposeCallbacks.ExecuteCore<TResult,TState>(Func`3<ResilienceContext,TState,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)', 'Polly.Utils.Pipeline.CompositeComponent.ExecuteCore<TResult,TState>(Func`3<ResilienceContext,TStat
e,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)', 'Polly.Utils.Pipeline.CompositeComponent.ExecuteCoreWithoutTelemetry<TResult,TState>(Func`3<ResilienceContext,TState,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)', 'Polly.Utils.Pipeline.CompositeCom
ponent.ExecuteCoreWithTelemetry<TResult,TState>(Func`3<ResilienceContext,TState,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)', 'Polly.Utils.Pipeline.CompositeComponent.<ExecuteCoreWithTelemetry>d__13`2', 'Polly.Utils.Pipeline.ExternalComponent.ExecuteCore<TR
esult,TState>(Func`3<ResilienceContext,TState,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)', 'Polly.Utils.Pipeline.ReloadableComponent.ExecuteCore<TResult,TState>(Func`3<ResilienceContext,TState,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)', 'Poll
y.Utils.Pipeline.CompositeComponent.DelegatingComponent.ExecuteCore<TResult,TState>(Func`3<ResilienceContext,TState,ValueTask`1<Outcome`1<TResult>>>,ResilienceContext,!!1)', 'Polly.Utils.Pipeline.CompositeComponent.DelegatingComponent.<>c__6`2' [C:\dev\git\eShop\src\Or
dering.BackgroundTasks\Ordering.BackgroundTasks.csproj]

cc @MichalStrehovsky

Additional context

No response

@martincostello
Copy link
Member

Interesting - we did investigate this as part of #1110 and thought we were good for the v8 release (at least for all the new assemblies). We must have missed something or introduced a regression.

@davidfowl
Copy link
Author

Are the assemblies marked with <IsAotCompatible>true</IsAotCompatible>?

@MichalStrehovsky
Copy link

The Roslyn analyzer (that gets added with <IsAotCompatible>true</IsAotCompatible>) doesn't have the generic cycle detector so it wouldn't help in this case. This can only be detected by the AOT compiler currently. It requires a whole program analysis.

Does this compile with a warning, or does it even manage to crash the compiler?

@martincostello martincostello self-assigned this Oct 27, 2023
@martincostello
Copy link
Member

IsAotCompatible

Looks like these were the ones we already added (we tweaked them a bit for our .NET 8 branch too):

<IsTrimmable>true</IsTrimmable>
<EnableTrimAnalyzer>true</EnableTrimAnalyzer>
<EnableSingleFileAnalyzer>true</EnableSingleFileAnalyzer>
<EnableAotAnalyzer>true</EnableAotAnalyzer>

I guess we weren't aware there were additional options we needed to turn on as part of our testing.

I'll take a look into this today.

Assuming it's a quick fix, do you folks still have time to take a new release for the dotnet/extensions stuff for RTM? If not, we can just roll whatever fixes are needed into our 8.1.0 release which we're aiming to release once we can depend on 8.0.0 itself for support for TimeProvider.

@MichalStrehovsky
Copy link

The reduced issue is this:

using System;

PipelineComponent p = new DelegatingComponent(null, null);
p.ExecuteCore<int, int>(state => default, default);


class PipelineComponent
{
    public virtual TResult ExecuteCore<TResult, TState>(Func<TState, TResult> callback, TState state)
    {
        return default;
    }
}

class DelegatingComponent : PipelineComponent
{
    PipelineComponent _component;
    PipelineComponent _next;

    public DelegatingComponent(PipelineComponent component, PipelineComponent next)
    {
        _component = component;
        _next = next;
    }

    public override TResult ExecuteCore<TResult, TState>(Func<TState, TResult> callback, TState state)
    {
        return _component.ExecuteCore(static (state) =>
        {
            return state._next.ExecuteCore(state.callback, state.state);
        },
        (_next, callback, state));
    }
}

Notice that DelegatingComponent.ExecuteCore<TResult, TState> introduces a call to ExecuteCore<TResult, (PipelineComponent, Func<TState, TResult>, TState)>. This is introducing a generic recursion. The generic recursion needs to be broken somehow.

More about this here: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/warnings/il3054

Here's two samples that I know of that fixed a similar issue in different codebases:

@martincostello
Copy link
Member

martincostello commented Oct 27, 2023

I'm still doing some poking around, but from doing dotnet new webapiaot with the RC2 SDK to get a template project I can reference against Polly's source and throwing in a few snippets of Polly code I don't seem to be getting any IL3054 warnings.

Can you point me at a sample project somewhere I can adapt to be part of our build to use to validate native AOT support?

martincostello added a commit to martincostello/Polly that referenced this issue Oct 27, 2023
Use attributes instead of pragmas so the native AOT compiler can see them.
Relates to App-vNext#1732.
@martincostello
Copy link
Member

Where I've gotten to so far is in this branch in my fork.

If I run the following it builds with no trim warnings:

$ dotnet publish .\test\Polly.AotTest\Polly.AotTest.csproj --tl
MSBuild version 17.8.0+6cdef4241 for .NET
Restore complete (0.9s)
  Polly.Core net6.0 succeeded (1.3s) → src\Polly.Core\bin\Release\net6.0\Polly.Core.dll
  Polly.RateLimiting net6.0 succeeded (1.4s) → src\Polly.RateLimiting\bin\Release\net6.0\Polly.RateLimiting.dll
  Polly.Extensions net6.0 succeeded (1.5s) → src\Polly.Extensions\bin\Release\net6.0\Polly.Extensions.dll
  Polly.AotTest succeeded (24.1s) → test\Polly.AotTest\bin\Release\net8.0\win-x64\publish\

Build succeeded in 28.4s

@martincostello
Copy link
Member

Pasting in #1732 (comment) I can force a IL3053 warning which gives me something to iterate on for now.

@martincostello
Copy link
Member

Possible fix in martincostello@393b2db#diff-ca91823ebbd702b095cfa7918b14e1504a51cb2a4125f5d7553158d1721cd80d. I'm going to benchmark it in a bit to see if the approach doesn't hurt perf. It at least fixes the IL3054 warning in my repro project.

martincostello added a commit that referenced this issue Oct 27, 2023
Use attributes instead of pragmas so the native AOT compiler can see them.
Relates to #1732.
@davidfowl
Copy link
Author

Thanks for looking at this @martincostello !

martincostello added a commit to martincostello/Polly that referenced this issue Oct 27, 2023
A potential fix for App-vNext#1732 using `RuntimeFeature.IsDynamicCodeSupported` to guard against the allocation regression that avoiding the infinite generic recursion causes through boxing.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 28, 2023
Add a benchmark for `CompositeComponent` to aid with App-vNext#1732.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 28, 2023
Resolve two AoT warnings on `OutcomePipelineBuilderExtensions`.
Relates to App-vNext#1732.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 28, 2023
Resolve two AoT warnings on `OutcomePipelineBuilderExtensions`.
Relates to App-vNext#1732.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 28, 2023
Add a console project that validates whether the new-in-v8 Polly assemblies are AoT compatible.

Relates to App-vNext#1732.
@martincostello martincostello added the aot Relates to native AoT support label Oct 28, 2023
martincostello added a commit that referenced this issue Oct 28, 2023
Resolve two AoT warnings on `OutcomePipelineBuilderExtensions`.
Relates to #1732.
martincostello added a commit that referenced this issue Oct 28, 2023
Resolve two AoT warnings on `OutcomePipelineBuilderExtensions`.
Relates to #1732.
martincostello added a commit that referenced this issue Oct 28, 2023
Add a benchmark for `CompositeComponent` to aid with #1732.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 28, 2023
A potential fix for App-vNext#1732 using `RuntimeFeature.IsDynamicCodeSupported` to guard against the allocation regression that avoiding the infinite generic recursion causes through boxing.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 28, 2023
Add a console project that validates whether the new-in-v8 Polly assemblies are AoT compatible.

Relates to App-vNext#1732.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 29, 2023
Add a console project that validates whether the new-in-v8 Polly assemblies are AoT compatible.

Relates to App-vNext#1732.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 29, 2023
A potential fix for App-vNext#1732 using `RuntimeFeature.IsDynamicCodeSupported` to guard against the allocation regression that avoiding the infinite generic recursion causes through boxing.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 29, 2023
Add a console project that validates whether the new-in-v8 Polly assemblies are AoT compatible.

Relates to App-vNext#1732.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 30, 2023
A potential fix for App-vNext#1732 using `RuntimeFeature.IsDynamicCodeSupported` to guard against the allocation regression that avoiding the infinite generic recursion causes through boxing.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 30, 2023
Add a console project that validates whether the new-in-v8 Polly assemblies are AoT compatible.

Relates to App-vNext#1732.
@martincostello martincostello added this to the v8.1.0 milestone Oct 30, 2023
martincostello added a commit to martincostello/Polly that referenced this issue Oct 30, 2023
A potential fix for App-vNext#1732 using `RuntimeFeature.IsDynamicCodeSupported` to guard against the allocation regression that avoiding the infinite generic recursion causes through boxing.
martincostello added a commit to martincostello/Polly that referenced this issue Oct 30, 2023
Add a console project that validates whether the new-in-v8 Polly assemblies are AoT compatible.

Relates to App-vNext#1732.
@martincostello martincostello linked a pull request Oct 30, 2023 that will close this issue
martincostello added a commit that referenced this issue Oct 30, 2023
- Fix #1732 using `RuntimeFeature.IsDynamicCodeSupported` to guard against the allocation regression that avoiding the infinite generic recursion causes through boxing.
- Add a console project that validates whether the new-in-v8 Polly assemblies are AoT compatible.
- Mark the new Polly v8 assemblies as AoT compatible.
- Add a micro-benchmark for `DelegatingComponent` code path for non-AoT and AoT.
@martincostello
Copy link
Member

@davidfowl I'm planning on publishing Polly 8.1.0 release tomorrow which contains a couple of changes that should resolve your AoT warnings.

If you'd like to try out a prerelease version in the meantime, there should be some NuGet packages attached to this CI build once it completes in ~30 minutes.

@martincostello
Copy link
Member

Polly 8.1.0 has now been published to NuGet.org, which contains 3 different AoT fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aot Relates to native AoT support feature suggestion
Projects
None yet
3 participants