Trimming System.SR

[tannergooding]

Issue

System.SR is currently one of the largest types when looking at the trimmer dependency dump. As of 6.0.100-alpha.1.21057.4 it is reported at 23470 bytes.

System.SR exists of over a thousand small properties that are effectively just internal static string Name => GetResourceString("Name");. Given that each of these methods actually uses a different string constant, they each take up their own space in metadata and each user ends up having a call to the relevant method.

I think there are two feasible fixes we could have here...

Proposal 1

We specially annotate the properties and have the IL Linker inline these at the usage site. Given that every one of these methods is auto-generated today and that they always follow the same format, this should be generally "safe" (and I believe similar to the NonVersionable attribute).

Proposal 2

We rework how resources are resolved so that, rather than going through a property (SR.Name) we do something like SR.GetResourceString(nameof(SRID.Name)) instead.

A local prototype of this (proposal 2) for S.P.Corelib in the default blazor-wasm project template shows a size reduction in System.Private.CoreLib.dll.br of 6.4kb (and a reduction of 26.5kb for the uncompressed S.P.Corelib).

[ghost]

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

Issue Details

---

Issue

System.SR is currently one of the largest types when looking at the trimmer dependency dump. As of 6.0.100-alpha.1.21057.4 it is reported at 23470 bytes.

System.SR exists of over a thousand small properties that are effectively just internal static string Name => GetResourceString("Name");. Given that each of these methods actually uses a different string constant, they each take up their own space in metadata and each user ends up having a call to the relevant method.

I think there are two feasible fixes we could have here...

Proposal 1

We specially annotate the properties and have the IL Linker inline these at the usage site. Given that every one of these methods is auto-generated today and that they always follow the same format, this should be generally "safe" (and I believe similar to the NonVersionable attribute).

Proposal 2

We rework how resources are resolved so that, rather than going through a property (SR.Name) we do something like SR.GetResourceString(nameof(SRID.Name)) instead.

A local prototype of this (proposal 2) for S.P.Corelib in the default blazor-wasm project template shows a size reduction in System.Private.CoreLib.dll.br of 6.4kb (and a reduction of 26.5kb for the uncompressed S.P.Corelib).

Author:	tannergooding
Assignees:	-
Labels:	size-reduction
Milestone:	-

[dotnet-issue-labeler]

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

[tannergooding]

CC. @vitek-karas, @marek-safar, @agocke, @MichalStrehovsky, @eerhardt, @joperezr

[agocke]

We specially annotate the properties and have the IL Linker inline these at the usage site.

I'm not sure I understand what's being inlined here. Is it the property body into the caller?

[tannergooding]

I'm not sure I understand what's being inlined here. Is it the property body into the caller?

Yes. Today, if you decompile S.P.Corelib and look at say InvalidOperationException..ctor(), you will see:

public InvalidOperationException()
    : base(SR.Arg_InvalidOperationException)
{
}

With proposal 1, you would instead see (the linker having inlined SR.Arg_InvalidOperationException due to the proposed special attribute):

public InvalidOperationException()
    : base(SR.GetResourceString("Arg_InvalidOperationException"))
{
}

With proposal 2, we manually change all the code and it becomes something like:

public InvalidOperationException()
    : base(SR.GetResourceString(nameof(SRID.Arg_InvalidOperationException)))
{
}

The last is what I manually did locally for a subset of S.P.Corelib to see the 6.4kb of savings in the compressed S.P.Corelib.br file.
However, the former (proposal 1) would functionally achieve the same thing and would likely be more maintainable for any similar situations that might come up in the future.

I would suggest that it stay internal only and have similar semantics to NonVersionable in Crossgen.

[danmoseley]

Most of our libraries don't call GetResourceString(string..) directly. It might be nice if those libraries could specify (simple define) that it should be private, so their code always goes through the strongly typed mechanism. These proposals might make that difficult because it needs to be exposed beyond the SR class. I wonder whether we can avoid that problem?

[jkotas]

A very related problem to this one is outlining of the exception throwing into throw helpers. Ideally, we would have all exception throwing goo outlined into a helper methods that enables better code quality and code sharing. We do that manually today in CoreLib (https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs) and to lesser degree in other libraries too. The manually written ThrowHelpers are bug prone and not linker friendly (the linker is not able to trim unused strings and paths from the manually written ThrowHelpers).

Can we solve both of these problems together?

[agocke]

I think we're blurring the lines between the linker and crossgen here. It's potentially a good reason to merge the two tools in the future

[MichalStrehovsky]

This wouldn't have to be restricted to the SR class - if illink can prove the method/property is not a target of reflection, there's no side effects, and inlining would be profitable, illink could inline methods in general (and slap an IgnoreAccessChecks attribute on the assembly to shut up warnings about violating visibility/accessibility checks).

With the annotations we added in .NET 5 illink has a pretty good picture of what is visible from reflection.

As a cheaper alternative, we could consider illink stripping the properties on the SR class and leaving just the methods. We'll probably get close to half of the expected benefit of inlining with that and such change is pretty cheap (we get rid of the entry in the Property table, the extra name without the get_ prefix, an entry in the MethodSemantics table).

Properties are only interesting for reflection and the runtime doesn't care about them. Illink could strip properties on all types where it knows the type is not a target of a GetProperty(ies) call.

[marek-safar]

I'm wondering if we could use source generators for this during compilation time and do less or no special work in IL linker. We need to get rid of code like this https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs#L524-L716 anyway and why to rewrite the code if we could generate it as size optimal from the start.

There is also the aspect of and resources and the resources accessors duplication between all runtime assemblies which has a size impact too.

[stephentoub]

Ideally, we would have all exception throwing goo outlined into a helper methods that enables better code quality and code sharing. We do that manually today in CoreLib (https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/ThrowHelper.cs) and to lesser degree in other libraries too

Related, note that with dotnet/csharplang#2145 / https://github.com/dotnet/csharplang/blob/master/proposals/null-arg-checking.md, which is on track for C# 10, for at least argument null validation the C# compiler will generate code like that used for ThrowHelper.
cc: @jaredpar
It's only partially related, though, as there aren't custom messages for those.

[jkotas]

I'm wondering if we could use source generators for this during compilation time and do less or no special work in IL linker. We need to get rid of code like this

How would you make it linker friendly without losing compactness of the current manually written ThrowHelper?

[marek-safar]

How would you make it linker friendly without losing compactness of the current manually written ThrowHelper?

I was thinking about something like this (not sure if it's possible with source generators).

class SomeType
{
    public void Method(int startIndex, int length)
    {
        if (length < 0)
            throw new ArgumentOutOfRangeException(nameof(length), "Length cannot be less than zero.");

        if (startIndex < 0)
            throw new ArgumentOutOfRangeException(nameof(startIndex), "StartIndex cannot be less than zero.");
    }

    // Compiler rewritten/generated version of same method
    public void Method(int startIndex, int length)
    {
        if (length < 0)
            Global.Throw1 ();

        if (startIndex < 0)
            Global.Throw2 ();
    }
}

static class Global
{
	// It will be reused when the data match
    public static void Throw1 ()
    {
        throw new ArgumentOutOfRangeException("length", "Length cannot be less than zero.");
    }

    public static void Throw2 ()
    {
        throw new ArgumentOutOfRangeException("startIndex", "StartIndex cannot be less than zero.");
    }
}

[tannergooding]

Source generators don't currently allow you to replace the contents of a method, only to define new code.

[eerhardt]

@vitek-karas @marek-safar - is this something we should do for 6.0? Or can it be moved to 'Future'?

[SamMonoRT]

Moving to 7.0.0