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

Support PublishAot in addition to RunAOTCompilation for doing Mono-based AOT #34051

Closed
wants to merge 1 commit into from

Conversation

akoeplinger
Copy link
Member

No description provided.

<!-- PublishAot depends on PublishTrimmed. This must be set early enough for the KnownILLinkPack to be restored. -->
<PublishTrimmed Condition="'$(PublishTrimmed)' == '' And '$(PublishAot)' == 'true'">true</PublishTrimmed>
<PublishTrimmed Condition="'$(PublishTrimmed)' == '' And ('$(PublishAot)' == 'true' and '$(PublishAotUsingMonoRuntime)' != 'true')">true</PublishTrimmed>
Copy link
Member

Choose a reason for hiding this comment

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

Why are we not enabling trimming and the analyzer if this is supposed to be "like native AOT, but for Mono"? I think this is going to cause confusion because this is a visibly different behavior for the same setting name.

Cc @dotnet/ilc-contrib @samsp-msft

Copy link
Member

Choose a reason for hiding this comment

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

The problem this is intending to solve right now is that there are two different ways of turning on aot. That alone could lead to confusion where both NativeAOT and Mono AOT are present (asp.net and blazor webassembly).

Copy link
Member

Choose a reason for hiding this comment

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

The problem I see is that we currently have several ways to approach AOT:

  1. AOT is forced on the user by the platform: e.g. Apple devices. User doesn't really want AOT but we're not going to run .NET code any other way. We have a "compatible AOT" there, where AOT is ideally just an implementation detail that tries not to get in users way. It's not something the user is going to enable in their project file and it doesn't matter how we name the property because it's enabled by default.
  2. AOT is a choice done by the user to optimize startup, without affecting compat. This maps to PublishReadyToRun in CoreCLR and I think it more closely matches AOT in Mono as well, as long as we're not trimming there. It will not improve memory use or size (in fact it will make the size worse)
  3. AOT is a choice done by the user to get smaller size, fast startup, low working set. This is how we always blog about/document PublishAot - always bring up compat as the cost of size/startup/memory.

I think if we want Mono to respect PublishAot, it should do the third bullet above. I don't think it should mean "make the size worse". Enabling trimming should be table stakes and I think it should even go as far as disable interpreter, strip IL, and do any other compat-breaking things that could be done, and we should have analyzers for that to help users navigate the compat part.

Otherwise it feels like we're just making PublishAot mean on Mono what PublishReadyToRun means on CoreCLR. If we want to look at it purely as a startup optimization, maybe Mono respecting PublishReadyToRun would be a better choice for that. Yep, the underlying implementation detail is not called ReadyToRun, but the end result observable to the user is closer to what PublishReadyToRun does.

Copy link
Member

Choose a reason for hiding this comment

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

And to add one more thing to that: PublishAot should also imply full trimming, not just partial trimming.

The north star for PublishAot should be a mode of .NET that is competitive with AOT-compiled languages wrt size/startup/memory. On ASP.NET side we look at Go a lot. On iOS side we should be looking at Swift. Without full trimming, this is not achievable. PublishAot should mean that.

Copy link
Member

Choose a reason for hiding this comment

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

To add to what Michal said, we have all documentation written in this spirit. If we were to make PublishAot to mean something else on Mono, the documentation would need to be updated in number of places to explain the different meaning. It would make the documentation very confusing.

One example from many that would need to get this treatment: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/?tabs=net7#limitations-of-native-aot-deployment

Copy link
Member

Choose a reason for hiding this comment

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

Wasm is a 4th option here where PublishAot can imply full trimming but will likely still negatively impact size. I fully support the idea that PublishAot has different defaults on browser-wasm than RunAOTCompilation.

Copy link
Member

Choose a reason for hiding this comment

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

I hear what you're saying. The way I see it is that we ship two aot compilers, one that is very prescriptive in the way it operates and the other with some variation (full aot, full w/ interp fallback, partial, etc). It seems confusing to heavily define AOT to mean one thing when the reality is different. The question I see is, do we go with what this PR is doing, which means we consolidate around turning both on via PubishAot or do we maintain the status quo because we want PublishAot to mean something very specific?

@sayedihashimi @lewing thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

The way I see it is that we ship two aot compilers

We ship 3 AOT compilers:

  • ReadyToRun AOT
  • Native AOT
  • Mono AOT

Mono AOT compiler characteristics are equivalent to ReadyToRun AOT compiler compiler characteristics on CoreCLR.

Copy link
Member

Choose a reason for hiding this comment

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

One data point is that when PublishAot was first introduced, there was a discussion to name it PublishNativeAot solely because PublishReadyToRun already existed and it is also AOT.

The decision was to use PublishAot for the restrictive AOT (without any other qualifier) despite the fact that we already had a form of AOT for these verticals (console apps at that time of 7.0, but we were already thinking of ASP.NET/WinForms and others). (I would have named it PublishNativeAot but I can also see the argument for PublishAot.)

We probably wouldn't have this conversation if this was originally named PublishNativeAot. I think the conversation we're having now largely resembles the original "should this be PublishAot or PublishNativeAot?" and that one was settled as "AOT should mean the AOT form-factor that people actually need to think about vs AOT as an implementation detail people don't think about much".

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Jul 17, 2023

How are the analyzers going to work in the NativeAOT experiment on iDevices?

We need the AOT/trimming/single-file analyzers enabled in the IDEs if the user has an intention to use native AOT.

The analyzers are how people troubleshoot compatibility issues. People not seeing analyzer feedback when they intend to use Native AOT would be a ship blocker. We don't optimize for such scenario and the experience is terrible.

@steveisok
Copy link
Member

How are the analyzers going to work in the NativeAOT experiment on iDevices?

We need the AOT/trimming/single-file analyzers enabled in the IDEs if the user has an intention to use native AOT.

That's a good point and I think the nativeaot rules should apply. I'm not sure we're all the way there b/c I think we're not using the publish targets.

@ivanpovazan
Copy link
Member

ivanpovazan commented Jul 21, 2023

@steveisok @MichalStrehovsky I have opened xamarin/xamarin-macios#18571 to address:

How are the analyzers going to work in the NativeAOT experiment on iDevices?

Feel free to add more context.

@akoeplinger
Copy link
Member Author

Closing for now until we have a decision on whether we want to do this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Infrastructure untriaged Request triage from a team member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants