-
Notifications
You must be signed in to change notification settings - Fork 128
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
Check for marking virtual method due to base only when state changes #3073
Conversation
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
@@ -2334,6 +2376,32 @@ void MarkInterfaceImplementations (TypeDefinition type) | |||
if (ShouldMarkInterfaceImplementation (type, iface)) | |||
MarkInterfaceImplementation (iface, new MessageOrigin (type)); | |||
} | |||
|
|||
bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementation iface) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
…RC2 SDK. Took the versions from dotnet/runtime.
Co-authored-by: Sven Boemer <sbomer@gmail.com>
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few more suggestions, otherwise LGTM. Thanks a lot!
…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
…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>
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:
This required some changes to
TypeMapInfo
:GetBaseMethods
to return OverrideInformation'sGetInterfaceImplementors
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:
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.Closes #3067
Closes #3069