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

Expand ILLink/ILC support for 'eliminate dead branches around typeof comparisons' to work on internal types too #110300

Open
Sergio0694 opened this issue Dec 2, 2024 · 8 comments
Labels
area-NativeAOT-coreclr untriaged New issue has not been triaged by the area owner

Comments

@Sergio0694
Copy link
Contributor

Sergio0694 commented Dec 2, 2024

Overview

This is a follow up to #102248 which primarily affects CsWinRT. In order to support marshalling types we don't own (meaning we cannot attach a vtable to them via an attribute), the AOT generators in CsWinRT also generate a global lookup table with all necessary vtables for types it has seen as possibly used across the ABI boundary, in a given project. This looks something like this:

internal static class GlobalVtableLookup
{
    [ModuleInitializer]
    internal static void InitializeGlobalVtableLookup()
    {
    	ComWrappersSupport.RegisterTypeComInterfaceEntriesLookup(LookupVtableEntries);
    	ComWrappersSupport.RegisterTypeRuntimeClassNameLookup(LookupRuntimeClassName);
    }
    
    private static ComWrappers.ComInterfaceEntry[] LookupVtableEntries(System.Type type)
    {
    	switch (type.ToString())
    	{
    	case "System.Collections.Generic.Dictionary`2[System.String,System.String]":
    	case "System.Collections.ObjectModel.ReadOnlyDictionary`2[System.String,System.String]":
                // Bunch of initialization...
                return TheVTableForThisType();
    	case "System.ComponentModel.DataAnnotations.ValidationResult[]":
                // Bunch of initialization...
                return TheVTableForThisType();
    	case "ABI.System.Collections.Generic.ToAbiEnumeratorAdapter`1[System.Collections.IList]":
                // Bunch of initialization...
                return TheVTableForThisType();
    	// ...
    	default:
               return null;
    	}
    }
    
    // 'LookupRuntimeClassName' here, which is kinda similar but returns type names instead
}

This works just fine, but we noticed the linker isn't able to remove all branches for types that are not constructed.

Repro steps

  1. Create a blank .NET 9 project like this:
<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0-windows10.0.17763.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishAot>true</PublishAot>
    <InvariantGlobalization>true</InvariantGlobalization>
    <CsWinRTAotWarningLevel>2</CsWinRTAotWarningLevel>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <IlcGenerateMstatFile>true</IlcGenerateMstatFile>
    <IlcGenerateDgmlFile>true</IlcGenerateDgmlFile>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="CommunityToolkit.Mvvm" Version="8.4.0-preview3" />
  </ItemGroup>
</Project>
  1. Paste this code:
using CommunityToolkit.Mvvm.ComponentModel;

Console.WriteLine(new MyViewModel());

public sealed partial class MyViewModel : ObservableObject
{
}
  1. Publish with dotnet publish -r win-x64 .\ConsoleApp13.csproj

Here's what we see in sizoscope:

Image

That is, sizoscope isn't able to trim branches for types not constructed when checked via the type name.

Note

We cannot do GetType() == typeof() checks, because that would not work with internal types (which we also need to handle).

Proposal

