- 
                Notifications
    You must be signed in to change notification settings 
- Fork 561
          [Mono.Android] trimmer feature switches for DotNetRuntimeType
          #10296
        
          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
Conversation
|  | ||
| [FeatureSwitchDefinition ($"{FeatureSwitchPrefix}{nameof (IsMonoRuntime)}")] | ||
| internal static bool IsMonoRuntime { get; } = | ||
| AppContext.TryGetSwitch ($"{FeatureSwitchPrefix}{nameof (IsMonoRuntime)}", out bool isEnabled) ? isEnabled : IsMonoRuntimeEnabledByDefault; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should enforce that the feature switch is always set explicitly and throw if it isn't. I'm worried that we could introduce a regression and for example we would drop the Microsoft.Android.Runtime.RuntimeFeature.IsMonoRuntime on CoreCLR in the SDK and now both IsMonoRuntime and IsCoreClrRuntime would be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add something that throws on startup if both are set to true.
        
          
                ...ndroid.Build.Tasks/Microsoft.Android.Sdk/targets/Microsoft.Android.Sdk.RuntimeConfig.targets
              
                Outdated
          
            Show resolved
            Hide resolved
        
      Context: #10198 (comment) `ManagedValueManager` is not currently trimmed away in Android apps running on Mono. We can make this happen by: * Remove `DotNetRuntimeType` enum * Introduce `RuntimeFeature.IsMonoRuntime` and `RuntimeFeature.IsCoreClrRuntime` * Set switches by default for each runtime: <ItemGroup> <RuntimeHostConfigurationOption Include="Microsoft.Android.Runtime.RuntimeFeature.IsMonoRuntime" Value="false" Trim="true" /> <RuntimeHostConfigurationOption Include="Microsoft.Android.Runtime.RuntimeFeature.IsCoreClrRuntime" Value="true" Trim="true" /> </ItemGroup> I have not yet seen a need within the code to introduce `RuntimeFeature.IsNativeAotRuntime`, but we could do so in the future. Currently, NativeAOT works via the other two runtime feature switches being `false` and then none of the problematic code paths are hit.
f615d39    to
    d834c56      
    Compare
  
    | } | ||
| }, | ||
| "PackageSize": 3148309 | ||
| "PackageSize": 3090965 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like it solves the minor app size regression from:
The number we used to have was 3078677, but that could have moved around slightly from other changes.
Context: #10198 (comment)
ManagedValueManageris not currently trimmed away in Android apps running on Mono.We can make this happen by:
Remove
DotNetRuntimeTypeenumIntroduce
RuntimeFeature.IsMonoRuntimeandRuntimeFeature.IsCoreClrRuntimeSet switches by default for each runtime:
I have not yet seen a need within the code to introduce
RuntimeFeature.IsNativeAotRuntime, but we could do so in the future.Currently, NativeAOT works via the other two runtime feature switches being
falseand then none of the problematic code paths are hit.