search

dotnet / linker

388 stars 126 forks source link

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

Closed jtschuster closed 1 year ago

jtschuster commented 1 year ago

Ports https://github.com/dotnet/linker/pull/3073 and https://github.com/dotnet/linker/pull/3098 to the .net7 release.

Customer Impact:

Trimming speed regressed significantly from .net6 to .net7, as much as 20x slowdown in one reported case (https://github.com/dotnet/linker/issues/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:

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 https://github.com/dotnet/linker/pull/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 (https://github.com/dotnet/linker/pull/3073) passed linker tests but had a bug found in integration to dotnet/sdk (fixed in https://github.com/dotnet/linker/pull/3098, included here). Linker testing may be missing some cases, so there is a chance there are other corner cases missing.

vitek-karas commented 1 year ago

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

jtschuster commented 1 year ago

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

agocke commented 1 year ago

@sbomer could you review this?

agocke commented 1 year ago

@vitek-karas Could you review as well?

agocke commented 1 year ago

Investigated this along with another perf improvement change in https://github.com/dotnet/linker/pull/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 commented 1 year ago

This actually fixes a functional issue as well - recursive generics cause stackoverflow without this change: https://github.com/dotnet/linker/issues/3155