Runtime: Link-time framework feature removal

Created on 25 Jun 2018 · 13Comments · Source: dotnet/runtime

Rationale

For various (often historical) reasons, many of the existing .NET APIs (ours or 3rd party) are unfriendly to tree shaking. This prevents big chunks of the library and library dependencies from being removed by IL linkers at the time of publishing the .NET app.

This issue addresses the API part of dotnet/designs#42. See the spec over there for full rationale. It provides a way for library authors to annotate methods that are responsible for supporting a feature with a potentially large set of dependencies.

Proposal

namespace System.Runtime.CompilerServices
{
    // Instructs IL linkers that the method body decorated with this attribute can be removed.
    // The return value of the method with removed body is replaced with the default value
    // that corresponds to the type (0 for int, null for reference types, etc.).
    // If FailsFast = true, the method body is replaced with a FailFast.
    [AttributeUsage(AttributeTargets.Method)]
    public sealed class RemovableAttribute : Attribute
    {
        // Name of the feature the removal is conditioned on. The name follows the guidelines
        // for choosing AppContext switch names.
        public string FeatureSwitchName { get; }

        // Indicates whether the method body should be replaced by a FailFast
        public bool FailsFast { get; set; }

        public RemovableAttribute(string featureSwitchName);
    }
}

Remarks

Tools would be required to match the attribute by name so it doesn't matter much in what contract we place this. Libraries targeting downlevel frameworks can use this by source-including the attribute definition.

For NetCoreApp 3.0 we can just forward to System.Private.CoreLib (we'll want these in S.P.CoreLib because there's a lot of junk there we should annotate and make treeshakeable).

I have a couple of samples where having this kind of annotation could save more than 2 MB in the x64 version of the UWP People app compiled with .NET Native. These are places where we _regressed_ compared to the version of .NET Native that the People app is shipping with right now. Most of the regression is a direct effect of NetStandard 2.0.

Making the non-file protocol support optional in XmlDocument saves 1.2 MB
Making the reflection fallback in DataContractSerializers optional saves 335 kB.
Making the reflection fallback in XmlSerializers optional saves 410 kB.

api-needs-work area-System.Runtime

Source

MichalStrehovsky

Most helpful comment

It looks like FailsFast is new. While I support the general idea, failfast is somewhat difficult to debug and does things like kill unit test failure reporting (in CoreFX, that means we'd likely lose the issue). We also don't use failfasts for any other user-facing features. I'd suggest a throwing option instead (and including the exception type in this proposal).

morganbr on 26 Jun 2018

👍3

All 13 comments

morganbr on 26 Jun 2018

👍3

This attribute would be placed in a separate contract (System.Runtime.Linking)

I do not think a separate contract for this is useful. Maintainers of (popular) NuGet libraries are very reluctant to depend on external packages like this. They will have a strong preference for including a source version instead. Also, I do not think we want to have yet another little .dll with <10 attributes in it.

jkotas on 26 Jun 2018

RemovableAttribute is a pretty vague name, maybe RemovableBodyAttribute.?

danmosemsft on 26 Jun 2018

It looks like FailsFast is new. While I support the general idea, failfast is somewhat difficult to debug and does things like kill unit test failure reporting (in CoreFX, that means we'd likely lose the issue).

The thing I'm struggling with is code that is never supposed to execute. If you look at the linked commits where this attribute is applied in various places of the framework (the serialization ones) - in those, the removed code is not expected to run. If it runs, it's a bug: a bug that we only hit at publish time. Forget about UWP for a second - UWP is such a minor scenario that it doesn't matter. This would be a bug you don't hit when you dotnet run, only when you dotnet publish. In my head, the severity of such bug is higher than other bugs. I really want to make sure it gets noticed and won't be swallowed by a catchall.

The problem with throwing an exception is twofold:

They are catchable. I think there was one place in the commits where we would in fact catch it, wrap it, and rethrow.
If we want to throw a new exception type, we need the source file defining the new attribute (for source distribution) to also include a definition of the exception. If we ever include the exception type in the framework (and have the tooling throw that), it's going to be a breaking change for people who were catching it from the source definition (no matter how much we ask people not to catch CodeRemovedException, they're going to). We also need to define behaviors when someone includes and uses the attribute, but forgets to include the exception type.

Some of these concerns could be mitigated by

having CoreCLR itself respect the attribute
throwing an existing exception type (NotSupportedException) with an unlocalizable exception message

But none of this is ideal.

I do not think a separate contract for this is useful. Maintainers of (popular) NuGet libraries are very reluctant to depend on external packages like this. They will have a strong preference for including a source version instead. Also, I do not think we want to have yet another little .dll with <10 attributes in it.

We can make it part of the next System.Runtime then, and use source distribution model for people who want to still target downlevel, but define removable features?

MichalStrehovsky on 26 Jun 2018

Having CoreCLR respect the attribute sounds like a very good idea. It makes testing and debugging easier and provides reproducibility. I'd also hope it's not particularly hard to implement.

I do like the idea of making the exception noisy if possible (Add a Managed Debugging Assistant? Make VS default to breaking on it? Call Debugger.Break()?), but I'm not sure that catching it is so much worse than catching any other exception. I think it also aligns with what we're planning to do with framework types.

I think not throwing because of source distribution loses important value for a minor case. For source, it would be fine to either use an existing exception or don't include the property.

morganbr on 27 Jun 2018

I believe that FailsFast if we have it should be a bool not a string.

davidwrighton on 27 Jun 2018

👍1

I believe that FailsFast if we have it should be a bool not a string.

Copy-paste. Fixed.

MichalStrehovsky on 27 Jun 2018

Good idea for this attribute to be sealed, as per official guidelines?

✓ DO seal custom attribute classes, if possible.

Joe4evr on 1 Jul 2018

Good idea for this attribute to be sealed, as per official guidelines?

Thanks for spotting! Fixed.

MichalStrehovsky on 2 Jul 2018

This attribute would be placed in a separate contract (System.Runtime.Linking?) so that we can also support it on downlevel targets.

No. We do not want to increase the set of assemblies, purely for being able to enable features on down-level platforms. That's because we have to make these assemblies type forward for eternity and thus penalize the future by enabling the past. We found this to be untenable (number of moving pieces, performance during build, etc). This API should go into an existing assembly.

If down-level is important, we should ensure the linker looks it up by name and doesn't require unification so that customers can define it in source if they have to. However, we've been told to avoid investing in these things and instead require the consume to upgrade if they want newer features. In the end, if the platform itself ships often enough and allows side by side install, less people will be stuck on down-level platforms.

I haven't read the spec yet, but will do and leave comments here.