Closed ivanpovazan closed 1 year ago
Tagging subscribers to 'os-ios': @steveisok, @akoeplinger, @kotlarmilos See info in area-owners.md if you want to be subscribed.
Author: | ivanpovazan |
---|---|
Assignees: | ivanpovazan |
Labels: | `area-Codegen-AOT-mono`, `os-ios`, `area-NativeAOT-coreclr` |
Milestone: | 8.0.0 |
/cc: @kotlarmilos @lambdageek @marek-safar @filipnavara
/cc: @SamMonoRT @eerhardt
Thanks Ivan. Can you or Michal explain the constant propagation issue? Is this some kind of IL trimmer bug - we substitute constants for feature switches all the time. Why is it a problem here?
My understanding is that the existing delegate-based approach will work with Mono AOT now. What is the cost for NativeAOT to implement universal shared generics?
What is the cost for NativeAOT to implement universal shared generics?
I was asking about this multiple times already, and the answer was always that's it's intentionally not planned. .NET Native used to implement it (with different codegen backend), but any remains of the code were removed over time.
I don't think that Linq.Expressions would have any effect on the decision as the library itself is in archived state.
Thanks Ivan. Can you or Michal explain the constant propagation issue? Is this some kind of IL trimmer bug - we substitute constants for feature switches all the time. Why is it a problem here?
The problem is that two of the three mentioned control variables are configured as constants:
From my understanding, when the constant propagation is enabled during Linq.Expressions
library build for iOS-like platforms, the first two will get removed and all the code paths they control, as they are constants. Once that happens, the feature switches ILC compiler depends on: https://github.com/dotnet/runtime/blob/2aa0c9ba7f46048a113d214a2377cd2bf14ff29e/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets#L271-L273 become obscure, as these variables will not be present in the Linq.Expressions
assembly for iOS-like platforms (this can be seen in the warning messages I pasted in the description).
Replacing them with a non-constant RuntimeFeature.IsDynamicCodeSupported
would do the trick, as it would prevent them from being removed during the library build. What gets trimmed and AOTed further (during the app build) would then be properly controlled with DynamicCodeSupport
feature switch (System.Runtime.CompilerServices.RuntimeFeature.IsDynamicCodeSupported documented here).
Ah, I see. It's not a trimmer bug. Would it be better if these three values should be their own top-level feature switches instead of constants in the source. and the SDKs should set them as appropriate if the trimmer isn't running.
Tagging subscribers to this area: @cston See info in area-owners.md if you want to be subscribed.
Author: | ivanpovazan |
---|---|
Assignees: | ivanpovazan |
Labels: | `area-System.Linq.Expressions`, `os-ios` |
Milestone: | 8.0.0 |
What is the cost for NativeAOT to implement universal shared generics?
Universal shared generics wouldn't help with the size here - even on .NET Native, we only used universal shared code for the absolutely most dynamic scenarios (MakeGenericXXX
APIs and cases of deep generic recursion). We'd not do it for cases like in Linq.Expressions because universal shared code is slow and the perf characteristics are hard to reason about for users - it was a feature we built only because we needed to be as compatible as possible. For Native AOT we don't try to be as compatible as possible because people always have a choice not to deploy with Native AOT - we wouldn't sacrifice predictable perf for this. .NET Native had this logic in Linq.Expressions ifdeffed out, despite having universal shared code.
Ah, I see. It's not a trimmer bug.
We could also view it as a trimmer bug though - when we're running library level trimming (the way we do when we build the dotnet/runtime repo) and the library contains an embedded substitution manifest for something, maybe ILLinker should be smart enough to know that the substitution could replace the IL at app build time and maybe it shouldn't inline the value it sees in IL right now. The problem is really that the library trimming is generating a broken assembly (the warning we see at app build time warning IL2009: System.Linq.Expressions: Could not find method 'System.Boolean get_CanEmitObjectArrayDelegate()' on type 'System.Dynamic.Utils.DelegateHelpers'
is a sign that the library trimming generated a broken assembly - the substitution wants to substitute something we inlined at library build). Cc @dotnet/illink for thoughts.
maybe ILLinker should be smart enough to know that the substitution could replace the IL at app build time and maybe it shouldn't inline the value it sees in IL right now.
As far as I know we want both behaviors. For example, for IntPtr.Size we want to inline the value at library build time, so that we can remove x86 code from x64 assembly. But in this case we would want to avoid inlining it.
We would have to add some kind of hint to differentiate the two cases. For example we could add support for reading the NoInlining
attribute from attribute.xml - and add it in library build to this property (and make sure linker will comply).
For the IntPtr.Size case, the substitution is unconditional - we can inline at library build time because the value cannot change. I'm thinking about only skipping it for things that have a conditional substitution conditioned on some feature. Would that be still problematic? It feels like that's something we should be never inlining during libs build.
You're right - but that would probably mean the feature switch definition is wrong in a way - library builds should basically never define any feature switch values - for exactly this reason.
I see - the problem is that the code hardcodes a return value, the feature switch doesn't kick in at all (and linker doesn't consider substitutions which didn't apply due to feature switch values). Why don't we read this from AppContext just like other feature switches? It could still default to true
.
The additional benefit would be that one would get the same implementation of Linq.Expression in an AOT app regardless if it's running on CoreCLR ot NativeAOT runtime (Debug versus Release and so on).
Why not to unify CanCreateArbitraryDelegates
and CanEmitObjectArrayDelegate
to return RuntimeFeature.IsDynamicCodeSupported
and delete https://github.com/dotnet/runtime/blob/2aa0c9ba7f46048a113d214a2377cd2bf14ff29e/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets#L271-L273 ?
Why not to unify
CanCreateArbitraryDelegates
andCanEmitObjectArrayDelegate
to returnRuntimeFeature.IsDynamicCodeSupported
and delete?
Doing this for CanCreateArbitraryDelegates
is in progress / being tested here: https://github.com/dotnet/runtime/pull/86758
CanEmitObjectArrayDelegate is going to crash on mono because it doesn't implement the private CoreLib API the else paths rely on.
We can certainly make it not constant to work around the trimming limitations but there is no point to make this user overridable with appcontext because there's only one right answer per runtime configuration right now.
because there's only one right answer per runtime configuration right now
I don't understand this.
For example, IsDynamicCodeSupported
on CoreCLR is similar - the runtime always supports it, so it should return true
. But it is implemented via AppContext
. The reason is that if we're running a PublishAot=true
app on CoreCLR, we want consistent behavior, so we return IsDynamicCodeSupported == false
even though the runtime (CoreCLR) is perfectly capable of supporting.
How is CanEmitObjectArrayDelegate
any different. On runtimes where it can be true
, it should be read from AppContext so that it can be turned off based on feature switches.
It relies on a private API that only exists in NativeAOT CoreLib right now.
FYI - a related discussion here is https://github.com/dotnet/runtime/issues/81803#issuecomment-1454200820. It's been on my backlog to get that fixed in .NET 8.
FYI - a related discussion here is #81803 (comment). It's been on my backlog to get that fixed in .NET 8.
I don't see fixing that as likely to help in this situation - we're unlikely to move the Reflection.Emit part to corelib as part of this fix (due to Linq.Expressions being archived). So we'd still need something to tell whether to call secret CoreLib APIs or do reflection emit. We have multiple options:
string.Empty.Length == 0
). The CoreLib secret API lightup code will not be trimmed during a trimmed app build because ILLinker doesn't understand it at library build time, and doesn't understand it at app build time either. Probably fixable by adding a substitution if we care.
- Apps that do not trim will notice
For Apple devices, we don't care about size when not trimming (we never have).
For Apple devices, we don't care about size when not trimming (we never have).
In that case I would probably recommend removing the special build of Linq.Expressions for iOS-like platforms completely and use the same assembly everywhere. It should be possible to get the same results with feature switches and trimming.
From what I see, we currently do two special things or iOS-like:
Instead, we could expose the option to remove Ref.Emit backend publicly (doc on how to do that here: https://github.com/dotnet/runtime/blob/e685ff9e0e1a57480df05abda6be135b72c8cbe6/docs/workflow/trimming/feature-switches.md) and have the Xamarin SDK turn the public flag on to remove it when trimming.
Fix the ILLinker bug where at library build it does substitutions for things that could look different at app trim time.
Note that would also allow us to fix https://github.com/dotnet/runtime/issues/80398.
@MichalStrehovsky
CanEmitObjectArrayDelegate is going to crash on mono because it doesn't implement the private CoreLib API the else paths rely on.
Is the private CoreLib API
related to using bound delegates? (from the discussion here: https://github.com/dotnet/runtime/issues/78889#issuecomment-1406376532)
@MichalStrehovsky
CanEmitObjectArrayDelegate is going to crash on mono because it doesn't implement the private CoreLib API the else paths rely on.
Is the
private CoreLib API
related to using bound delegates? (from the discussion here: #78889 (comment))
The private API is to support creating an instance of a "magic" kind of delegate - given the type of the delegate to create delegateType
(e.g. Func<int, int>
) and an existing delegate in the shape of Func<object?[], object?>
, create an instance of delegateType
that when invoked, will put all it's argument in an object array and call the other delegate. I don't think it's related to the bound delegate issue - that looks like a more general issue not specific to Linq.Expressions.
@vargaz @LeVladIonescu is the functionality from Michal's previous comment:
The private API is to support creating an instance of a "magic" kind of delegate - given the type of the delegate to create delegateType (e.g. Func<int, int>) and an existing delegate in the shape of Func<object?[], object?>, create an instance of delegateType that when invoked, will put all it's argument in an object array and call the other delegate. I don't think it's related to the bound delegate issue - that looks like a more general issue not specific to Linq.Expressions.
covered with: https://github.com/dotnet/runtime/issues/83329 ?
Don't think so, thats a different issue.
Considering the risks of changing the ILLink constant propagation to take into account definitions from the substitution file and changing the MonoAOT behaviour to match NativeAOT, would it make sense to instead:
CanCreateArbitraryDelegates
CanEmitObjectArrayDelegate
CanCompileToIL
Linq.Expressions
library to utilize 1) where:
private static bool CanCreateArbitraryDelegates => RuntimeFeature.CanCreateArbitraryDelegates;
// ...
private static bool CanEmitObjectArrayDelegate => RuntimeFeature.CanEmitObjectArrayDelegate;
// ...
private static bool CanCompileToIL => RuntimeFeature.CanCompileToIL;
Set default values for the new feature switches from 1) and define the linker substitutions accordingly:
Feature switch | MonoAOT | NativeAOT |
---|---|---|
CanCreateArbitraryDelegates | true | false |
CanEmitObjectArrayDelegate | true | false |
CanCompileToIL | false | false |
This way we would:
Linq.Expressions
library buildLinq.Expressions
on all platformsLinq.Expressions.dll
built for iOS-like platforms would be the same for all supported runtimesAs a follow-up we will investigate and work on enabling MonoAOT to have the support for false
values for all three feature switches (and the codepaths false
values imply), so the above can be eventually replaced with IsDynamicCodeSupported
if it turns out to be feasible and does not introduce any regressions.
Would this make sense?
Disable IPConstprop during library build for iDevices. We already disable it elsewhere (to work around this issue). It likely increases the size of the assembly. Apps that use trimming will not notice because we'll trim with ipconstprop at app build time. Apps that do not trim will notice
What are the cons of following this approach? It would align the "iDevice" build with the rest of the platforms (all other builds have ILLinkDisableIPConstProp=true
).
I understand that it will result in a larger untrimmed assembly. But we should measure how much size are we actually talking about. Is it just a few KBs? And my understanding is that all "iDevice" apps will end up being trimmed at the app level anyway - so the resulting app sizes wouldn't be affected.
But we should measure how much size are we actually talking about. Is it just a few KBs?
It will be a size regression when not trimming because we'll no longer remove Ref.Emit-based expressions at library build time, but per https://github.com/dotnet/runtime/issues/87924#issuecomment-1608839320 it should not be a concern. We'll still trim it at app build time when trimming the app.
I have measured the Linq.Expressions.dll
size:
main
with disabling constant propagation for library build via the following diff1
:
diff --git a/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj b/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj
index 0d9a6e1c1c2..12152cb237d 100644
--- a/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj
+++ b/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj
@@ -16,7 +16,7 @@
with a wrong value at library build time and the substitution can still be selected at publish time.
For iOS/tvOS/Catalyst we prefer smaller size by default, so keep constprop enabled to get rid of the expression compiler.
-->
- <ILLinkDisableIPConstProp Condition="'$(TargetPlatformIdentifier)' != 'ios' and '$(TargetPlatformIdentifier)' != 'tvos' and '$(TargetPlatformIdentifier)' != 'maccatalyst'">true</ILLinkDisableIPConstProp>
+ <ILLinkDisableIPConstProp>true</ILLinkDisableIPConstProp>
</PropertyGroup>
<ItemGroup>
<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.xml" />
diff2
:
diff --git a/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj b/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj
index 0d9a6e1c1c2..6bc375fa637 100644
--- a/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj
+++ b/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj
@@ -10,13 +10,13 @@
<PropertyGroup>
<TargetPlatformIdentifier>$([MSBuild]::GetTargetPlatformIdentifier('$(TargetFramework)'))</TargetPlatformIdentifier>
<IsInterpreting Condition="'$(TargetPlatformIdentifier)' == 'ios' or '$(TargetPlatformIdentifier)' == 'tvos' or '$(TargetPlatformIdentifier)' == 'maccatalyst'">true</IsInterpreting>
- <ILLinkSubstitutionsLibraryBuildXml Condition="'$(IsInterpreting)' == 'true'">ILLink\ILLink.Substitutions.IsInterpreting.LibraryBuild.xml</ILLinkSubstitutionsLibraryBuildXml>
+ <!-- <ILLinkSubstitutionsLibraryBuildXml Condition="'$(IsInterpreting)' == 'true'">ILLink\ILLink.Substitutions.IsInterpreting.LibraryBuild.xml</ILLinkSubstitutionsLibraryBuildXml> -->
<!--
Disable constant propagation so that methods referenced from ILLink.Substitutions.xml don't get inlined
with a wrong value at library build time and the substitution can still be selected at publish time.
For iOS/tvOS/Catalyst we prefer smaller size by default, so keep constprop enabled to get rid of the expression compiler.
-->
- <ILLinkDisableIPConstProp Condition="'$(TargetPlatformIdentifier)' != 'ios' and '$(TargetPlatformIdentifier)' != 'tvos' and '$(TargetPlatformIdentifier)' != 'maccatalyst'">true</ILLinkDisableIPConstProp>
+ <ILLinkDisableIPConstProp>true</ILLinkDisableIPConstProp>
</PropertyGroup>
<ItemGroup>
<ILLinkSubstitutionsXmls Include="$(ILLinkDirectory)ILLink.Substitutions.xml" />
System.Linq.Expressions.dll | Size (b) | diff (b) | diff (%) |
---|---|---|---|
main | 448512 | NaN | NaN |
main+diff1 | 449024 | 512 | 0,11% |
main+diff2 | 561664 | 113152 | 25,09% |
NOTE: The diffs presented above are just for performing measurements
diff2
approach it will become a size regression since https://github.com/xamarin/xamarin-macios/issues/18340 is not yet implemented, and IsDynamicCodeSupported
is not set for Mono, meaning further that the assembly will not get trimmed during publish.I think diff1
seems like a good approach to try (along with updating the comment directly above the changed line). That is inline with what I was thinking.
@MichalStrehovsky as https://github.com/dotnet/runtime/pull/88539 got merged in, should we open an issue to change how the private NativeAOT feature switches are handled: https://github.com/dotnet/runtime/blob/2aa0c9ba7f46048a113d214a2377cd2bf14ff29e/src/coreclr/nativeaot/BuildIntegration/Microsoft.NETCore.Native.targets#L271-L273
There are two problems with this: 1) [bigger-issue] In Xamarin+NativeAOT build scenario even when https://github.com/dotnet/runtime/pull/88723 gets merged in, the library will still not be in the right shape once it reaches ILC during publish. The reason for that is that ILLink is being run before ILC, so its constant propagation during app deployment will again remove the conditional variables that NativeAOT expects to find in the assembly.
.targets
so that Xamarin SDK can pass them properly to ILLink. It would also help if these are promoted into actual feature switches handled by SDK, then these workarounds would not be necessary.
2) [smaller-issue] The feature switches for CanCompileToIL
and CanCreateArbitraryDelegates
have become redundant as these are now controlled via IsDynamicCodeSupported
and should/could be removed[smaller-issue] The feature switches for CanCompileToIL and CanCreateArbitraryDelegates have become redundant as these are now controlled via IsDynamicCodeSupported and should/could be removed
Yep, looks like these --feature
lines can now be deleted.
[bigger-issue] In Xamarin+NativeAOT build scenario
I think this one would be fixable by replacing the --feature
line with something along the lines of:
<ItemGroup>
<RuntimeHostConfigurationOption Include="System.Linq.Expressions.CanEmitObjectArrayDelegate"
Value="false"
Trim="true" />
</ItemGroup>
I think if we put this into the NativeAOT targets, it would affect both ILLink and NativeAOT. I wouldn't pull this into a feature switch one can control externally - there's only one correct value for this and there's little reason to make it configurable.
If this works, could you submit a PR?
Sure, I will try it out and ping you.
I am closing this issue as completed as all the identified work has been done and adequate tracking issues were created.
Description
There is a difference in how
System.Linq.Expressions.dll
is built for iOS-like and other desktop/mobile platforms, which is observable here:https://github.com/dotnet/runtime/blob/2aa0c9ba7f46048a113d214a2377cd2bf14ff29e/eng/illink.targets#L226
https://github.com/dotnet/runtime/blob/2aa0c9ba7f46048a113d214a2377cd2bf14ff29e/src/libraries/System.Linq.Expressions/src/System.Linq.Expressions.csproj#L19
Up until now this difference was not noticeable, however with enabling support for iOS-like platforms with NativeAOT we started experiencing:
Linq.Expressions
tests with NativeAOT on iOS-like platforms: https://github.com/dotnet/runtime/pull/87260/files#diff-4422ce02e9237f4f8e6021526506f87216778e497b33ce676872c1f70e1483c2R54System.Func`3
delegate instances take up to 18% of accountable size of a MAUI iOS app with NativeAOT (the cause explained bellow)Additionally, there were several issues reported against MonoAOT, struggling with
Linq.Expressions.Interpreter
reported here:Explanation
NativeAOT
The issue was called out by @MichalStrehovsky noting that codepaths in
Linq.Expressions
library controlled by:CanCompileToIL
CanEmitObjectArrayDelegate
CanCreateArbitraryDelegates
are not AOT friendly (reported here).
For desktop platforms, NativeAOT fixes this by:
Linq.Expressions.dll
When it comes to iOS-like platforms, above is not true. When
Linq.Expressions
library is built, constant propagation is enabled and control variables get removed during the library build. This further causes above-listed NativeAOT feature switches not to have any effect (fail to trim during app build), causing the AOT compilation to follow unsupported code paths which fail at runtime. Examples:Build warnings:
Test crash https://github.com/dotnet/runtime/blob/7b7d56fa3967a272c0f720ff0d9e38c4b2af369d/src/tests/nativeaot/SmokeTests/UnitTests/Delegates.cs#L54 on a iOS device:
MonoAOT
As for MonoAOT is concerned, there were several issues reported against the
Linq.Expressions.Interpreter
support. Some issues were fixed, some are still open, but in general following unfriendly AOT codepaths also hurts Mono. Example:includes generating code for a lot of generic delegates and Mono tries its best to support these, but at what cost?
It is true that choosing delegates to implement fast invocation of methods should have better performance, but in case with Mono and delegates over value types, the compiler will generate
GSHAREDVT
methods (generic sharing for value types) which are actually quite slow. On the other hand, NativeAOT does not have generic sharing for value types and generates instead all possible variations (causing the problem reported above 2) with a template MAUI app)Risk assessment
Pros
Linq.Expressions
on iOS-like platforms~30%
smaller MAUI template app~2.5%
smaller MAUI template app (the difference is way smaller compared to NativeAOT as Mono generates GSHAREDVT for fast invocation)Attempting to JIT compile method (wrapper delegate-invoke)
users had to enable mono interpreter in their projects (UseInterpreter=true
) to cover-up for missing methods during runtime, which also affects the application sizeCons
GSHAREDVT
in these casesProposal
Here is the list of tasks which should resolve all the reported issues around
Linq.Expressions
on iOS-like platformsThanks to @MichalStrehovsky @vargaz @rolfbjarne for helping to identify these issues.