Check for marking virtual method due to base only when state changes#3073
Merged
jtschuster merged 43 commits intodotnet:mainfrom Oct 27, 2022
Merged
Check for marking virtual method due to base only when state changes#3073jtschuster merged 43 commits intodotnet:mainfrom
jtschuster merged 43 commits intodotnet:mainfrom
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
jtschuster
commented
Oct 18, 2022
| MarkInterfaceImplementation (iface, new MessageOrigin (type)); | ||
| } | ||
|
|
||
| bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementation iface) |
Member
Author
There was a problem hiding this comment.
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
commented
Oct 18, 2022
| { | ||
| if (Annotations.IsMarked (iface)) | ||
| return; | ||
| Annotations.MarkProcessed (iface, reason ?? new DependencyInfo (DependencyKind.InterfaceImplementationOnType, ScopeStack.CurrentScope.Origin.Provider)); |
Member
Author
There was a problem hiding this comment.
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
marek-safar
reviewed
Oct 19, 2022
…RC2 SDK. Took the versions from dotnet/runtime.
sbomer
reviewed
Oct 20, 2022
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
Member
Author
|
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. |
sbomer
approved these changes
Oct 27, 2022
Member
sbomer
left a comment
There was a problem hiding this comment.
A few more suggestions, otherwise LGTM. Thanks a lot!
1 task
jtschuster
added a commit
to jtschuster/linker
that referenced
this pull request
Oct 31, 2022
…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>
tlakollo
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 4, 2022
…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
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
…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
vitek-karas
added a commit
to vitek-karas/linker
that referenced
this pull request
Dec 9, 2022
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
vitek-karas
added a commit
that referenced
this pull request
Dec 12, 2022
Recurisve generics with interface marking annotation used to cause stackoverflow in the linker. The test now passes since the problem was fixed in #3073
dotnet-bot
pushed a commit
to dotnet/dotnet
that referenced
this pull request
Dec 13, 2022
….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 ]]
tlakollo
pushed a commit
to tlakollo/linker
that referenced
this pull request
Dec 20, 2022
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
tlakollo
pushed a commit
to tlakollo/runtime
that referenced
this pull request
Dec 22, 2022
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
sbomer
added a commit
that referenced
this pull request
Jan 18, 2023
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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:GetBaseMethodsto return OverrideInformation'sGetInterfaceImplementorsto 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