We need a generalized way for this to work in such a way that we can also leverage this for internal types. For instance, either:

  • Making ILLink/ILC also trim branches just comparing the fully qualified type name
  • Making ILLink/ILC support this via some magic intrinsic (eg. GetType() == Type.GetType("The.Type.Name")
  • Something else..?

To clarify, the issue is internal types from other assemblies, ie. such that we can't just do typeof(TheType) in code.

cc. @MichalStrehovsky

@Sergio0694 Sergio0694 added area-NativeAOT-coreclr area-Tools-ILLink .NET linker development as well as trimming analyzers labels Dec 2, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Dec 2, 2024
Copy link
Contributor

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

Copy link
Contributor

Tagging subscribers to this area: @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

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

Copy link
Contributor

Tagging subscribers to this area: @dotnet/illink
See info in area-owners.md if you want to be subscribed.

@Sergio0694 Sergio0694 removed the area-Tools-ILLink .NET linker development as well as trimming analyzers label Dec 2, 2024
@MichalStrehovsky
Copy link
Member

Making ILLink/ILC also trim branches just comparing the fully qualified type name

Since anyone can have their own System.Type descendants that may return anything as their ToString or FullName, we'd need guarantees that this is the real deal (RuntimeType). So it would have to be something like someInstance.GetType().FullName (it cannot be a System.Type that we obtained from who-knows-where). Would such restriction work?

Making ILLink/ILC support this via some magic intrinsic (eg. GetType() == Type.GetType("The.Type.Name")

We'd probably not want to introduce patterns that only perform well when rewritten. I wonder if it would be feasible to introduce an UnsafeAccessor that can do the equivalent for Type.GetType at compile time (e.g. [UnsafeAccessor(Kind.Type, Name = "The.Type.Name, Assembly")] extern Type GetTheType(); - runtime magic would rewrite this to a method that returns a typeof basically)

@jkotas
Copy link
Member

jkotas commented Dec 2, 2024

generate a global lookup table with all necessary vtables for types it has seen as possibly used across the ABI boundary

These sort of global lookup tables are very common in interop solution. It came up in the context of Android interop recently. Android interop has one of these tables as well (with different set of problems). We may want to look at creating an API that makes it possible to build these tables in the most efficient way possible for each form factor.

cc @AaronRobinsonMSFT

@JakeYallop
Copy link
Contributor

Related (archived repository):

dotnet/linker#1868
dotnet/linker#1794

@MichalStrehovsky
Copy link
Member

I was thinking a bit about how to approach this. The existing switch statement over strings is pretty difficult to analyze (the C# compiler needs to rewrite this significantly since there's no switch over strings in IL). Having a cascade of ifs doesn't perform very well. We probably don't want a cascade of ifs.

In a sense, this is similar to #12260, but the necessity to be able to access internal types makes it more challenging. But still, it would be nice if the potential solution was applicable to both. I don't know if there is one.

The shape that would work the best would be something that is easy to analyze and easy to eliminate. Off the top of my head, I see two mechanisms: method calls, or custom attributes. Method calls would need to be to methods that don't take parameters (if a method takes parameters, it's much harder to erase it).

The method call approach would be something like this:

x.Register<Foo, Bar>();
x.Register<Baz, Bazz>();
// ...

We'd mark the Register method as RequiresUnreferencedCode, but we would intrinsically recognize calls that are in "good shape" and wouldn't generate the RUC warnings. "Good shape" means that the call uses a concrete type, not some generic parameter. Analysis would treat this specially and if e.g. Foo wasn't generated in the program, the whole x.Register line associated with it would be eliminated.

There would be another API to completement the Register call that would take a typeof(Baz) and then do something with Bazz as a result. I purposefully left that out as it goes too much into implementation and I'm not sure what the shape should be, really.

The other approach would be similar, but instead of method calls, we would encode these pairs into custom attributes. Trimming would also treat them specially and if the first type is unused, the whole attribute would be dropped. The disadvantage of attributes is that they are observable - so dropping custom attributes produces observable side effects (and behavior difference between trimmed and untrimmed apps). We could argue that those are "our" attributes and only our blessed APIs should access them, but anyone can. This could be used to implement a "was this type trimmed?" API. We don't really want such API.

Now for accessing internal types - the custom attribute approach would be simpler, we can just have custom attributes accept strings instead of System.Type (or strings in addition to System.Type). The method call approach has a problem because we might not be able to access the type used as the generic parameter. We'd need a new kind of UnsafeAccessor that can call instantiated generic methods using inaccessible type parameters. It's not impossible either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr untriaged New issue has not been triaged by the area owner
Projects
Status: No status
Development

No branches or pull requests

4 participants