Enable featuredefault Substitutions To Only Apply at App Publish Time

[eerhardt]

In #80246, we are adding support for RuntimeFeature.IsDynamicCodeSupported to be a feature switch, instead of being hard-coded to true for CoreCLR. This allows developers to test their code simulating it running with NativeAOT, without actually needing to publish for NativeAOT.

As part of adding this feature switch, I don't want to regress app sizes for existing apps. Since CoreCLR currently hard-codes RuntimeFeature.IsDynamicCodeSupported to true. So when you trim a CoreCLR app with code like the following:

RuntimeFeature.IsDynamicCodeSupported ?
    new ReflectionEmitCachingMemberAccessor() :
    new ReflectionMemberAccessor();

The ReflectionMemberAccessor class can be trimmed, since IsDynamicCodeSupported will always be true.

To do this, I wanted to use a featuredefault option in my ILLink.Substitutions.xml. This would tell the trimmer that if no one specifies the RuntimeFeature.IsDynamicCodeSupported feature switch, assume it is true.

However, since we run the trimmer during the build of System.Private.CoreLib, this featuredefault is getting applied during the "library build" time. Which means the property is getting hard-coded to true at the build time, and the code in the RuntimeFeature.IsDynamicCodeSupported property is getting removed.

We should figure out a way where a library can embed a featuredefault ILLink.Substitutions.xml element, but only have it apply during "app publish" time.

cc @sbomer @vitek-karas

[sbomer]

The --ignore-substitutions option claims to skip reading embedded substitutions:
https://github.com/dotnet/linker/blob/4b3f78cbc7284b4198652a695e9fe0267133728e/src/linker/Linker/Driver.cs#L1367

However, I think that setting also skips reading command-line substitutions:
https://github.com/dotnet/linker/blob/4f0d349e43a71d44ef71339293701fcfc2da997e/src/linker/Linker.Steps/BodySubstitutionParser.cs#L32

If we fix this, I think it would make sense to:

• Set --ignore-substitutions during the corelib build
• Pass any substitutions that are supposed to be baked in as part of the corelib build via --substitutions
• Also include the baked-in substitutions as embedded resources (so that if the constant methods are referenced during application link, they are properly substituted at that time)

[eerhardt]

Set --ignore-substitutions during the corelib build

Could it make sense to flip this around and have an "opt out" list of substitutions on the command line? Basically all the substitutions that happen during the build today of CoreLib should be respected at build time. There are just a handful (at this point - only one) that should be opted out of.

[vitek-karas]

I would argue that you don't need the embedded ones if you have the same on command line - because trimming will bake the value into and it will turn that property into return const - and constant prop should be able to figure that out during app build. But I don't want to depend on that behavior - at least not yet.

I don't have a strong opinion either way. It would be easier to do what @sbomer suggest on the linker side, but would require more work in runtime build. And the other way round.

[eerhardt]

How much work would it be to add a command line option like --ignore-featuredefault <feature name>? What it would mean would be for this run of the linker to not respect the featuredefault setting for that feature in any of the ILLink.*.xml files.

Then everything else stays the same. We don't need to mess with any of the existing unconditional substitutions that are happening (which is actually kind of complicated because of all the ways we build CoreLib - mono/coreclr/nativeaot x OS x architecture).

[marek-safar]

We could also ignore all embedded features switches in 'library mode.

[akoeplinger]

Maybe it's time to revisit the "library build" in runtime? is it still worth it?

[eerhardt]

is it still worth it?

IMO the value is in the following places:

1. In System.Private.CoreLib, the unconditional substitutions are applied to all the code in CoreLib at build time.
2. We use a lot of "shared source" files in CoreLib and other libraries, and the trimmer will eliminate methods in shared source files that aren't used in a specific library.

[ghost]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In #80246, we are adding support for RuntimeFeature.IsDynamicCodeSupported to be a feature switch, instead of being hard-coded to true for CoreCLR. This allows developers to test their code simulating it running with NativeAOT, without actually needing to publish for NativeAOT.

As part of adding this feature switch, I don't want to regress app sizes for existing apps. Since CoreCLR currently hard-codes RuntimeFeature.IsDynamicCodeSupported to true. So when you trim a CoreCLR app with code like the following:

RuntimeFeature.IsDynamicCodeSupported ?
    new ReflectionEmitCachingMemberAccessor() :
    new ReflectionMemberAccessor();

The ReflectionMemberAccessor class can be trimmed, since IsDynamicCodeSupported will always be true.

To do this, I wanted to use a featuredefault option in my ILLink.Substitutions.xml. This would tell the trimmer that if no one specifies the RuntimeFeature.IsDynamicCodeSupported feature switch, assume it is true.

However, since we run the trimmer during the build of System.Private.CoreLib, this featuredefault is getting applied during the "library build" time. Which means the property is getting hard-coded to true at the build time, and the code in the RuntimeFeature.IsDynamicCodeSupported property is getting removed.

We should figure out a way where a library can embed a featuredefault ILLink.Substitutions.xml element, but only have it apply during "app publish" time.

cc @sbomer @vitek-karas

Author:	eerhardt
Assignees:	-
Labels:	untriaged, area-Tools-ILLink, needs-area-label
Milestone:	-

[sbomer]

Per discussion at #103463 (comment), I think a better way to accomplish this is to avoid using featuredefault for substitutions, and to instead place defaults into the MSBuild targets that are applied only at app publish time.

[eerhardt]

That makes sense to me. Not sure why that wasn't considered when I opened this issue. If we can make the MSBuild targets work for #80398, then we can close this issue.