[release/7.0] Check for marking virtual method due to base only when state changes #3094
Conversation
…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>
marek-safar
changed the title
… is not marked (dotnet#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.
|
Given the perf improvements shown above, I agree we should try to take this for servicing. |
|
I've verified locally that this works in the 7.0 runtime builds and runtime tests pass after using it. |
|
@sbomer could you review this? |
|
@vitek-karas Could you review as well? |
| [SetupLinkerTrimMode ("link")] | ||
| [IgnoreDescriptors (false)] |
There was a problem hiding this comment.
I would expect these to be the defaults - do we need to specify it?
There was a problem hiding this comment.
Yes, the defaults are to ignore descriptor files and "skip" trim mode for references.
| public static void Foo () | ||
| { | ||
| ((IFoo) null).Method (); | ||
| object x = null; |
There was a problem hiding this comment.
Why do we need this line object x = null - it doesn't seem related in any way to the rest of the code here.
| [SetupCompileBefore ("base.dll", new[] { "Dependencies/Base.cs" })] // Base Implements IFoo.Method (psuedo-reference to ifoo.dll) | ||
| [SetupCompileBefore ("ifoo.dll", new[] { "Dependencies/IFoo.cs" }, references: new[] { "base.dll" })] // Derived2 references base.dll (circular reference) | ||
| [SetupCompileBefore ("derived1.dll", new[] { "Dependencies/Derived1.cs" }, references: new[] { "ifoo.dll", "base.dll" })] | ||
| public class BaseProvidesInterfaceMethodCircularReference |
There was a problem hiding this comment.
Would be good to check that we kept the Base.Method (as the actual implementation method)
And also that we removed Derived2 since it's not used by anything (even though it affects the linker's internal structures)
| // Methods on instantiated types that override a ov.Override from a base type (not an interface) should be marked | ||
| // Interface ov.Overrides should only be marked if the interfaceImplementation is marked, which is handled below |
There was a problem hiding this comment.
This explanation is... weird. I honestly couldn't figure it out.
What does it mean "method which overrides something from a base type? Or maybe I'm reading it wrong.
I think what this is trying to say is something like:
"An override needs to be kept if - a) it's an override on an instantiated type (of a marked base) or b) it's an override of an abstract base (where base is marked, and thus the override must be there to produce valid IL - we also alread operate only on overrides which type is marked)
| if (Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, method) && isInstantiated) { | ||
| MarkMethod (method, new DependencyInfo (DependencyKind.OverrideOnInstantiatedType, method.DeclaringType), ScopeStack.CurrentScope.Origin); | ||
| /// <summary> | ||
| /// Marks the Override of <paramref name="overrideInformation"/> with the correct reason. Should be called when <see cref="ShouldMarkOverrideForBase(OverrideInformation, bool)"/> returns true. |
There was a problem hiding this comment.
If it's a prereq that ShouldMarkOverrideForBase returns true, then maybe this should assert it?
| var markedBaseMethods = bases.Where (ov => Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)); | ||
| foreach (var ov in markedBaseMethods) { | ||
| if (ShouldMarkOverrideForBase (ov)) | ||
| MarkOverrideForBaseMethod (ov); |
There was a problem hiding this comment.
There are only two places where we use this - and they're both in the form "if (Should ...) Mark...." - It would feel cleaner if we had just one method "MarkOverrideForBaseAsNecessary" (or something like that), which does the checks and marking in one go (would also avoid the current assumption that Mark is only called when Should returned true).
But maybe this is solved after we fix 3090
| @@ -2027,6 +2043,10 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in | |||
| _typesWithInterfaces.Add ((type, ScopeStack.CurrentScope)); | |||
|
|
|||
| if (type.HasMethods) { | |||
| // TODO: MarkMethodIfNeededByBaseMethod should include logic for IsMethodNeededBytTypeDueToPreservedScope | |||
There was a problem hiding this comment.
Link to a bug - probably 3090?
| // TODO: MarkMethodIfNeededByBaseMethod should include logic for IsMethodNeededBytTypeDueToPreservedScope | ||
| foreach (var method in type.Methods) { | ||
| MarkMethodIfNeededByBaseMethod (method); | ||
| } | ||
| // For methods that must be preserved, blame the declaring type. | ||
| MarkMethodsIf (type.Methods, IsMethodNeededByTypeDueToPreservedScope, new DependencyInfo (DependencyKind.VirtualNeededDueToPreservedScope, type), ScopeStack.CurrentScope.Origin); |
There was a problem hiding this comment.
This is basically a second "foreach ( ... in type.Methods)" - would look nicer if we merged them into one foreach.
|
Investigated this along with another perf improvement change in #3150. That change absolutely pales in comparison to this one. With this fix some of my test apps went from 30s compiles to 6s compiles. I think this change is worth taking, the other one maybe not. |
|
This actually fixes a functional issue as well - recursive generics cause stackoverflow without this change: |
This is a copy of the test added in dotnet#3156
* Address PR comments from PR dotnet#3094 Commit migrated from dotnet@e816e73
* Address PR comments from PR dotnet/linker#3094 Commit migrated from dotnet/linker@e816e73
Ports #3073 and #3098 to the .net7 release.
Customer Impact:
Trimming speed regressed significantly from .net6 to .net7, as much as 20x slowdown in one reported case (#3083).
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.
This also changes behavior when a type is marked as instantiated. Previously, we would mark all methods that override a method from a preserved scope (i.e. in an assembly that is not to be trimmed), 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.
Co-authored-by: Sven Boemer sbomer@gmail.com
Profiling the linker locally with VS on a hello world console app, I got the following results:
This PR takes 69% as long as release/7.0, a 1.45x speedup
This is probably where we would see the least improvement, since the virtual method issues scaled particularly poorly (in ASP.Net benchmark build, the time went from 87310 cpu ms to 32011 cpu ms (a 2.72x speedup) after #3073), so I think with a 30+% reduction in execution time on hello world, we should try to take it to servicing.
Testing:
This change should only change behavior in the situation where a type is instantiated but an interface implementation of an interface in 'skip' assembly is unmarked, and the interface type is not marked. Previously, the linker would mark the interface implementation and interface always once the type is marked as instantiated. Now, the linker will only mark the interface implementation if the interface type is marked.
Risk:
The first commit related to this change (#3073) passed linker tests but had a bug found in integration to dotnet/sdk (fixed in #3098, included here). Linker testing may be missing some cases, so there is a chance there are other corner cases missing.