Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: MakeWeakTypeReference as trimming indicator #74307

Open
hez2010 opened this issue Aug 21, 2022 · 25 comments
Open

[API Proposal]: MakeWeakTypeReference as trimming indicator #74307

hez2010 opened this issue Aug 21, 2022 · 25 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-AssemblyLoader-coreclr linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@hez2010
Copy link
Contributor

hez2010 commented Aug 21, 2022

Background and motivation

While we were working with trimming, we found that we often want to express "code only needed if a type is preserved", but currently we do not have such thing in C#.

For example, in XAML frameworks, we use URLs to locate resources. With source generator, the code of the resource locator which is generated by a source generator may consist with a method with plenty of if statements, for example:

// auto-generated by a source generator
object? LoadResource(string path)
{
    if (path == "ms-appx://MyAssembly/My/Fancy/XamlControl1Style") return XamlLoader.Load<XamlControl1Style>();
    if (path == "ms-appx://MyAssembly/Another/XamlControl2Style") return XamlLoader.Load<XamlControl2Style>();
    return null;
}

With this source generator approach, we can achieve trimming compatibility. However, this would result in all types referenced in LoadResource being preserved even they are not being actually used. In this case, we only want XamlControl1Style to be preserved if XamlControl1 is referenced in other pieces of code.

API Proposal

namespace System.Diagnostics.CodeAnalysis;

public class ControlFlow
{
    public static bool MakeWeakTypeReference<T>() => true;
}

API Usage

object? LoadResource(string path)
{
    if (ControlFlow.MakeWeakTypeReference<XamlControl1>())
        if (path == "ms-appx://MyAssembly/My/Fancy/XamlControl1Style")
            return XamlLoader.Load<XamlControl1Style>();
    if (ControlFlow.MakeWeakTypeReference<XamlControl2>())
        if (path == "ms-appx://MyAssembly/Another/XamlControl2Style")
            return XamlLoader.Load<XamlControl2Style>();
    return null;
}

It behaves as a no-op which always returns true, but if there are no other type references to XamlControl1, the whole block of the if statement can be safely trimmed, which produces code after trimming like below:

object? LoadResource(string path)
{
    if (path == "ms-appx://MyAssembly/Another/XamlControl2Style")
        return XamlLoader.Load<XamlControl2Style>();
    return null;
}

Therefore, all types related to XamlControl1 and XamlControl1Type can also be trimmed away.

This should also support and and or so that we can express multiple types as a single prerequisite:

if (ControlFlow.MakeWeakTypeReference<Foo>() && ControlFlow.MakeWeakTypeReference<Bar>())
{
    // only preserved if both Foo and Bar are referenced
}

if (ControlFlow.MakeWeakTypeReference<Foo>() || ControlFlow.MakeWeakTypeReference<Bar>())
{
    // only preserved if either Foo or Bar is referenced
}

// Bogus
if (!ControlFlow.MakeWeakTypeReference<Foo>())
{
    // no-op, can never reach because `!ControlFlow.MakeWeakTypeReference<Foo>()` is always false, maybe a warning should be emitted by the compiler or illink?
}

Alternative Designs

No response

Risks

No response

@hez2010 hez2010 added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Aug 21, 2022
@dotnet-issue-labeler
Copy link

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.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 21, 2022
@teo-tsirpanis teo-tsirpanis added the linkable-framework Issues associated with delivering a linker friendly framework label Aug 21, 2022
@ghost
Copy link

ghost commented Aug 21, 2022

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

Issue Details

Background and motivation

While we were working with trimming, we found that we often want to express "code only needed if a type is preserved", but currently we do not have such thing in C#.

For example, in XAML frameworks, we use URLs to locate resources. With source generator, the code of the resource locator which is generated by a source generator may consist with a method with plenty of if statements, for example:

