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

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

Merged
merged 5 commits into from
Jan 18, 2023

Conversation

jtschuster
Copy link
Member

@jtschuster jtschuster commented Oct 31, 2022

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 [email protected]

Profiling the linker locally with VS on a hello world console app, I got the following results:

release/6.0.x release/7.0 This PR
2846 cpu ms 5088 cpu ms 3528 cpu ms

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.

…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 <[email protected]>
@marek-safar marek-safar added this to the .NET 7.0.x milestone Nov 1, 2022
@marek-safar marek-safar changed the title Check for marking virtual method due to base only when state changes [release/7.0] Check for marking virtual method due to base only when state changes Nov 1, 2022
… 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.
@vitek-karas
Copy link
Member

Given the perf improvements shown above, I agree we should try to take this for servicing.

@jtschuster
Copy link
Member Author

I've verified locally that this works in the 7.0 runtime builds and runtime tests pass after using it.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

approved. please get a few code reviews (given the size), and we can take for consideration in 7.0.x

@agocke
Copy link
Member

agocke commented Nov 22, 2022

@sbomer could you review this?

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.

The change looks good to me, but I did already spend significant time with the original changes, so another pair of eyes might be good.

src/linker/Linker.Steps/MarkStep.cs Outdated Show resolved Hide resolved
@agocke
Copy link
Member

agocke commented Nov 28, 2022

@vitek-karas Could you review as well?

Comment on lines +19 to +20
[SetupLinkerTrimMode ("link")]
[IgnoreDescriptors (false)]
Copy link
Member

Choose a reason for hiding this comment

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

I would expect these to be the defaults - do we need to specify it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the defaults are to ignore descriptor files and "skip" trim mode for references.

public static void Foo ()
{
((IFoo) null).Method ();
object x = null;
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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)

Comment on lines +736 to +737
// 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
Copy link
Member

Choose a reason for hiding this comment

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

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.
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

This is basically a second "foreach ( ... in type.Methods)" - would look nicer if we merged them into one foreach.

Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

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

The change looks good for servicing.

Sorry I used this as a way to suggest lot of cleanup, which we should not do in servicing. But please consider the comments for future PR into main.

jtschuster added a commit to jtschuster/linker that referenced this pull request Nov 29, 2022
jtschuster added a commit that referenced this pull request Nov 30, 2022
* Address PR comments from PR #3094
@agocke
Copy link
Member

agocke commented Dec 7, 2022

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.

@vitek-karas
Copy link
Member

This actually fixes a functional issue as well - recursive generics cause stackoverflow without this change:
#3155

tlakollo pushed a commit to tlakollo/linker that referenced this pull request Dec 20, 2022
* Address PR comments from PR dotnet#3094

Commit migrated from dotnet@e816e73
tlakollo pushed a commit to tlakollo/runtime that referenced this pull request Dec 22, 2022
@sbomer sbomer merged commit ae8160b into dotnet:release/7.0 Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants