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

Trim unused interfaces #100000

Merged
merged 3 commits into from Mar 23, 2024
Merged

Conversation

MichalStrehovsky
Copy link
Member

So far, the compiler could only trim unused interface methods; the interface types themselves were left alone. There is not a huge cost associated with having these extra MethodTables, but it sill impacts size/working set/startup. Initial estimate showed this could actually be up to 2% for BasicMinimalApi so I decided to investigate a fix.

This is an attempt to start trimming them. I chose a relatively conservative approach:

  • Stop unconditionally rooting the interface MethodTable in the implementing class MethodTable InterfaceList. Instead check whether someone else marked it first.
  • Track whether an interface type definition is used in any way. This avoids problem with variance, etc. If an interface definition is used, all instantiations that we were trying to optimize away get the MethodTable and won't be optimized. We should be able to do better than this, maybe, at some point.
  • Classes that implement generic interfaces with default methods need special treatment because we index into interface list at runtime and we might not know the correct index yet. So just forget the optimization in that case.

Fixes #66716.

So far, the compiler could only trim unused interface __methods__; the interface __types__ themselves were left alone. There is not a huge cost associated with having these extra `MethodTable`s, but it sill impacts size/working set/startup. Initial estimate showed this could actually be up to 2% for BasicMinimalApi so I decided to investigate a fix.

This is an attempt to start trimming them. I chose a relatively conservative approach:

* Stop unconditionally rooting the interface `MethodTable` in the implementing class `MethodTable` InterfaceList. Instead check whether someone else marked it first.
* Track whether an interface type definition is used in any way. This avoids problem with variance, etc. If an interface definition is used, all instantiations that we were trying to optimize away get the `MethodTable` and won't be optimized. We should be able to do better than this, maybe, at some point.
* Classes that implement generic interfaces with default methods need special treatment because we index into interface list at runtime and we might not know the correct index yet. So just forget the optimization in that case.

Fixes dotnet#66716.
@filipnavara
Copy link
Member

I vaguely remember that some part of the iOS managed static registrar or the linker pipeline may depend on the current behavior. So, cc @ivanpovazan, @simonrozsival in case they do remember the details or have some general concerns.

@simonrozsival
Copy link
Member

@filipnavara If I understand this change correctly, I don't think this should break iOS. There are cases when we explicitly cast objects to an interface they implement and call a method on the interface. I think that falls under the second bullet point and in that case the optimization won't be applied.

@filipnavara
Copy link
Member

I think that falls under the second bullet point and in that case the optimization won't be applied.

Thanks for checking!

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@EgorBo
Copy link
Member

EgorBo commented Mar 20, 2024

ID: 100000 🎉

@MichalStrehovsky
Copy link
Member Author

So this won't be quite the promised 2% size saving on WebapiAot template because once I started to worry about generic interfaces and the suppressions that assume IL level trimming we have within the libraries, it became 0.7%.

In libraries, we have several places where we call GetInterfaces() without a properly annotated/statically known type and assume that if we grab the type definition of the interface we got from GetInterfaces() and compare with typeof(OpenVersion<>), it's going to be reliable and trim safe way to detect if the type implemented say OpenVersion<int>. It's safe in IL sense. In AOT sense, we could have stripped OpenVersion<int> if it was never used and then this trim-unsafe piece of code won't find it. So we can't do it. Oh well.

But the good news is that dotnet new console --aot followed by dotnet publish /p:OptimizationPreference=Size /p:StackTraceSupport=false /p:UseSystemResourceKeys=true is now 877 kB.

Size statistics

Pull request #100000

Project Size before Size after Difference
avalonia.app-linux 24681968 24615120 -66848
avalonia.app-windows 22321152 22253568 -67584
hello-linux 1316304 1266592 -49712
hello-minimal-linux 1172808 1119000 -53808
hello-minimal-windows 947200 898560 -48640
hello-windows 1200640 1149952 -50688
webapiaot-linux 10363808 10301344 -62464
webapiaot-windows 9438720 9377280 -61440

@MichalStrehovsky MichalStrehovsky marked this pull request as ready for review March 21, 2024 11:07
@MichalStrehovsky
Copy link
Member Author

@dotnet/ilc-contrib this is ready for review

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@MichalStrehovsky
Copy link
Member Author

Hmm, the linux-arm failure is interesting but unrelated:

(lldb) bt
This version of LLDB has no plugin for the language "assembler". Inspection of frame variables will be limited.
* thread #1, name = 'Microsoft.Exten', stop reason = signal SIGSEGV
  * frame #0: 0x00eab31e Microsoft.Extensions.Logging.Console.Tests`RhpCheckedAssignRefr1 at WriteBarriers.S:251
    frame #1: 0x011ec304 Microsoft.Extensions.Logging.Console.Tests`System.Runtime.TypeCast___cctor at TypeCast.cs:36
    frame #2: 0x0121c2b8 Microsoft.Extensions.Logging.Console.Tests`Internal.Runtime.CompilerHelpers.StartupCodeHelpers__RunInitializers(typeManager=<unavailable>, section=<unavailable>) at StartupCodeHelpers.cs:181
    frame #3: 0x0121be8e Microsoft.Extensions.Logging.Console.Tests`Internal.Runtime.CompilerHelpers.StartupCodeHelpers__InitializeModules(osModule=<unavailable>, pModuleHeaders=<unavailable>, count=<unavailable>, pClasslibFunctions=<unavailable>, nClasslibFunctions=<unavailable>) at StartupCodeHelpers.cs:53
    frame #4: 0x00e6f4d4 Microsoft.Extensions.Logging.Console.Tests`main [inlined] InitializeRuntime() at main.cpp:203:5 [opt]
    frame #5: 0x00e6f470 Microsoft.Extensions.Logging.Console.Tests`main(argc=9, argv=0xffca6804) at main.cpp:221:19 [opt]
