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.

Sign up for GitHub

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

Jump to bottom

Stop using undocumented attribute trimming #9060

Closed
sbomer opened this issue Jun 27, 2024 · 5 comments · Fixed by #9062
Closed

Stop using undocumented attribute trimming #9060

sbomer opened this issue Jun 27, 2024 · 5 comments · Fixed by #9062
Assignees
@jonathanpeppers
Labels
Area: App+Library Build Issues when building Library projects or Application projects. Area: Linker Issues when linking assemblies.

Comments

@sbomer
Copy link
Member

sbomer commented Jun 27, 2024

Android framework version

net9.0-android

Affected platform version

.NET 9

Description

Android should consider removing the _AggressiveAttributeTrimming setting, see dotnet/runtime#88805.

Android targets are using the undocumented _AggressiveAttributeTrimming setting which cannot be statically validated with trim warnings. The use of this option was introduced in #6563 together with NullabilityInfoContextSupport which has been removed in dotnet/runtime#103970, so it's not clear how much size savings come from _AggressiveAttributeTrimming on its own.

Steps to Reproduce

n/a

Did you find any workaround?

No response

Relevant log output

No response
@sbomer sbomer added the needs-triage Issues that need to be assigned. label Jun 27, 2024
@sbomer sbomer mentioned this issue Jun 27, 2024
Closed
@jpobst jpobst assigned jonathanpeppers and unassigned jpobst Jun 27, 2024
@jpobst jpobst added Area: App+Library Build Issues when building Library projects or Application projects. Area: Linker Issues when linking assemblies. and removed needs-triage Issues that need to be assigned. labels Jun 27, 2024
@jonathanpeppers
Copy link
Member

I guess this was added in 60e983c, so it's been here since .NET 6.

This means we shouldn't use any _-prefixed switch listed here?

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jun 28, 2024
@jonathanpeppers
[trimming] remove $(_AggressiveAttributeTrimming) by default
8d7808a 
Fixes: dotnet#9060

This was introduced in 60e983c (.NET 6 timeframe).

It is a "private" switch that is no longer recommended.
@jonathanpeppers jonathanpeppers mentioned this issue Jun 28, 2024
Merged
@jonathanpeppers
Copy link
Member

The only other undocumented one is $(_SystemDiagnosticsMetricsMeterIsSupported). We did enable that on purpose as it contributed to a startup time regression in .NET 8.

@jonathanpeppers
Copy link
Member

It looks like a medium-sized app is about 77kb bigger and hello world android 20kb bigger.

That seems reasonable if there is concern of something breaking from the setting.

@MichalStrehovsky
Copy link
Member

The only other undocumented one is $(_SystemDiagnosticsMetricsMeterIsSupported). We did enable that on purpose as it contributed to a startup time regression in .NET 8.

This looks to be implemented by Microsoft.Android.Sdk.RuntimeConfig.targets, so it's not a "private" of the SDK, it's a private of Xamarin.

I would suggest switching to the MetricsSupport property and deleting all of _SystemDiagnosticsMetricsMeterIsSupported. The other one is public and documented and it looks like these two properties do the exact same thing:

https://github.com/search?q=org%3Adotnet+System.Diagnostics.Metrics.Meter.IsSupported&type=code

jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this issue Jul 1, 2024
@jonathanpeppers
[trimming] use public $(MetricsSupport) property
0cb05a4 
Context: dotnet#9060 (comment)
Context: https://github.com/dotnet/sdk/blob/e18cfb7a09d74952d5e9c2448d31dee313e059bb/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L522-L525

In eb6397d, we introduced `$(_SystemDiagnosticsMetricsMeterIsSupported)`
to avoid a startup time regression with `HttpClient`-related code. At
the time, the public property `$(MetricsSupport)` had not been
introduced yet in the .NET SDK.

Let's use `$(MetricsSupport)` instead of the private
`$(_SystemDiagnosticsMetricsMeterIsSupported)` property.
@jonathanpeppers jonathanpeppers mentioned this issue Jul 1, 2024
Merged
@jonathanpeppers
Copy link
Member

I opened:

jonathanpeppers added a commit that referenced this issue Jul 1, 2024
@jonathanpeppers
[trimming] use public $(MetricsSupport) property (#9068)
200f843 
Context: #9060 (comment)
Context: https://github.com/dotnet/sdk/blob/e18cfb7a09d74952d5e9c2448d31dee313e059bb/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets#L522-L525

In eb6397d, we introduced `$(_SystemDiagnosticsMetricsMeterIsSupported)`
to avoid a startup time regression with `HttpClient`-related code. At
the time, the public property `$(MetricsSupport)` had not been
introduced yet in the .NET SDK.

Let's use `$(MetricsSupport)` instead of the private
`$(_SystemDiagnosticsMetricsMeterIsSupported)` property.
@github-actions github-actions bot locked and limited conversation to collaborators Aug 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@jonathanpeppers jonathanpeppers

Labels
Area: App+Library Build Issues when building Library projects or Application projects. Area: Linker Issues when linking assemblies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[trimming] remove $(_AggressiveAttributeTrimming) by default jonathanpeppers/xamarin-android
4 participants
@jpobst @sbomer @jonathanpeppers @MichalStrehovsky