Check for marking virtual method due to base only when state changes #3073

jtschuster · 2022-10-18T17:12:11Z

Instead of checking every virtual method to see if it should be kept due to a base method every iteration of the MarkStep pipeline, check each method only when its relevant state has changed. These state changes are:

The override's declaring type is marked as instantiated
The base method is marked
The override's declaring type is marked as relevant to variant casting

This required some changes to TypeMapInfo:

modify GetBaseMethods to return OverrideInformation's
add a new method, GetInterfaceImplementors to get all types that implement a given interface.

I profiled the ASP.Net benchmark's build link time and got the following time in CPU ms:
27325 before the regression related to #3067
87310 in main
32011 after this change

This also changes behavior in a couple places:

When a type is marked as instantiated. Previously, we would mark all methods that override a method from a preserved scope, which would mark methods implementing an interface even if the interface implementation isn't kept. Now, the method should only be kept if the interface is marked and the interface implementation is marked.
IDynamicInterfaceCastable is special cased to keep the interface implementation on all types that are instantiated or relevant to variant casting, regardless of whether or not IDynamicInterfaceCastable is marked or not.
The IDynamicCastable test is updated to use the corelib descriptors to keep the IDynamicCastable interface implementations

Closes #3067
Closes #3069

Instead of checking every virtual method to see if it should be kept due to a base method every iteration of the MarkStep pipeline, check each method only when it's relevant state has changed. These state changes are: - The override's declaring type is marked as instantiated - The base method is marked - The overrides's declaring type is marked as relevant to variant casting

…from preserved scope

jtschuster · 2022-10-18T19:32:45Z

src/linker/Linker.Steps/MarkStep.cs

@@ -2334,6 +2376,32 @@ void MarkInterfaceImplementations (TypeDefinition type)
 				if (ShouldMarkInterfaceImplementation (type, iface))
 					MarkInterfaceImplementation (iface, new MessageOrigin (type));
 			}
+
+			bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementation iface)


This is moved to a local method because it assumes that 'type' is marked as instantiated, so we wouldn't want to call it elsewhere.

jtschuster · 2022-10-18T19:39:14Z

