Annotate framework assemblies as trimmable #48428

sbomer · 2021-02-17T23:11:37Z

This will opt them into trimming with the behavior described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#assemblymetadataistrimmable-true.

The linker part of this change is happening in dotnet/linker#1839.

@marek-safar @eerhardt @vitek-karas

This will opt them into trimming with the behavior described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#assemblymetadataistrimmable-true.

dotnet-issue-labeler · 2021-02-17T23:11:41Z

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

eerhardt · 2021-02-18T01:21:21Z

eng/versioning.targets

@@ -20,6 +20,9 @@
    <AssemblyMetadata Include="PreferInbox">
      <Value>True</Value>
    </AssemblyMetadata>
+    <AssemblyMetadata Include="IsTrimmable">


Are we sure we want to do this blindly? It feels like at least there should be a Condition to allow individual assemblies to turn it off.

Note that today we only analyze the assemblies in the shared fx / runtimepack for ILLink warnings. All OOB assemblies go unanalyzed. Once we have a story for analyzing libraries, we should hook dotnet/runtime up to it.

I can add a condition. Are there any in particular that you think I should turn off to start with? (Would it maybe make sense to turn it off for OOB assemblies and opt them in individually when we analyze them?)

Also, note that the feature as designed isn't tied to the warnings inherently - the attribute just says "trim me" - whether the assembly is linker-safe or not. That said, maybe it makes sense to opt out of trimming in cases where we don't trim a library during the runtime build because it uses too much reflection, like this:

runtime/src/libraries/System.Private.DataContractSerialization/src/System.Private.DataContractSerialization.csproj

Lines 7 to 8 in 75d5cc3



<ILLinkTrimAssembly>false</ILLinkTrimAssembly>

All libraries need to be trimmable in 6.0 but I'd probably exclude netfx build targets

All libraries need to be trimmable in 6.0

I agree we want to get there. But I think we should do it in a pragmatic way. Turning it on for all assemblies and letting people tell us what breaks doesn't seem like the best approach.

My thinking is that once we are done annotating the shared fx libraries, we start analyzing ILLink warnings for OOB libraries. When each library is "clean" of ILLink warnings, we mark it as "Trimmable".

For the initial change, I was assuming we would analyze System.IO.Pipelines, and assuming it is clean, we mark it as "IsTrimmable".

like this: System.Private.DataContractSerialization.csproj

That one is part of the shared fx, so it gets trimmed today in a user's app 🙃. When we get to that assembly, that comment and line will go away and we will trim it as part of the build like every other shared fx library.

Per discussion with @eerhardt, the plan is to opt all assemblies in as suggested by @marek-safar - and there's a new condition that we can use to opt out per project, should we need to.

By following the existing pattern for AssemblyMetadata like "Serviceable" and "PreferInbox", the attribute will be placed in src and ref assemblies, and also in assemblies targeting netstandard/netcoreapp/netfx. We need to include it for netstandard/netcoreapp because plenty of OOB assemblies build for those targets. Including the attributes in netfx assemblies shouldn't cause any issues since there are no common scenarios where an app targeting net6.0 references one of our netfx assemblies. I think it makes sense to be consistent with existing attributes on this, but @marek-safar if you prefer I could also disable it for netfx.

and ref assemblies

Are ref assemblies ever trimmed?

I think it makes sense to be consistent with existing attributes on this, but @marek-safar if you prefer I could also disable it for netfx.

I'm not against it, just wanted to be more cautious for netfx to not break bad code which could, for example, read only the last AssemblyMetadata but the chance is low.

Are ref assemblies ever trimmed?

No. But they are also never "serviced", and yet we are marking them with "Serviceable" assembly metadata. We should consistently apply these attributes across the assemblies.

No. But they are also never "serviced", and yet we are marking them with "Serviceable" assembly metadata. We should consistently apply these attributes across the assemblies.

FWIW starting .NET 5, we can now service the ref pack, we try to avoid it but we added support to be able to do it in case we need it, but I'm fine with adding IsTrimmable to them for consistency.

ghost · 2021-02-18T01:36:20Z

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

This will opt them into trimming with the behavior described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#assemblymetadataistrimmable-true.

The linker part of this change is happening in dotnet/linker#1839.

@marek-safar @eerhardt @vitek-karas

Author:	sbomer
Assignees:	-
Labels:	`area-Infrastructure-libraries`
Milestone:	-

ghost · 2021-02-18T01:36:26Z

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar, @tannergooding, @CoffeeFlux
See info in area-owners.md if you want to be subscribed.

Issue Details

This will opt them into trimming with the behavior described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#assemblymetadataistrimmable-true.

The linker part of this change is happening in dotnet/linker#1839.

@marek-safar @eerhardt @vitek-karas

Author:	sbomer
Assignees:	-
Labels:	`area-Infrastructure-libraries`, `size-reduction`
Milestone:	-

eerhardt · 2021-02-18T01:37:54Z

Will this resolve dotnet/aspnetcore#27782 by itself? Or will we need more work to fix that?

sbomer · 2021-02-18T01:52:48Z

If we opt System.IO.Pipelines in as part of the change, there shouldn't be any more work in runtime or aspnet, but we'll need to wait for dotnet/linker#1839 to flow, and for the upcoming SDK change that will include an update to use the new options.

eng/versioning.targets

src/libraries/System.IO.Pipelines/src/System.IO.Pipelines.csproj

A project just needs to set <SetIsTrimmable>false</SetIsTrimmable>, and it won't get the trimmable metadata. I also got rid of the SkipFrameworkAssemblyMetadata check since I don't think the IsTrimmable metadata should be tied to whether we are building a framework assembly, and this property was unused as far as I could tell.

eerhardt

Thanks @sbomer.

Note, after this gets plumbed all the way through, we should be able to remove the StartsWith('Microsoft.Extensions.') workaround in the blazor SDK (cc @pranavkm @captainsafia)

https://github.com/dotnet/sdk/blob/7029c8c218d113e69db39e69aae5a53efd26f244/src/BlazorWasmSdk/Targets/Microsoft.NET.Sdk.BlazorWebAssembly.Current.targets#L437-L439

safern · 2021-02-19T03:57:25Z

eng/versioning.targets

@@ -83,7 +86,7 @@
      </AssemblyAttribute>

      <AssemblyAttribute Include="System.Reflection.AssemblyMetadata"
-                         Condition="'$(SkipFrameworkAssemblyMetadata)' != 'true' and '@(AssemblyMetadata)' != ''">


Just curious, why are we removing the SkipFrameworkAssemblyMetadata condition?

It doesn't appear to be used.

https://github.com/dotnet/runtime/search?q=SkipFrameworkAssemblyMetadata

Also, I'm assuming it is for the case where IsTrimmable should be generated for the assembly, but someone adds SkipFrameworkAssemblyMetadata to the project. That would block IsTrimmable from getting generated.

Yup - see also the second commit message: d03327e

stephentoub · 2021-02-19T16:38:35Z

src/libraries/System.Private.CoreLib/src/Internal/AssemblyAttributes.cs

@@ -22,5 +22,6 @@

 [assembly: AssemblyMetadata("Serviceable", "True")]
 [assembly: AssemblyMetadata(".NETFrameworkAssembly", "")]


Completely unrelated to this PR, but it caught my eye... is this value still desired? Should the "Framework" piece be removed?
cc: @ericstj

Certain things look for this string. ex.

https://github.com/dotnet/corert/blob/24f1edc938af9954eac9a3385958787ca64956d5/src/ILCompiler.Compiler/src/Compiler/FrameworkStringResourceBlockingPolicy.cs#L58-L87

These assemblies now opt into trimming using AssemblyMetadata: dotnet/runtime#48428. The SDK side of the change was made in dotnet/sdk#16094. For more context, see https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#net-6.

[master] Update dependencies from dotnet/sdk - Coherency Updates: - Microsoft.NET.ILLink.Tasks: from 6.0.100-preview.2.21124.3 to 6.0.100-preview.2.21125.1 (parent: Microsoft.NET.Sdk) - Don't set IsTrimmable on netcoreapp in 6.0 These assemblies now opt into trimming using AssemblyMetadata: dotnet/runtime#48428. The SDK side of the change was made in dotnet/sdk#16094. For more context, see https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#net-6.

Annotate framework assemblies as trimmable

e9caaa7

This will opt them into trimming with the behavior described at https://github.com/mono/linker/blob/main/docs/design/trimmed-assemblies.md#assemblymetadataistrimmable-true.

eerhardt reviewed Feb 18, 2021

View reviewed changes

eerhardt added the area-Infrastructure-libraries label Feb 18, 2021

eerhardt added the size-reduction Issues impacting final app size primary for size sensitive workloads label Feb 18, 2021

eerhardt reviewed Feb 18, 2021

View reviewed changes

eng/versioning.targets Outdated Show resolved Hide resolved

eerhardt reviewed Feb 18, 2021

View reviewed changes

eng/versioning.targets Outdated Show resolved Hide resolved

eerhardt reviewed Feb 18, 2021

View reviewed changes

src/libraries/System.IO.Pipelines/src/System.IO.Pipelines.csproj Outdated Show resolved Hide resolved

sbomer force-pushed the trimmingOptIn branch from e90a6fb to d03327e Compare February 18, 2021 23:12

eerhardt mentioned this pull request Feb 18, 2021

Enable ILLink warning analysis for OOB libraries #48488

Closed

eerhardt approved these changes Feb 19, 2021

View reviewed changes

safern reviewed Feb 19, 2021

View reviewed changes

marek-safar mentioned this pull request Feb 19, 2021

Mark all Blazor runtime pack assemblies as trimmable dotnet/aspnetcore#30286

Closed

stephentoub reviewed Feb 19, 2021

View reviewed changes

stephentoub approved these changes Feb 19, 2021

View reviewed changes

sbomer merged commit 09a1256 into dotnet:master Feb 19, 2021

This was referenced Mar 3, 2021

ConfigurationManager is not trim compatible #49062

Closed

WindowsDesktop and ASP.NET are not considered linkable frameworks #49252

Closed

ghost locked as resolved and limited conversation to collaborators Mar 21, 2021

sbomer deleted the trimmingOptIn branch November 3, 2023 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Annotate framework assemblies as trimmable #48428

Annotate framework assemblies as trimmable #48428

sbomer commented Feb 17, 2021

dotnet-issue-labeler bot commented Feb 17, 2021

eerhardt Feb 18, 2021

sbomer Feb 18, 2021

sbomer Feb 18, 2021

marek-safar Feb 18, 2021

eerhardt Feb 18, 2021 •

edited

Loading

sbomer Feb 18, 2021

safern Feb 19, 2021

marek-safar Feb 19, 2021

eerhardt Feb 19, 2021

safern Feb 19, 2021

ghost commented Feb 18, 2021

ghost commented Feb 18, 2021

eerhardt commented Feb 18, 2021

sbomer commented Feb 18, 2021

eerhardt left a comment

safern Feb 19, 2021

eerhardt Feb 19, 2021

sbomer Feb 19, 2021

stephentoub Feb 19, 2021

eerhardt Feb 19, 2021

	<!-- Too much private reflection. Do not bother with trimming -->
	<ILLinkTrimAssembly>false</ILLinkTrimAssembly>

		@@ -22,5 +22,6 @@

		[assembly: AssemblyMetadata("Serviceable", "True")]
		[assembly: AssemblyMetadata(".NETFrameworkAssembly", "")]

Annotate framework assemblies as trimmable #48428

Annotate framework assemblies as trimmable #48428

Conversation

sbomer commented Feb 17, 2021

dotnet-issue-labeler bot commented Feb 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 18, 2021

ghost commented Feb 18, 2021

eerhardt commented Feb 18, 2021

sbomer commented Feb 18, 2021

eerhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt Feb 18, 2021 •

edited

Loading