// auto-generated by a source generator
object? LoadResource(string path)
{
    if (path == "ms-appx://MyAssembly/My/Fancy/XamlControl1Style") return XamlLoader.Load<XamlControl1Style>();
    if (path == "ms-appx://MyAssembly/Another/XamlControl2Style") return XamlLoader.Load<XamlControl2Style>();
    return null;
}

With this source generator approach, we can achieve trimming compatibility. However, this would result in all types referenced in LoadResource being preserved even they are not being actually used. In this case, we only want XamlControl1Style to be preserved if XamlControl1 is referenced in other pieces of code.

API Proposal

namespace System.Diagnostics.CodeAnalysis;

public class ControlFlow
{
    public static bool MakeWeakTypeReference<T>() => true;
}

API Usage

object? LoadResource(string path)
{
    if (ControlFlow.MakeWeakTypeReference<XamlControl1>())
        if (path == "ms-appx://MyAssembly/My/Fancy/XamlControl1Style")
            return XamlLoader.Load<XamlControl1Style>();
    if (ControlFlow.MakeWeakTypeReference<XamlControl2>())
        if (path == "ms-appx://MyAssembly/Another/XamlControl2Style")
            return XamlLoader.Load<XamlControl2Style>();
    return null;
}

It behaves as a no-op which always returns true, but if there are no other type references to XamlControl1, the whole block of the if statement can be safely trimmed, which produces code after trimming like below:

object? LoadResource(string path)
{
    if (ControlFlow.MakeWeakTypeReference<XamlControl2>())
        if (path == "ms-appx://MyAssembly/Another/XamlControl2Style")
            return XamlLoader.Load<XamlControl2Style>();
    return null;
}

Therefore, all types related to XamlControl1 and XamlControl1Type can also be trimmed away.

This should also support and and or so that we can express multiple types as a single prerequisite:

if (ControlFlow.MakeWeakTypeReference<Foo>() && ControlFlow.MakeWeakTypeReference<Bar>())
{
    // only preserved if both Foo and Bar are referenced
}

if (ControlFlow.MakeWeakTypeReference<Foo>() || ControlFlow.MakeWeakTypeReference<Bar>())
{
    // only preserved if either Foo or Bar is referenced
}

// Bogus
if (!ControlFlow.MakeWeakTypeReference<Foo>())
{
    // no-op, can never reach because `!ControlFlow.MakeWeakTypeReference<Foo>()` is always false, maybe a warning should be emitted by the compiler or illink?
}

Alternative Designs

No response

Risks

No response

Author: hez2010
Assignees: -
Labels:

api-suggestion, untriaged, linkable-framework

Milestone: -

@teo-tsirpanis
Copy link
Contributor

Interesting idea; the name does not sound right to me though.

@hez2010
Copy link
Contributor Author

hez2010 commented Aug 21, 2022

Interesting idea; the name does not sound right to me though.

Yes. I did think about the name for a while but didn't conclude a fancy name :)

@MichalPetryka
Copy link
Contributor

Maybe IsTypePresent<T>?

@teo-tsirpanis
Copy link
Contributor

That's better, and with that name it fits more on Type.

@eerhardt
Copy link
Member

See the following code, which we use in System.Text.Json to do this exact scenario:

// This method takes an unannotated string which makes linker reflection analysis lose track of the type we are
// looking for. This indirection allows the removal of the type if it is not used in the calling application.
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2057:TypeGetType",
Justification = "This method exists to allow for 'weak references' to the Stack and Queue types. If those types are used in the app, " +
"they will be preserved by the app and Type.GetType will return them. If those types are not used in the app, we don't want to preserve them here.")]
private static Type? GetTypeIfExists(string name) => Type.GetType(name, false);

@ghost
Copy link

ghost commented Aug 22, 2022

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

Issue Details

Background and motivation

While we were working with trimming, we found that we often want to express "code only needed if a type is preserved", but currently we do not have such thing in C#.

For example, in XAML frameworks, we use URLs to locate resources. With source generator, the code of the resource locator which is generated by a source generator may consist with a method with plenty of if statements, for example:

