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

[ILLink] Mark recursive interface implementations in MarkStep #99922

Merged
merged 8 commits into from
Mar 27, 2024

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Mar 18, 2024

From #99566 (comment), splitting off a simpler change that doesn't cache the recursive interfaces and doesn't use the recursive interfaces in DIM finding. This does nearly double the time it takes to trim a hello world, whereas #99566 speeds it up slightly, so I'd be concerned about timeouts if this gets merged.

- ILVerify will fail due to members not being kept, so to make the
  errors more informative, run kept member validation first, then
  ILVerify
@jtschuster jtschuster requested a review from marek-safar as a code owner March 18, 2024 18:53
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 18, 2024
@jtschuster
Copy link
Member Author

jtschuster commented Mar 18, 2024

The only test diff compared with #99566 comes in MultipleDimsProvidedByRecursiveInterface.cs:

@@ -16,14 +16,16 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.DefaultInterfaceMethods
        [KeptMemberInAssembly ("library.dll", typeof(Program.I0), "Method()")]
        [KeptTypeInAssembly ("library.dll", typeof(Program.I00))]
        [KeptMemberInAssembly ("library.dll", typeof(Program.I00), "Program.I0.Method()")]
-       [KeptMemberInAssembly ("library.dll", typeof(Program.I01), "Program.I0.Method()")]
+       // Bug: DIM resolution doesn't look at recursive interfaces
+       //[KeptMemberInAssembly ("library.dll", typeof(Program.I01), "Program.I0.Method()")]
        [KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.I00), "library.dll", typeof (Program.I0))]
        [KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.MyFoo), "library.dll", typeof (Program.I000))]
        [KeptTypeInAssembly ("library.dll", typeof(Program.I000))]
        [KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.I000), "library.dll", typeof (Program.I00))]
-       [KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.MyFoo), "library.dll", typeof (Program.I010))]
-       [KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.I010), "library.dll", typeof (Program.I01))]
-       [KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.I01), "library.dll", typeof (Program.I0))]
+       // Bug: DIM resolution doesn't look at recursive interfaces
+       //[KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.MyFoo), "library.dll", typeof (Program.I010))]
+       //[KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.I010), "library.dll", typeof (Program.I01))]
+       //[KeptInterfaceOnTypeInAssembly ("library.dll", typeof (Program.I01), "library.dll", typeof (Program.I0))]
 #endif
        class MultipleDimsProvidedByRecursiveInterface
        {

@jtschuster jtschuster requested a review from sbomer March 18, 2024 19:03
@jtschuster jtschuster added area-Tools-ILLink .NET linker development as well as trimming analyzers and removed linkable-framework Issues associated with delivering a linker friendly framework needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Mar 18, 2024
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 18, 2024
@jtschuster jtschuster changed the title Mark recursive interface [ILLink] Mark recursive interface implementations in MarkStep Mar 18, 2024
@jtschuster jtschuster added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 18, 2024
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Could we mitigate the perf impact by caching the recursive interfaces? It seems like that would still be substantially simpler than #99566.

@sbomer
Copy link
Member

sbomer commented Mar 21, 2024

Do we need to update any other calls to type.Interfaces to look at the recursive interfaces?

@jtschuster
Copy link
Member Author

Do we need to update any other calls to type.Interfaces to look at the recursive interfaces?

Yeah, there are a few places, like interface method resolution, but they might be fairly significant changes, so should we separate them out into their own PRs?

Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

Yeah, there are a few places, like interface method resolution, but they might be fairly significant changes, so should we separate them out into their own PRs?

That seems fine to me. The worst I would expect is that we miss marking some interfaces which we are already missing today.

src/tools/illink/src/linker/Linker/Annotations.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
src/tools/illink/src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved
jtschuster and others added 2 commits March 25, 2024 17:33
@jtschuster jtschuster removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 26, 2024
Copy link
Member

@sbomer sbomer left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@jtschuster jtschuster merged commit 1fa699e into dotnet:main Mar 27, 2024
77 of 80 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants