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

Default values for feature switches in trimmed console apps #14475

Closed
vitek-karas opened this issue Nov 10, 2020 · 9 comments · Fixed by #18812
Closed

Default values for feature switches in trimmed console apps #14475

vitek-karas opened this issue Nov 10, 2020 · 9 comments · Fixed by #18812
Milestone

Comments

@vitek-karas
Copy link
Member

vitek-karas commented Nov 10, 2020

Runtime repo has been introducing feature switches which among other things help reduce application size when the app is trimmed. They also help avoid trim analysis warnings. This issue is to determine and implement the default values for various feature switches when trimming is turned on.

Ideally the experience would be that trimming a template console app doesn't produce any trim analysis warnings and the size of the app is as small as possible. Downside of turning off features by default is obviously potentially unexpected change of behavior of the app.

For description of the feature switches please refer to feature-switches.md

MSBuild Property Name Default Default when PublishTrimmed=true Notes
StartupHookSupport true false https://github.com/dotnet/sdk/pull/14432/files
DebuggerSupport true true (unchanged) Don't want to break diagnostics
EnableUnsafeUTF7Encoding false false (unchanged) Already turned off by default, so no-op
EnableUnsafeBinaryFormatterSerialization true / false in ASP.NET false The plan is to turn this off in future releases, so doing so for trimming in 6.0 seems OK
EventSourceSupport true true (unchanged) Don't want to break diagnostics
InvariantGlobalization false ??? Needs investigation how much it actually helps for console apps
UseSystemResourceKeys false ???
HttpActivityPropagationSupport true true (unchanged) Don't want to break diagnostics

TODO:

  • Measure size impact of each of these on a sample console app (so that we have data to drive the decisions)
  • Measure how many warnings are produced if these are left turned on
@vitek-karas vitek-karas added this to the 6.0.1xx milestone Nov 10, 2020
@vitek-karas
Copy link
Member Author

/cc @mateoatr @LakshanF @agocke

@eerhardt
Copy link
Member

My thoughts and opinions:

I think DebuggerSupport and EventSourceSupport are important enough that we should keep them defaulted to the same value in a trimmed app. Being able to diagnose issues in an application without rebuilding it is important IMO. I think HttpActivityPropagationSupport falls into this same category.

I believe the default for EnableUnsafeUTF7Encoding is false in .NET 5. See dotnet/docs#19274 and dotnet/runtime#37535. I've changed the table above. @GrabYourPitchforks can correct me if I'm wrong.

I believe EnableUnsafeBinaryFormatterSerialization is planned on getting its default changed in the future regardless of trimming or not. See https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/binaryformatter-obsoletion.md#binaryformatter-disabled-by-default-across-all-project-types-net-7 for the BinaryFormatter plan. Again, @GrabYourPitchforks can correct me here. We could consider changing this default in a trimmed app earlier (i.e. in .NET 6), since the technology is marked as obsolete and we don't want new apps using it. Removing it in trimmed apps by default reinforces this stance.

For InvariantGlobalization, for typical .NET apps that get their globalization data from data already installed on the machine (either NLS or ICU), I'm not sure changing the default has much value. However, for app models that need to bring the globalization data with the app (Blazor WASM, mobile, etc), it may make sense to change the default for those app models.

UseSystemResourceKeys - I could maybe see us changing the default value for this one. Typically exceptions thrown from the runtime aren't shown to the end user. That's why we haven't spent the time to localize the runtime exception messages into languages other than English. Given that these exception messages aren't shown to users, it may make sense to trim these resource strings by default in a trimmed app.

@vitek-karas
Copy link
Member Author

Thanks @eerhardt - that makes sense.
Leaving DebuggerSupport and EventSourceSupport on desirable. The problem is that these introduce trim analysis warnings, so we will need to find solutions for them.

I'm not sure about UseSystemResourceKeys - these include exceptions from all of framework not just the low-level runtime. On the other hand without it we will have to somehow deal with ResourceManager not being compatible with trimming (probably through another feature switch, since I can't see us actually "Fixing" it).

@vitek-karas
Copy link
Member Author

I updated the table with the suggestions from @eerhardt for the most part.

@vitek-karas
Copy link
Member Author

The ability to use PublishTrimmed=true to declare intent to trim the application and thus enable correct setting of defaults as described above is discussed in #14562.

@eerhardt
Copy link
Member

@vitek-karas - is there more work to do here?

@vitek-karas
Copy link
Member Author

Probably not - what we don't have is any documentation what are the defaults with/without trimming. PublishTrimmed=true effectively works as the "intent" currently (it can be used in dotnet build and doesn't break the world as it did before).

@sbomer - with your IsTrimmable changes - can you please look at this issue and at #14562 and see if there are any holes left, or we basically implemented all of the things described here?

@sbomer
Copy link
Member

sbomer commented May 17, 2021

I think we can close this issue.

Ideally the experience would be that trimming a template console app doesn't produce any trim analysis warnings

This is the case :)

Measure size impact of each of these on a sample console app (so that we have data to drive the decisions)
Measure how many warnings are produced if these are left turned on

I'm not sure we really did these, but I think we have case-by-case justification for each feature switch that we defaulted.

@sbomer - with your IsTrimmable changes - can you please look at this issue and at #14562 and see if there are any holes left, or we basically implemented all of the things described here?

I left a few comments there. I think the actionable remaining work is just docs and whether to rename the publish options.

@sbomer sbomer closed this as completed May 17, 2021
@eerhardt
Copy link
Member

eerhardt commented Jul 2, 2021

Re-opening since a few of the settings didn't get defaulted as specified above. Specifically:

  • EnableUnsafeUTF7Encoding = false
  • EnableUnsafeBinaryFormatterSerialization = false

We should also consider defaulting

  • MetadataUpdaterSupport = false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants