Enable ILLink for all library configurations

[ericstj]

Copying from https://github.com/dotnet/runtime/issues/45665

The suggestion was made to run the ILLinker to trim dead code from all libraries, not just those libraries that make up the shared framework. Here's an excerpt of the discussion.

It was a scoping decision back when we first added it. Folks weren't willing to test the output of things that weren't going in the shared framework (eg: UAP assemblies, packages that would be used by desktop, etc) so we constrained it to only the shared framework. Folks were confident that we had better test coverage of the shared framework since that's what we were shipping at the time. We could broaden that scope but we'd probably want to make sure such a change had decent test coverage + auditing of the new assemblies it touched across all their supported frameworks.

This still applies, we don't test all package configurations. I think the linker is much more stable now and we could enable it. I think when doing so we'd want to audit what it does to non-tested configurations. That could be done with Assembly diffing. If the changes the linker made matched those made to the netcoreapp build we could be reasonably confident that the netcoreapp build would remain a good proxy for the downlevel builds. If we noticed any significant differences we could do targeted testing / investigation to ensure we have correct linker configurations for those configurations. This will also enable the linker on completely OOB assemblies where it never ran before (CodeDom, System.Management, MEF, etc) and where we haven't invested in test coverage. We'd want to do some manual diffing of these to validate that removals are safe, potentially adding test coverage to touch impacted codepaths.

Do folks think this effort is worth the cost?

[ghost]

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

Issue Details

---

Copying from https://github.com/dotnet/runtime/issues/45665 The suggestion was made to run the ILLinker to trim dead code from all libraries, not just those libraries that make up the shared framework. Here's an excerpt of the discussion. > It was a scoping decision back when we first added it. Folks weren't willing to test the output of things that weren't going in the shared framework (eg: UAP assemblies, packages that would be used by desktop, etc) so we constrained it to only the shared framework. Folks were confident that we had better test coverage of the shared framework since that's what we were shipping at the time. We could broaden that scope but we'd probably want to make sure such a change had decent test coverage + auditing of the new assemblies it touched across all their supported frameworks. This still applies, we don't test all package configurations. I think the linker is much more stable now and we could enable it. I think when doing so we'd want to audit what it does to non-tested configurations. That could be done with Assembly diffing. If the changes the linker made matched those made to the netcoreapp build we could be reasonably confident that the netcoreapp build would remain a good proxy for the downlevel builds. If we noticed any significant differences we could do targeted testing / investigation to ensure we have correct linker configurations for those configurations. This will also enable the linker on completely OOB assemblies where it never ran before (CodeDom, System.Management, MEF, etc) and where we haven't invested in test coverage. We'd want to do some manual diffing of these to validate that removals are safe, potentially adding test coverage to touch impacted codepaths. Do folks think this effort is worth the cost?

Author:	ericstj
Assignees:	-
Labels:	`area-Infrastructure-libraries`, `untriaged`
Milestone:	-

[eerhardt]

One thought that I have in this area is: If we think it is valuable for our libraries outside of the shared framework, it is also valuable to libraries outside of dotnet/runtime. Which, to me, sounds like it should be a feature in the official product.

cc @sbomer @vitek-karas

[ViktorHofer]

cc @stephentoub as he was the one who asked the question of why we don't run ILLink on all tfms.

[ericstj]

sounds like it should be a feature in the official product.

Technically I think you could set up something similar using the real product. I think the question of productizing dead-code trimming is a different issue and I'd not like to fork this issue to have that discussion. @eerhardt if you'd like to make such a request please file a different issue.

Lets leave this issue to address the question of enabling dead-code trimming on all the libs produced by dotnet/runtime.

[stephentoub]

Do folks think this effort is worth the cost?

I think it'd be worthwhile to do so, even if it's piecemeal. For example, some of the assemblies ship as part of Microsoft.AspNetCore.App, and would contribute similar benefits to why we trim for Microsoft.NETCore.App. For pure OOBs, I also think there's a reasonable benefit, even if smaller.

[ericstj]

Projects can opt-in today piecemeal by setting ILLinkTrimAssembly = true.

In addition to that libraries can enable assembly diffing to see what it does: https://github.com/dotnet/runtime/blob/220bf9714248cca8ef18cb4175ae83b1cf210a70/eng/illink.targets#L335-L338

We have a public AsmDiff now. It'd be nice to clean that up and make it easier, cc @Anipik

[stephentoub]

Projects can opt-in today piecemeal by setting ILLinkTrimAssembly = true.

Then maybe that's how we start. Ala nullable where we incrementally added #nullable enable and then deleted them all when we were able to flip the whole assembly, we could go assembly by assembly turning it on and validating the diff is expected and things work correctly, and then flip it for the whole repo if/when we get there. We could start with the Microsoft.Extensions.* assemblies and others that ship in the ASP.NET shared framework.

[ericstj]

Yeah, I'd want to provide the tooling for folks to do this safely though. I'd like to get that diffing in place before opening this up for folks to do it blind.

[ghost]

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding See info in area-owners.md if you want to be subscribed.

Issue Details

---

Copying from https://github.com/dotnet/runtime/issues/45665 The suggestion was made to run the ILLinker to trim dead code from all libraries, not just those libraries that make up the shared framework. Here's an excerpt of the discussion. > It was a scoping decision back when we first added it. Folks weren't willing to test the output of things that weren't going in the shared framework (eg: UAP assemblies, packages that would be used by desktop, etc) so we constrained it to only the shared framework. Folks were confident that we had better test coverage of the shared framework since that's what we were shipping at the time. We could broaden that scope but we'd probably want to make sure such a change had decent test coverage + auditing of the new assemblies it touched across all their supported frameworks. This still applies, we don't test all package configurations. I think the linker is much more stable now and we could enable it. I think when doing so we'd want to audit what it does to non-tested configurations. That could be done with Assembly diffing. If the changes the linker made matched those made to the netcoreapp build we could be reasonably confident that the netcoreapp build would remain a good proxy for the downlevel builds. If we noticed any significant differences we could do targeted testing / investigation to ensure we have correct linker configurations for those configurations. This will also enable the linker on completely OOB assemblies where it never ran before (CodeDom, System.Management, MEF, etc) and where we haven't invested in test coverage. We'd want to do some manual diffing of these to validate that removals are safe, potentially adding test coverage to touch impacted codepaths. Do folks think this effort is worth the cost?

Author:	ericstj
Assignees:	-
Labels:	`Discussion`, `area-Infrastructure-libraries`, `linkable-framework`
Milestone:	6.0.0

[eerhardt]

Are we sure we need to address this in 6.0? I see it is in the 6.0 milestone.

I can't see prioritizing this work higher than a "P2" at this point, as it doesn't affect any of the scenarios we are targeting for the trimming effort (WASM and Xamarin).

[eerhardt]

Moving to 'Future' as this isn't high priority to get into 6.0.