Add basic handling of OptimizationPreference for crossgen2 on mobile r2r#130300
Add basic handling of OptimizationPreference for crossgen2 on mobile r2r#130300BrzVlad wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the Crossgen2 MSBuild targets used for ReadyToRun publishing to better align mobile (iOS-like + Android) behavior with OptimizationPreference, reduce duplicated RID checks, and adjust default stripping behavior for mobile builds.
Changes:
- Introduces
_IsIOSLikeRid/_IsAndroidRidhelper properties and uses them to simplify mobile RID conditionals. - Defaults Crossgen2 to size-focused optimization (
--Os) on mobile whenOptimizationPreferenceis unset orSize, and uses--Otwhen it isSpeed(only when$(Optimize)is true). - Applies Crossgen2
--strip-debug-info/--strip-inlining-infoon Android as well (and disables these strip flags for Debug / DebuggerSupport builds), while keeping--strip-il-bodiesiOS-like only.
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(PublishReadyToRunStripDebugInfo)' != 'false' and $(RuntimeIdentifier.StartsWith('iossimulator-'))">$(PublishReadyToRunCrossgen2ExtraArgs);--strip-debug-info</PublishReadyToRunCrossgen2ExtraArgs> | ||
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(PublishReadyToRunStripDebugInfo)' != 'false' and $(RuntimeIdentifier.StartsWith('tvossimulator-'))">$(PublishReadyToRunCrossgen2ExtraArgs);--strip-debug-info</PublishReadyToRunCrossgen2ExtraArgs> | ||
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(PublishReadyToRunStripDebugInfo)' != 'false' and $(RuntimeIdentifier.StartsWith('maccatalyst-'))">$(PublishReadyToRunCrossgen2ExtraArgs);--strip-debug-info</PublishReadyToRunCrossgen2ExtraArgs> | ||
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(PublishReadyToRunStripInliningInfo)' != 'false' and '$(DebuggerSupport)' != 'true' and '$(Configuration)' != 'Debug' and ('$(_IsIOSLikeRid)' == 'true' or '$(_IsAndroidRid)' == 'true')">$(PublishReadyToRunCrossgen2ExtraArgs);--strip-inlining-info</PublishReadyToRunCrossgen2ExtraArgs> |
There was a problem hiding this comment.
The properties here get duplicated in the command since they also come from the sdk, once the sdk gets updated. This enables imediate testing but I'm wondering if this code should be removed in the future, once we bump to a preview containing the corresponding sdk changes.
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(Optimize)' == 'true' and ('$(_IsIOSLikeRid)' == 'true' or '$(_IsAndroidRid)' == 'true') and ('$(OptimizationPreference)' == 'Size' or '$(OptimizationPreference)' == '')">--Os;$(PublishReadyToRunCrossgen2ExtraArgs)</PublishReadyToRunCrossgen2ExtraArgs> | ||
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(Optimize)' == 'true' and ('$(_IsIOSLikeRid)' == 'true' or '$(_IsAndroidRid)' == 'true') and '$(OptimizationPreference)' == 'Speed'">--Ot;$(PublishReadyToRunCrossgen2ExtraArgs)</PublishReadyToRunCrossgen2ExtraArgs> |
There was a problem hiding this comment.
Just for my understanding - why did you choose --Os over --Ot as the default optimization preference?
|
Copilot says that this PR is causing some failures on iOS + macCatalyst because of the new |
@kotlarmilos Any idea about this before I try to reproduce ? Seems weird since this options should be part of P5, so reasonably well tested already |
No, I haven't seen this before. Could it happen if stripping is performed twice over the same assembly? |
This shouldn't be the case, it is already enabled in P5 on Apple mobile |
|
I'm trying to reproduce the failures locally, will keep you updated. |
|
Note 🤖 This comment was authored by GitHub Copilot (CLI) at the request of the account owner. Root cause of the
But the iOS functional-test R2R build compiles in non-composite mode (per-assembly, |
|
@davidnguyen-tech Thanks, this is good direction!
Correct, this PR adds --strip-il-bodies to the general PublishReadyToRunCrossgen2ExtraArgs, which the RunReadyToRunCompiler task appends to every crossgen2 invocation. The SDK only ever adds it through Crossgen2CompositeExtraCommandLineArgs, gated on PublishReadyToRunComposite == true.
This could be a bug in how the functional tests are built where the composite mode isn't set properly. I confirmed the libraries and runtime tests run in composite mode. |
|
Tried to reproduce locally but failed. The functional tests are weird: https://github.com/dotnet/runtime/blob/main/src/tests/FunctionalTests/iOS/Simulator/CoreCLR/iOS.CoreCLR.R2R.Test.csproj, they set |
7fd70df to
9fac3a5
Compare
| nodeFactoryFlags.StripILBodies = Get(_command.StripILBodies); | ||
| if (nodeFactoryFlags.StripILBodies && !composite) | ||
| { | ||
| throw new Exception(SR.ErrorStripILBodiesRequiresComposite); |
This also makes the r2r use the size opt (--Os) as a default on mobile. Refactor repeated rid check by using `_IsIOSLikeRid` like we do in the sdk. Disable the strip flags on debug builds, to match the sdk Enable --strip-debug-info and --strip-inlining-info on android release as well. --strip-il-bodies is not enabled because IL should still be needed for tiered jit.
Not supported yet
264b1d2 to
40cf589
Compare
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(PublishReadyToRunStripDebugInfo)' != 'false' and '$(DebuggerSupport)' != 'true' and '$(Configuration)' != 'Debug' and ('$(_IsIOSLikeRid)' == 'true' or '$(_IsAndroidRid)' == 'true')">$(PublishReadyToRunCrossgen2ExtraArgs);--strip-debug-info</PublishReadyToRunCrossgen2ExtraArgs> | ||
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(PublishReadyToRunStripILBodies)' != 'false' and '$(DebuggerSupport)' != 'true' and '$(Configuration)' != 'Debug' and '$(_IsIOSLikeRid)' == 'true'">$(PublishReadyToRunCrossgen2ExtraArgs);--strip-il-bodies</PublishReadyToRunCrossgen2ExtraArgs> | ||
|
|
||
| <PublishReadyToRunCrossgen2ExtraArgs Condition="'$(Optimize)' == 'true' and ('$(_IsIOSLikeRid)' == 'true' or '$(_IsAndroidRid)' == 'true') and ('$(OptimizationPreference)' == 'Size' or '$(OptimizationPreference)' == '')">--Os;$(PublishReadyToRunCrossgen2ExtraArgs)</PublishReadyToRunCrossgen2ExtraArgs> |
There was a problem hiding this comment.
@MichalStrehovsky Should we add Balanced to differentiate from unset here? Then for mobiles unset would be size but otherwise it'd be Balanced.
There was a problem hiding this comment.
That depends on how crossgen wants to document this. Defaulting to size already doesn't match the native AOT behaviors or documentation, so crossgen can do whatever it wants.
There was a problem hiding this comment.
The native AOT docs literally say setting this to Size will prefer size at the cost of any other metric (startup, memory, etc.) and the user is supposed to measure whether the compromise is worth it to them.
This also makes the r2r use the size opt (
--Os) as a default on mobile.Refactor repeated rid check by using
_IsIOSLikeRidlike we do in the sdk.Disable the strip flags on debug builds, to match the sdk.
Enable
--strip-debug-infoand--strip-inlining-infoon android release as well.--strip-il-bodiesis not enabled because IL should still be needed for tiered jit.