// auto-generated by a source generator
object? LoadResource(string path)
{
    if (path == "ms-appx://MyAssembly/My/Fancy/XamlControl1Style") return XamlLoader.Load<XamlControl1Style>();
    if (path == "ms-appx://MyAssembly/Another/XamlControl2Style") return XamlLoader.Load<XamlControl2Style>();
    return null;
}

With this source generator approach, we can achieve trimming compatibility. However, this would result in all types referenced in LoadResource being preserved even they are not being actually used. In this case, we only want XamlControl1Style to be preserved if XamlControl1 is referenced in other pieces of code.

API Proposal

namespace System.Diagnostics.CodeAnalysis;

public class ControlFlow
{
    public static bool MakeWeakTypeReference<T>() => true;
}

API Usage

object? LoadResource(string path)
{
    if (ControlFlow.MakeWeakTypeReference<XamlControl1>())
        if (path == "ms-appx://MyAssembly/My/Fancy/XamlControl1Style")
            return XamlLoader.Load<XamlControl1Style>();
    if (ControlFlow.MakeWeakTypeReference<XamlControl2>())
        if (path == "ms-appx://MyAssembly/Another/XamlControl2Style")
            return XamlLoader.Load<XamlControl2Style>();
    return null;
}

It behaves as a no-op which always returns true, but if there are no other type references to XamlControl1, the whole block of the if statement can be safely trimmed, which produces code after trimming like below:

object? LoadResource(string path)
{
    if (path == "ms-appx://MyAssembly/Another/XamlControl2Style")
        return XamlLoader.Load<XamlControl2Style>();
    return null;
}

Therefore, all types related to XamlControl1 and XamlControl1Type can also be trimmed away.

This should also support and and or so that we can express multiple types as a single prerequisite:

if (ControlFlow.MakeWeakTypeReference<Foo>() && ControlFlow.MakeWeakTypeReference<Bar>())
{
    // only preserved if both Foo and Bar are referenced
}

if (ControlFlow.MakeWeakTypeReference<Foo>() || ControlFlow.MakeWeakTypeReference<Bar>())
{
    // only preserved if either Foo or Bar is referenced
}

// Bogus
if (!ControlFlow.MakeWeakTypeReference<Foo>())
{
    // no-op, can never reach because `!ControlFlow.MakeWeakTypeReference<Foo>()` is always false, maybe a warning should be emitted by the compiler or illink?
}

Alternative Designs

No response

Risks

No response

Author: hez2010
Assignees: -
Labels:

api-suggestion, area-AssemblyLoader-coreclr, untriaged, linkable-framework

Milestone: -

@jkoritzinsky
Copy link
Member

@eerhardt that's really clever and I think a good example for where an API like this could be useful. I think being able to have a well-known API that would enable this to work while using generics or strongly-typed System.Type objects instead of having to round-trip through type name parsing would be quite useful and likely more maintainable than the existing mechanism you shared.

@MichalPetryka
Copy link
Contributor

@eerhardt that's really clever and I think a good example for where an API like this could be useful. I think being able to have a well-known API that would enable this to work while using generics or strongly-typed System.Type objects instead of having to round-trip through type name parsing would be quite useful and likely more maintainable than the existing mechanism you shared.

Additionally, it'd be more AOT safe.

@teo-tsirpanis
Copy link
Contributor

namespace System;

public abstract partial class Type
{
    public static Type? GetTypeIfExists<T>() => typeof(T); // would be replaced with null by the trimmer if the type does not exist.
}

@kekekeks
Copy link

kekekeks commented Aug 23, 2022

I believe that just getting the Type instance conditionally isn't enough for XAML styles. Not sure how it works in the case of the code generator mentioned in the issue, but with Avalonia XAML Styles will usually reference the properties of the type itself from MSIL.