(lldb) dis -b -A thumbv7
Microsoft.Extensions.Logging.Console.Tests`RhpCheckedAssignRef:
    0xeab2be <+0>:   0xf3bf8f5f   dmb    sy

Microsoft.Extensions.Logging.Console.Tests`RhpCheckedAssignRefr1:
    0xeab2c2 <+4>:   0x6001       str    r1, [r0]
    0xeab2c4 <+6>:   0xf2400c4c   movw   r12, #0x4c
    0xeab2c8 <+10>:  0xf2c00ca0   movt   r12, #0xa0
    0xeab2cc <+14>:  0x44fc       add    r12, pc
    0xeab2ce <+16>:  0xf8dcc000   ldr.w  r12, [r12]
    0xeab2d2 <+20>:  0x4560       cmp    r0, r12
    0xeab2d4 <+22>:  0xd328       blo    0x89b328                  ; <+106>
    0xeab2d6 <+24>:  0xf2400c3e   movw   r12, #0x3e
    0xeab2da <+28>:  0xf2c00ca0   movt   r12, #0xa0
    0xeab2de <+32>:  0x44fc       add    r12, pc
    0xeab2e0 <+34>:  0xf8dcc000   ldr.w  r12, [r12]
    0xeab2e4 <+38>:  0x4560       cmp    r0, r12
    0xeab2e6 <+40>:  0xd21f       bhs    0x89b328                  ; <+106>
    0xeab2e8 <+42>:  0xf6470ca0   movw   r12, #0x78a0
    0xeab2ec <+46>:  0xf2c00c9e   movt   r12, #0x9e
    0xeab2f0 <+50>:  0x44fc       add    r12, pc
    0xeab2f2 <+52>:  0xf8dcc000   ldr.w  r12, [r12]
    0xeab2f6 <+56>:  0x4561       cmp    r1, r12
    0xeab2f8 <+58>:  0xd31b       blo    0x89b332                  ; <+116>
    0xeab2fa <+60>:  0xf6470c92   movw   r12, #0x7892
    0xeab2fe <+64>:  0xf2c00c9e   movt   r12, #0x9e
    0xeab302 <+68>:  0x44fc       add    r12, pc
    0xeab304 <+70>:  0xf8dcc000   ldr.w  r12, [r12]
    0xeab308 <+74>:  0x4561       cmp    r1, r12
    0xeab30a <+76>:  0xd212       bhs    0x89b332                  ; <+116>
    0xeab30c <+78>:  0xf2400c00   movw   r12, #0x0
    0xeab310 <+82>:  0xf2c00c9f   movt   r12, #0x9f
    0xeab314 <+86>:  0x44fc       add    r12, pc
    0xeab316 <+88>:  0xf8dcc000   ldr.w  r12, [r12]
    0xeab31a <+92>:  0xeb0c2090   add.w  r0, r12, r0, lsr #10
->  0xeab31e <+96>:  0xf890c000   ldrb.w r12, [r0]
    0xeab322 <+100>: 0xf1bc0fff   cmp.w  r12, #0xff
    0xeab326 <+104>: 0xd100       bne    0x89b32a                  ; <+108>
    0xeab328 <+106>: 0xe003       b      0x89b332                  ; <+116>
    0xeab32a <+108>: 0xf04f0cff   mov.w  r12, #0xff
    0xeab32e <+112>: 0xf880c000   strb.w r12, [r0]
    0xeab332 <+116>: 0x4770       bx     lr
(lldb)

Cc @filipnavara

Runfo:

runfo get-helix-payload -j 320338f9-9d8c-4deb-9eeb-884cfb26892e -w Microsoft.Extensions.Logging.Console.Tests -o c:\helix_payload\Microsoft.Extensions.Logging.Console.Tests

@MichalStrehovsky
Copy link
Member Author

I'll wait with merging this until after #98712. I don't want to mask off how much of a regression that PR is.

@MichalStrehovsky MichalStrehovsky added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 21, 2024
@jkotas
Copy link
Member

jkotas commented Mar 22, 2024

the linux-arm failure is interesting

Opened #100112

@MichalStrehovsky MichalStrehovsky removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 23, 2024
@MichalStrehovsky MichalStrehovsky merged commit 17f8138 into dotnet:main Mar 23, 2024
20 of 22 checks passed
@MichalStrehovsky MichalStrehovsky deleted the intfstrip branch March 23, 2024 22:28
@github-actions github-actions bot locked and limited conversation to collaborators Apr 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[NativeAOT] Metadata not trimmed for nested Interface types when compiling TerraFx.Interop.Windows
5 participants