src/linker/Linker.Steps/MarkStep.cs


 		protected internal virtual void MarkInterfaceImplementation (InterfaceImplementation iface, MessageOrigin? origin = null, DependencyInfo? reason = null)
 		{
 			if (Annotations.IsMarked (iface))
 				return;
+			Annotations.MarkProcessed (iface, reason ?? new DependencyInfo (DependencyKind.InterfaceImplementationOnType, ScopeStack.CurrentScope.Origin.Provider));


Before moving the MarkProcessed call, the linker hit a cycle where marking iface.InterfaceType would call MarkInterfaceImplementation with the same arguments and overflow the stack

src/linker/Linker.Steps/MarkStep.cs

src/linker/Linker.Steps/SealerStep.cs

src/linker/Linker.Steps/MarkStep.cs

…RC2 SDK. Took the versions from dotnet/runtime.

src/linker/Linker/TypeMapInfo.cs

src/linker/Linker.Steps/MarkStep.cs

…heckMove

Co-authored-by: Sven Boemer <sbomer@gmail.com>

… into virtualCheckMove

- use null check instead of type check - Remove redundant interface check

…RC2 SDK (dotnet#3077) Change analyzer versions such that the repo can be built with .NET 7 RC2 SDK. - Took the versions from dotnet/runtime. - Remove CheckAttributeInstantiation method since is no longer necessary - Remove global attributes since they are no longer necessary - Workaround the fact that compiler injects System.Runtime.CompilerServices.RefSafetyRulesAttribute into every assembly to describe the language version Co-authored-by: tlakollo <tlakaelel_axayakatl@outlook.com>

… into virtualCheckMove

jtschuster · 2022-10-25T20:05:29Z

I rebased by mistake and the git history got messed up and duplicated a bunch of commits. The only real changes should be from 2bc88c0.

…riantCasting

…heckMove

sbomer

A few more suggestions, otherwise LGTM. Thanks a lot!

src/linker/Linker.Steps/MarkStep.cs

…levant

…otnet#3073) Instead of checking every virtual method to see if it should be kept due to a base method every iteration of the MarkStep pipeline, check each method only when its relevant state has changed. Co-authored-by: Sven Boemer <sbomer@gmail.com>

…otnet/linker#3073) Instead of checking every virtual method to see if it should be kept due to a base method every iteration of the MarkStep pipeline, check each method only when its relevant state has changed. Co-authored-by: Sven Boemer <sbomer@gmail.com> Commit migrated from dotnet/linker@8306887

Recurisve generics with interface marking annotation used to cause stackoverflow in the linker. The test now passes since the problem was fixed in dotnet#3073

Recurisve generics with interface marking annotation used to cause stackoverflow in the linker. The test now passes since the problem was fixed in #3073

….0 (#3156) Recurisve generics with interface marking annotation used to cause stackoverflow in the linker. The test now passes since the problem was fixed in dotnet/linker#3073 Original commit: dotnet/linker@dde6d62 [[ commit created by automation ]]

Recurisve generics with interface marking annotation used to cause stackoverflow in the linker. The test now passes since the problem was fixed in dotnet#3073 Commit migrated from dotnet@dde6d62

Recurisve generics with interface marking annotation used to cause stackoverflow in the linker. The test now passes since the problem was fixed in dotnet/linker#3073 Commit migrated from dotnet/linker@dde6d62

…state changes (#3094) * Check for marking virtual method due to base only when state changes (#3073) Instead of checking every virtual method to see if it should be kept due to a base method every iteration of the MarkStep pipeline, check each method only when its relevant state has changed. Co-authored-by: Sven Boemer <sbomer@gmail.com> * Don't mark override of abstract base if the override's declaring type is not marked (#3098) * Don't mark an override every time the base is abstract, only if the declaring type is also marked Adds a condition to ShouldMarkOverrideForBase to exit early if the declaring type of the method is not marked. * Add test case for #3112 with pseudo-circular reference with ifaces * Link issue to TODO * Adds a test for recursive generics on interfaces This is a copy of the test added in #3156 Co-authored-by: Sven Boemer <sbomer@gmail.com> Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>

jtschuster added 2 commits October 17, 2022 16:52

Special case IDynamicInterfaceCastable and don't keep all interfaces …

bdca16a

…from preserved scope

jtschuster commented Oct 18, 2022

View reviewed changes

Remove unused method and redundant while loop

e5643fb

jtschuster requested review from sbomer and vitek-karas October 18, 2022 20:11

jtschuster marked this pull request as ready for review October 18, 2022 20:11

jtschuster requested a review from marek-safar as a code owner October 18, 2022 20:11

jtschuster mentioned this pull request Oct 18, 2022

Use LinkContext caching when resolving ExportedTypes #3074

Closed

Add comments to TypeMapInfo methods

3854d3f

jtschuster changed the title ~~Check for marking method for base only when state changes~~ Check for marking virtual method due to base only when state changes Oct 18, 2022

jtschuster mentioned this pull request Oct 18, 2022

Use LinkContext caching when resolving ExportedTypes #3075

Merged

marek-safar reviewed Oct 19, 2022

View reviewed changes

vitek-karas and others added 3 commits October 19, 2022 04:37

Change analyzer versions such that the repo can be built with .NET 7 …

04801f1

…RC2 SDK. Took the versions from dotnet/runtime.

PR feedback: Fix typo and return early

d406e2b

Remove commented code and extra null check

20c52af

sbomer reviewed Oct 20, 2022

View reviewed changes

src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved

src/linker/Linker/TypeMapInfo.cs Outdated Show resolved Hide resolved

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

jtschuster and others added 10 commits October 20, 2022 12:39

Merge branch 'main' of https://github.com/dotnet/linker into virtualC…

c027846

…heckMove

Merge branch 'EnableRC2'

3dbce05

Update src/linker/Linker/TypeMapInfo.cs

6484a01

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Update src/linker/Linker/TypeMapInfo.cs

e6c0429

Co-authored-by: Sven Boemer <sbomer@gmail.com>

Merge branch 'virtualCheckMove' of https://github.com/jtschuster/linker…

d53e433

… into virtualCheckMove

PR feedback:

324d4cb

- use null check instead of type check - Remove redundant interface check

format

756f547

remove using

e18e579

Don't ignore descriptors in test

cd38208

Merge branch 'main' of https://github.com/dotnet/linker

8088e70

jtschuster requested a review from sbomer October 25, 2022 03:55

jtschuster mentioned this pull request Oct 25, 2022

TrimMode partial very slow to link #3083

Closed

jtschuster and others added 6 commits October 25, 2022 12:54

Don't ignore descriptors in test

0de4d04

Don't go through bases of bases when in GetMarkedBaseMethods

c84326e

Remove GetInterfaceImplementors

2bc88c0

Merge branch 'virtualCheckMove' of https://github.com/jtschuster/linker…

959c7c3

… into virtualCheckMove

Remove GetInterfaceImplementors

fdc6bb3

jtschuster added 4 commits October 25, 2022 13:12

Remove leftover IsDirectBase code

0cf21d0

Merge branch 'virtualCheckMove2' into virtualCheckMove

eb6a399

Remove call to MarkMethodIfNeededByBaseMethod from MarkAsRelevantToVa…

07c28b5

…riantCasting

Merge branch 'main' of https://github.com/dotnet/linker into virtualC…

c2dcdfe

…heckMove

jtschuster requested a review from sbomer October 27, 2022 17:50

Merge branch 'main' of https://github.com/dotnet/linker into virtualC…

7b85004

…heckMove

sbomer approved these changes Oct 27, 2022

View reviewed changes

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved

jtschuster mentioned this pull request Oct 27, 2022

Move all "Should method be marked in the current analysis state" logic to one place #3090

Open

1 task

jtschuster added 2 commits October 27, 2022 13:53

PR Feedback: inline methods and remove changes that are not longer re…

49d2e0f

…levant

Remove outdated comment

4c12f43

jtschuster enabled auto-merge (squash) October 27, 2022 22:06

jtschuster merged commit 8306887 into dotnet:main Oct 27, 2022

jtschuster mentioned this pull request Oct 31, 2022

[release/7.0] Check for marking virtual method due to base only when state changes #3094

Merged

vitek-karas mentioned this pull request Dec 9, 2022

Add a test which uncovered stack overflow in 7.0 #3156

Merged

vitek-karas added a commit that referenced this pull request Dec 12, 2022

Add a test which uncovered stack overflow in 7.0 (#3156)

dde6d62

Recurisve generics with interface marking annotation used to cause stackoverflow in the linker. The test now passes since the problem was fixed in #3073

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for marking virtual method due to base only when state changes #3073

Check for marking virtual method due to base only when state changes #3073

jtschuster commented Oct 18, 2022 •

edited

Loading

jtschuster Oct 18, 2022

jtschuster Oct 18, 2022

jtschuster commented Oct 25, 2022

sbomer left a comment

Check for marking virtual method due to base only when state changes #3073

Check for marking virtual method due to base only when state changes #3073

Conversation

jtschuster commented Oct 18, 2022 • edited Loading

jtschuster Oct 18, 2022

Choose a reason for hiding this comment

jtschuster Oct 18, 2022

Choose a reason for hiding this comment

jtschuster commented Oct 25, 2022

sbomer left a comment

Choose a reason for hiding this comment

jtschuster commented Oct 18, 2022 •

edited

Loading