e. g. we have something like this (equivalent C# translation, the actual XAML compiler generates msil):

Application.Resources[typeof(Button)] = new ControlTheme(typeof(Button))
{
  new Setter(Button.TemplateProperty, new FuncControlTemplate<Button>(() => new ContentPresenter
  {
      [ContentPresenter.ContentProperty] = new TemplateBinding(Button.ContentProperty),
      [ContentPresenter.PaddingProperty] = new TemplateBinding(Button.PaddingProperty)
  }));,
  new Setter(Button.PaddingProperty, new Padding(4)),
  ...
}

During linking if Button class isn't used, it's needed to trim the entire code that constructs the Button's theme, because everything is strongly typed and liker we'll see references from MSIL, so the code would have to look like this:

if(Type.Exists<Button>())
{
  Application.Resources[typeof(Button)] = new ControlTheme(typeof(Button))
  {
    new Setter(Button.TemplateProperty, new FuncControlTemplate<Button>(() => new ContentPresenter
    {
        [ContentPresenter.ContentProperty] = new TemplateBinding(Button.ContentProperty),
        [ContentPresenter.PaddingProperty] = new TemplateBinding(Button.PaddingProperty)
    }));,
    new Setter(Button.PaddingProperty, new Padding(4)),
    ...
  };
}

with if body trimmed away if Button class isn't referenced elsewhere.

@MichalPetryka
Copy link
Contributor

Working on Type would also be less AOT and linker friendly since for example the AOT wouldn't be able to MakeGenericType with types provided that way.

@hez2010
Copy link
Contributor Author

hez2010 commented Aug 23, 2022

During linking if Button class isn't used, it's needed to trim the entire code that constructs the Button's theme, because everything is strongly typed and liker we'll see references from MSIL, so the code would have to look like this:

if(Type.Exists<Button>())
{
  Application.Resources[typeof(Button)] = new ControlTheme(typeof(Button))
  {
    new Setter(Button.TemplateProperty, new FuncControlTemplate<Button>(() => new ContentPresenter
    {
        [ContentPresenter.ContentProperty] = new TemplateBinding(Button.ContentProperty),
        [ContentPresenter.PaddingProperty] = new TemplateBinding(Button.PaddingProperty)
    }));,
    new Setter(Button.PaddingProperty, new Padding(4)),
    ...
  };
}

with if body trimmed away if Button class isn't referenced elsewhere.

I think if (Type.GetTypeIfExists<Button>() != null) is equivalent to if (Type.Exists<Button>()).

@vitek-karas
Copy link
Member

This is another manifestation of a problem which we've ran into couple of times already but so far didn't really tackle. Basically source generators have a hard time attaching the generated code to a "target" type. The usual recommendation is to make the target type partial and thus let the source generator adds stuff to it. But that's not always possible, for example in the above case of Button class, there's no good way to attach anything to it from the app's code.

So people solve this by introducing some types of registration mechanism. System.Text.Json uses the "context" for example. If such registration is global, it naturally leads to rooting everything in the registry regardless if it's needed by the app or not.

There was a similar discussion on the topic in this issue #50333. The original take is somewhat different, but it discusses basically the same problem space.

I do like the approach proposed in this issue which uses "code", instead of declarative attributes or similar solutions. But as proposed this goes against one of the main design principles of trimming:

Trimming an app should not change any observable behavior of the code

This proposal would explicitly add a public API which changes behavior when trimmed. As used above it's OK, but one could easily use it to change other types of behavior and thus introduce unpredictability into the development process (debug build should behave the same as published app).

Trimmer already has a feature which goes into this area:

void Test(object o)
{
    if (o is SpecialType)
        Console.WriteLine("Found something special");
}

If this is the only reference to SpecialType in the app, trimmer will actually remove SpecialType and rewrite the condition to if (false). But it keeps the if branch around, along with all of its dependencies. (see tests).

If we extended this to actually remove the if branch it could be used to provide a solution the problem stated in this issue. The advantage of such solution would be that there's no observable difference when the app is trimmed. If the SpecialType is not used anywhere in the app, the if branch would never execute anyway (trimming or not), so it's effectively dead code and can be removed.

It would require a slight design change in the above samples, but it feels possible. I can also imagine supporting the Type version of this:

void Test(Type type)
{
    if (type == typeof(SpecialType))
        Console.WriteLine(...);
}

Although this might need more thinking if it's truly correct.

Note that the "instance" version of this check is actually slightly better: If the SpecialType is used elsewhere, but it's never instantiated the condition can still be optimized away (as it will always be false). This is not true for the "type" variant.

@MichalStrehovsky
Copy link
Member

I agree with Vitek that is this proposing an API addition that goes against how we want trimming to work - the trimmed app should behave the same as untrimmed app by default. Whenever there's a behavior difference, there should be a warning.

We would need to immediately tag this API as RequiresUnreferencedCode so that a warning is triggered. Suppressing trimming warnings is a can of worms and we very much recommend against suppressing any warnings. We were not able to get warning suppressions right in this very repo with all the expertise we have. I have zero confidence in third parties to get their suppressions right.

Besides the philosophical problem above, the problem I have with an API like this is that it relies on branch removal. Branch removal does happen both in IL Linker and Native AOT. It is also done with different algorithms, and I would like to see the NativeAOT one to be replaced with whatever constprop RyuJIT can do, which is yet another algorithm.

We didn't document branch removal rules on purpose. We only test them on the runtime repo. We had to adjust code in the runtime repo to fit the envelope of the optimization in the past. This would make relying on the shape of the optimization part of a public API surface (the API by itself has no legitimate use outside of this pattern). There would be no feedback if constprop is not able to kick in and doesn't eliminate a branch that the user expects. This is hard to debug for end users.

The other thing is that "a type is preserved" is a very murky concept:

class Program
{
    static void Do(SomeClass x) { }

    static void Main()
    {
         // Is SomeClass preserved? Trimming at IL Level would need to preserve it
        // because it’s referenced from the method signature. But it doesn’t exist for NativeAOT purposes
        // except in the PDB.
        Do(null);

        // Is SomeClass preserved now?
        // Assume SomeSimpleStaticMethod was simple enough that IL Linker inlined it.
        SomeClass.SomeSimpleStaticMethod();

        // How about now?
        // SomeClass would be reflection visible now when IL trimming. NativeAOT still has leeway.
        SomeClass.OtherSimpleStaticMethod();
    }
}

This API is basically trying to query at runtime for the result of a compile time optimization that is supposed to be transparent.


I would really like to think more in directions that can have the same behavior with/without trimming.

For the XAML example, could the Button class in its constructor do a callout that registers the styles? I assume we don't need styles for a button if no button was created yet.

@kekekeks
Copy link

The problem with XAML styles is that they are completely separate from the control itself. Control does not and should not know anything about its styles, they are essentially user-defined and can be switched to a separate set of styles at runtime during the theme switch.
Just like in WPF, in Avalonia styles are defined in a separate assembly (in case of Avalonia they come in separate nuget packages) and can be replaced for the entire app or a particular control subtree with another set of styles that come from a 3rd party package.

@kekekeks
Copy link

Another use case:

My COM interop bindings generate COM interfaces as normal .NET interfaces.

e. g.

[uuid(ED7F3339-8EB4-4912-9D6F-EC4CDD094164})]
interface IFoo : IUnknown
{
	void Bar();
}

