Default new runtime feature switches #23932
Conversation
These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects. Fix dotnet#23716
ghost
added
the
area-mvc
| @@ -25,6 +25,14 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| <!-- Trimmer defaults --> | |||
| <PublishTrimmed Condition="'$(PublishTrimmed)' == ''">true</PublishTrimmed> | |||
| <TrimMode Condition="'$(TrimMode)' == ''">link</TrimMode> | |||
| <TrimmerRemoveSymbols Condition="'$(TrimmerRemoveSymbols)' == ''">false</TrimmerRemoveSymbols> | |||
There was a problem hiding this comment.
The SDK defaults TrimmerRemoveSymbols based on DebuggerSupport. If DebuggerSupport is false, then the linker will remove the symbols. Since the Blazor SDK removes symbols from the publish output, I thought it was reasonable to always default TrimmerRemoveSymbols=false so even if DebuggerSupport is false in Release mode, we still get the linked symbols, in case the developer wants to archive the linked symbols.
There was a problem hiding this comment.
Seems reasonable to me - I doubt that processing symbols significantly changes link time. Curious that the logic to remove symbols from publish isn't shared.
There was a problem hiding this comment.
If the trimmer already has support for removing symbols, it might be useful to just piggy back on it. Would it help if we removed BlazorWebAssemblyEnableDebugging and defaulted DebuggerSupport to false?
There was a problem hiding this comment.
Would it help if we removed BlazorWebAssemblyEnableDebugging
I think this makes sense - might as well use the established properties in the core SDK and the runtime.
and defaulted DebuggerSupport to false?
Is there ever a case where you'd want to preserve .pdbs for later debugging or perf sampling? I guess if users wanted symbols (but wanted to trim out the debug-only code for smaller apps) they could just set TrimmerRemoveSymbols themselves. I guess the current default behavior in Blazor WASM is to not get symbols. So I think that would be fine.
There was a problem hiding this comment.
In general I agree, only one concern:
- People will probably not realize that the build doesn't produce PDBs at the beginning
- They will only find out once they actually need the PDB for anything... but then it's too late as the deployed builds of the app won't have it
- Making it opt-in solves the problem only partially (for those who can redeploy and repro the issue)
I guess it depends on how we think people we use the PDBs for Blazor (and if they bring any value).
For example for server .NET Core apps (ASP.NET) I personally would definitely vote for always producing PDBs and just not bundling them to the final package.
There was a problem hiding this comment.
I guess it depends on how we think people we use the PDBs for Blazor (and if they bring any value).
One place this brings value is when we start producing linker warnings. If the pdb is available, the linker will give source and line info in the warning. The developer will see the exact line of code causing the warning. That's super useful IMO.
There was a problem hiding this comment.
My thinking is that having the .pdbs available somewhere - e.g. in the obj folder - by default, even if they are removed from the published assets, is useful. Honestly, I'm not sure why dotnet publish puts symbols in the publish folder by default.
There was a problem hiding this comment.
My thinking is that having the .pdbs available somewhere - e.g. in the obj folder - by default, even if they are removed from the published assets, is useful.
Those should still be around. We don't prevent creating the pdbs, just stop it from being copied to the publish output.
If the pdb is available, the linker will give source and line info in the warning
Couldn't the linker still use pdbs and then discard these?
There was a problem hiding this comment.
Couldn't the linker still use pdbs and then discard these?
If TrimmerRemoveSymbols is set to true, the linker won't update the symbols for the linked assemblies. Having the pre-trimmed symbols won't do you any good if you are using the trimmed assemblies - they won't match.
There was a problem hiding this comment.
Re source line info from linker: For this the linker needs the pdbs on the input - it doesn't matter if the linker itself will not write a pdb for this feature. That said I agree that having the pdbs (after linking) around somewhere is a good thing.
| <UseSystemResourceKeys Condition="'$(UseSystemResourceKeys)' == ''">true</UseSystemResourceKeys> | ||
| <EnableUnsafeUTF7Encoding Condition="'$(EnableUnsafeUTF7Encoding)' == ''">false</EnableUnsafeUTF7Encoding> | ||
| <HttpActivityPropagationSupport Condition="'$(HttpActivityPropagationSupport)' == ''">false</HttpActivityPropagationSupport> | ||
| <DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' == 'Release' and '$(BlazorWebAssemblyEnableDebugging)' != 'true'">false</DebuggerSupport> |
There was a problem hiding this comment.
Any thoughts on the rule here? Removing "debug only" code, I've seen over 100 KB (uncompressed) size savings in a default template app.
There was a problem hiding this comment.
I think this is perfect:
- In Release builds we want as small as possible (and debugging is definitely optional in that case)
- In Debug builds we want nice debugging UX
| <EventSourceSupport Condition="'$(EventSourceSupport)' == ''">false</EventSourceSupport> | ||
| <UseSystemResourceKeys Condition="'$(UseSystemResourceKeys)' == ''">true</UseSystemResourceKeys> | ||
| <EnableUnsafeUTF7Encoding Condition="'$(EnableUnsafeUTF7Encoding)' == ''">false</EnableUnsafeUTF7Encoding> |
There was a problem hiding this comment.
Nit: I know it's not related to this change, but it just caught my eye... the naming is so inconsistent. We have "SomethingSupport" then "UseSomething" and then also "EnableSomething". Would be nice to have some system to this.
There was a problem hiding this comment.
The difference between "SomethingSupport" and "EnableSomething" is what the default behavior is. By default, "EventSource", "Debugger", and "HttpActivityPropagation" are all supported by default. UTF7 Encoding is disabled by default, and you need to enable it to use it.
Since the default is enabled, it wouldn't make sense to say EnableDebugger. The corresponding word would be DisableDebugger, but I got feedback in dotnet/runtime#37288 (comment) to make it a positive name, and not a negative name. So I followed the same naming with EventSource and HttpActivityPropagation.
Side Note on EnableUnsafeUTF7Encoding: ILLink.Substitutions + default feature switch values don't work very well with the way we link the runtime libraries during the build. We would need a way to ignore the substitution during the runtime build, but still embed it into the assembly. I had a conversation with @sbomer about this, and we decided that for now, setting EnableUnsafeUTF7Encoding to the default value here is probably the best option for 5.0.
UseSystemResourceKeys is a bit of an outlier. It isn't a "support" or an "enable". It kind of fits in an "Other" category with InvariantGlobalization.
Any suggestions on how to make this better?
There was a problem hiding this comment.
Maybe EnableSystemResourceTrim - but it's not exactly the same...
Honestly I'm just glad we obviously thought about it 😄
Re having susbtitutions which are not applied during library build - I would have to think about this some more, but I'm definitely willing to add something to the linker to make this possible. CoreFx is special in that we ship the linked binaries, but I do expect other libraries to run linker for the purposes of analysis - so they might run into similar problem.
|
Hello @pranavkm! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit * Default new runtime feature switches These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects. Fix dotnet#23716 * PR feedback
Porting #23932 to WebAssemblySDK. * Default new runtime feature switches These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects. Fixes #25099 If there is an ask mode template to fill out, let me know and I can do it.
Porting #23932 to WebAssemblySDK. * Default new runtime feature switches These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects. Fixes #25099 If there is an ask mode template to fill out, let me know and I can do it.
Porting #23932 to WebAssemblySDK. * Default new runtime feature switches These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects. Fixes #25099 If there is an ask mode template to fill out, let me know and I can do it.
* Default new runtime feature switches These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects. Fix dotnet/aspnetcore#23716 * PR feedback Commit migrated from dotnet/aspnetcore@7b42cf1275b2
…t/aspnetcore#23987) * Default new runtime feature switches These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects. Fix dotnet/aspnetcore#23716 * PR feedback Commit migrated from dotnet/aspnetcore@427bfc8d6b05
These new feature switches have been added to the runtime to make applications smaller. Setting reasonable defaults to Blazor wasm projects.
Fix #23716
See related SDK change to add the last 2 properties: dotnet/sdk#12457