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

[trimmer] pass --disable-opt unusedtypechecks #9435

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Oct 21, 2024

Fixes: #9399
An alternative to: #9411

The .NET trimmer will inline cast statements such as:

var d = Application.Context.Resources.GetDrawable (resId);
Assert.IsNotNull (d as NinePatchDrawable);

If NinePatchDrawable is not "instantiated" anywhere via new(), inlined to:

var d = Application.Context.Resources.GetDrawable (resId);
Assert.IsNotNull (null); // BOOM!

We can solve this by passing --disable-opt unusedtypechecks, with $(_AndroidEnableUnusedTypeChecks) as a private MSBuild property to opt out if needed.

Since Java can create these objects, this feels like the reasonable fix for .NET 9 GA.

I also fixed the $(RuntimeIdentifiers) in TestApks.targets to check if someone passed -r android-arm64, for example.

@jonathanpeppers
Copy link
Member Author

The trimmer issue looks OK:

image

As well as the apk size regression test:

image

@@ -18,7 +18,7 @@

<PropertyGroup>
<!-- APK tests might run on 32-bit emulators -->
<RuntimeIdentifiers>android-arm64;android-x86;android-x64;</RuntimeIdentifiers>
<RuntimeIdentifiers Condition=" '$(RuntimeIdentifier)' == '' ">android-arm64;android-x86;android-x64;</RuntimeIdentifiers>
Copy link
Member

Choose a reason for hiding this comment

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

Please update the PR description to explain/describe this change as well.

@vitek-karas
Copy link
Member

@sbomer FYI

I must admit I'm not a fan of this solution. It basically says that we know there are trimming holes in the system and we're going to try to avoid them by disabling certain optimizations in the trimmer. The problem is that trimmer may make similar assumptions elsewhere and we didn't catch it yet - or it will start to do so in the next release for example.
I'd say as a stop-gap solution for .NET 9, this is OKish, but we should really fix this as soon as possible.
Especially in light of the full trim mode "support", which will likely make this problem just worse.

@jonpryor
Copy link
Member

Linux tests keep failing for "bizarre" reasons:

Workload installation failed: Version 35.0.1-ci.pr.gh9435.57 of package microsoft.android.sdk.linux is not found in NuGet feeds /mnt/vss/_work/1/s/bin/BuildRelease/nuget-unsigned/;https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-android-df9aaf29-1/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9-transport/nuget/v3/index.json;https://pkgs.dev.azure.com/xamarin/public/_packaging/Xamarin.Android/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-emsdk-91b783ed/nuget/v3/index.json;https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-runtime-ef07c4f2/nuget/v3/index.json.
##[error]build-tools/create-packs/Directory.Build.targets(133,5): Error MSB3073: The command ""/mnt/vss/_work/1/s/bin/Release/dotnet/dotnet" workload install android --skip-manifest-update --skip-sign-check --verbosity diag --source "/mnt/vss/_work/1/s/bin/BuildRelease/nuget-unsigned/" --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-android-df9aaf29-1/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-android-df9aaf29-1/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-public/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-eng/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet-tools/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet8-transport/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9-transport/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/dotnet9-transport/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/xamarin/public/_packaging/Xamarin.Android/nuget/v3/index.json"](https://pkgs.dev.azure.com/xamarin/public/_packaging/Xamarin.Android/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-emsdk-91b783ed/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-emsdk-91b783ed/nuget/v3/index.json%22) --source "[https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-runtime-ef07c4f2/nuget/v3/index.json"](https://pkgs.dev.azure.com/dnceng/public/_packaging/darc-pub-dotnet-runtime-ef07c4f2/nuget/v3/index.json%22) --temp-dir "obj/Release/.xa-workload-temp-r4whol04.5k5"" exited with code 1.

Retrying the whole thing.

@jonpryor
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jonpryor jonpryor merged commit b90c559 into main Oct 22, 2024
43 of 58 checks passed
@jonpryor jonpryor deleted the dev/peppers/unusedtypechecks branch October 22, 2024 13:39
@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Oct 22, 2024

@sbomer FYI

I must admit I'm not a fan of this solution. It basically says that we know there are trimming holes in the system and we're going to try to avoid them by disabling certain optimizations in the trimmer. The problem is that trimmer may make similar assumptions elsewhere and we didn't catch it yet - or it will start to do so in the next release for example. I'd say as a stop-gap solution for .NET 9, this is OKish, but we should really fix this as soon as possible. Especially in light of the full trim mode "support", which will likely make this problem just worse.

@vitek-karas yes, @sbomer and I chose this as a better idea than #9411. We would like to find a better solution later, especially if there is NativeAOT support for Android. Perhaps .NET 10?

@sbomer
Copy link
Member

sbomer commented Oct 22, 2024

I don't think this is a reliable long-term solution, but my understanding from conversation with @jonathanpeppers is that we don't have a way to know ahead of time which types may be created from java.

For future reference, this fix is effectively adding a heuristic that says "if there is a type check for a type (and it implements IJavaObject), then assume it may be created from the java side". If we had a source of truth that could tell us which IJavaObject implementors need to be kept, we could rely on that instead and the type check removal optimization wouldn't cause problems.

@github-actions github-actions bot locked and limited conversation to collaborators Nov 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C# type of Preference trimmed away in Release builds, causing NullReferenceException
5 participants