becomes

// IUnknown is defined as a marker interface on top of IDisposable
interface IFoo : IUnknown
{
    void Bar();
}

Interop is handled by separate generated classes __MicroComIFooProxy (implements the managed interface and forwards calls to native function pointers) and __MicroComIFooVTable (builds a CCW vtable).

Both are registered by the module initializer. I'd like to be able to trim unneeded interop types when the interface isn't used.

@jkotas
Copy link
Member

jkotas commented Aug 26, 2022

Interop is handled by separate generated classes __MicroComIFooProxy (implements the managed interface and forwards calls to native function pointers) and __MicroComIFooVTable (builds a CCW vtable).

CsWinRT solved this problem by linking the interop helper types via custom attributes: microsoft/CsWinRT#1224

@kekekeks
Copy link

I believe such custom attributes require reflection access. MicroCom aims to work with IlcDisableReflection=True

@teo-tsirpanis
Copy link
Contributor

No, the attributes would be accessed at the time of the source generation, not at runtime.

@kekekeks
Copy link

kekekeks commented Aug 26, 2022

type.GetCustomAttribute looks like run-time reflection to me: https://github.com/microsoft/CsWinRT/pull/1224/files#diff-2c2d5b5c93b2d4df6db3e983e3ba3126c9de5efa3648d9987c499904a3ea99b2R26-R54
This code doesn't seem like it would run with IlcDisableReflection too

                var helper = $"ABI.{fullTypeName}";
                return type.Assembly.GetType(helper) ?? Type.GetType(helper);

@jkotas
Copy link
Member

jkotas commented Aug 27, 2022

type.GetCustomAttribute looks like run-time reflection

Yes, custom attributes are accessed via reflection. The key advantage is that the helper type associations via custom attribute can be statically analyzed for correctness and the solution does not have concerns shared by @vitek-karas and @MichalStrehovsky above.

var helper = $"ABI.{fullTypeName}";
return type.Assembly.GetType(helper) ?? Type.GetType(helper);

This is temporary fallback path that should go away eventually.

@MichalStrehovsky
Copy link
Member

I believe such custom attributes require reflection access. MicroCom aims to work with IlcDisableReflection=True

Working with IlcDisableReflection is a noble goal but I don't think the base class libraries will ever be in a position where they work with IlcDisableReflection=true. They likely won't even work in the more usable future version of it: #67193 (comment). Trimmable reflection is not a problem and would be a good solution for this.

IlcDisableReflection is a tech demo that was introduced before we had things like IlcTrimMetadata=true (which is now the default). With metadata trimming, I don't believe you'll see meaningful size differences with/without reflection for apps that are bigger than a console hello world. IlcDisableReflection produces meaningful savings for hello-world-sized apps because it allows removing the whole reflection stack, but that's a fixed ~1MB cost that is not worth it for most. We'll likely do work that will indirectly make this cost smaller over time. The reflection stack in NativeAOT is a bit overengineered.

If you still see a meaningful difference for Avalonia-sized apps, please add <ItemGroup><IlcArg Include="--make-repro-path:c:\some\path\where\a\zip\can\be\created" /></ItemGroup> to the project and share the resulting ZIP with me (in a new issue) - it's likely a bug - IlcDisableReflection has been useful in finding those in the past.

@vitek-karas
Copy link
Member

We didn't document branch removal rules on purpose. We only test them on the runtime repo. We had to #40539 in the runtime repo to fit the envelope of the optimization in the past. This would make relying on the shape of the optimization part of a public API surface (the API by itself has no legitimate use outside of this pattern). There would be no feedback if constprop is not able to kick in and doesn't eliminate a branch that the user expects. This is hard to debug for end users.

I view this as two separate things:

  • Correctness/consistency - the app behaves the same with/without trimming. The idea of branch removal doesn't violate this in any way. (unlike the proposed API which does)
  • Size optimization (trimming) - currently we don't describe nor make any promises with regard to trimming specific pieces of the app. The tools (trimmer, NativeAOT, ...) may decide to keep code/metadata around without any specific documented reason. This leads to us not making any size guarantees. The branch removal proposed above doesn't work today, but if it enabled it, it would simple improve size for some apps, but should have no other impact.

That said, it would be nice to have some agreed upon way especially for libraries to verify that they can be successfully trimmed to a certain degree - and to enable detection or regressions and such. But that's sort of outside the scope of this issue.

@vitek-karas vitek-karas removed the untriaged New issue has not been triaged by the area owner label Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-AssemblyLoader-coreclr linkable-framework Issues associated with delivering a linker friendly framework
Projects
Status: No status
Development

No branches or pull requests

10 participants