Don't remove unused interfaces in library mode #2711

jtschuster · 2022-03-28T19:16:17Z

@vitek-karas's changes in the linker appears to fix the library interface trimming issue without any additions. I've just added another tests with behavior closer to the issue.

After linking, IPAddressInformationCollection retains the ICollection interface and the properties and methods:

Fixes #2238

This will make sure that we keep all interfaces and all interface method implementations on such type.

sbomer · 2022-03-29T16:59:52Z

src/linker/Linker.Steps/MarkStep.cs

+					if (unusedInterfacesOptimizationEnabled)
+						continue;
+
+					// If the optimization is disabled, make sure to mark all methods which implement interfaces


I'm not sure I understand why we need this logic. It doesn't actually seem to mark all methods which implement interfaces, so maybe the comment needs clarification - but either way I would have expected that the logic in ProcessOverride would already be marking the required methods which implement interface methods.

You might be right - I must admit that I still don't fully grasp the exact order in which things happen in this case.
I'm pretty sure the comment was there because that was what I wanted to do - but then I realized that I don't need that actually since ProcessOverride will do it for me. So I simplified the code. It's possible it can be entirely removed.

What the code does currently is that it will mark methods which are overrides/impls of methods from non-link assemblies (in library mode that's basically anything outside of the current assembly). I'm not sure if we can remove that fully - it depends if in library mode we mark all of the non-link assemblies, I'm not sure we do.

I'm not 100% sure what we currently do for "library mode" since we never really came up with a design for it, but I think you're right that we don't mark assemblies other than the library. That's also what would make sense to me.

So I guess ProcessOverride won't be called for the case of a non-virtual method implementing an interface method defined in a referenced assembly - then I can see why we would need this logic. I wonder if there's a way we could simplify this, since it's very similar to IsVirtualNeededByTypeDueToPreservedScope. It feels like we want to do substantially the same thing for virtual method overrides and for interface method implementations.

Maybe that could be generalized to IsMethodNeededByTypeDueToPreservedScope so that it works for non-virtual methods, and methods implementing interfaces.

sbomer · 2022-03-29T17:02:31Z

src/linker/Linker.Steps/MarkStep.cs

+				// UnusedInterfaces optimization is turned off mark all interface implementations
+				bool unusedInterfacesOptimizationEnabled = Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, type);
+				if (!Annotations.IsInstantiated (type) && !Annotations.IsRelevantToVariantCasting (type) &&
+					unusedInterfacesOptimizationEnabled)


It looks like this is marking the .interfaceimpl unconditionally. Is that what we want? It seems like it could result in keeping the impl for private interfaces, which isn't strictly necessary for library mode - it should be ok to remove impls of unused private interfaces.

If we want to keep all impls for simplicity, it's worth clarifying whether the intention is to keep all interface methods as well. My gut reaction is that it would be confusing to keep all impls but still allow trimming unused interface methods.

You're right - this didn't consider private interfaces.

vitek-karas · 2022-03-30T15:36:57Z

We might need to modify the behavior of this change. I think the best description of the desired behavior for library mode is:

Public types must keep all public interfaces even if they're not instantiated. Not doing this means that we remove parts of the public API effectively. Library mode needs to guarantee that it's not going to change the public API of the assembly in any way.

The change doesn't try to implement exactly that, instead it tries to define and implement what is the correct behavior if the UnusedInterfaces optimization is disabled. It's questionable if these two should mean the same thing. If they're not the same we would have either add a new optimization or modify how rooting is done in library mode.

What library mode already does:

All public types are marked
- Note that this includes public interface types
All public methods on these types are marked
- Note that this means that public interfaces are effectively entirely preserved

What we could do is change the linker such that if UnusedInterfaces optimization is disabled we will on all marked types keep all interface impls for marked interfaces - always (so regardless if the type is instantiated or not).

This would mean that since we already mark all public types and interfaces, we will preserve all public interfaces on all public types. It also means that the public interfaces will keep all their methods and thus we should keep all the method impls of such interfaces.

The remaining question is what is the desired behavior of UnusedInterfaces=false for a type which implements private interface and the interface is not used anywhere. Based on the name of the optimization, I would prefer we simply keep all interfaces of a marked type (public or private). Such behavior would not be necessary for library mode though - it would be effectively overmarking slightly. The question is if we should then rename the optimization to better fit the library mode requirements or "relax" library mode trimming (or add a new optimization which fits the library mode).

Personally I would keep this simple and "relax" the library mode trimming - the size impact of this should be relatively small, but we need to measure for sure. But my preference is almost purely based on the simplicity of the solution - with the assumption that size impact is negligible.

vitek-karas · 2022-03-30T20:14:43Z

Discussed this with Sven offline: we would like to keep this as simple as possible - so we should try to go with the "keep all interfaces on marked types" solution (basically fix the behavior of UnusedInterfaces=false). Measure the size impact on framework and then decide if it's good enough.

marek-safar · 2022-03-31T06:38:47Z

test/Mono.Linker.Tests.Cases/Libraries/RootLibrary.cs

@@ -8,6 +12,7 @@ namespace Mono.Linker.Tests.Cases.Libraries
 {
 	[SetupLinkerArgument ("-a", "test.exe", "library")]
 	[SetupLinkerArgument ("--enable-opt", "ipconstprop")]
+	[SetupLinkerArgument ("--skip-unresolved")]


Could you come up with a test that does not require this extra setting?

…ad of Interface specific version

vitek-karas · 2022-04-05T13:45:06Z

I spent some time looking into this in more detail and also into the static interface problem mentioned in dotnet/runtime#67453.

I also had several discussions about this. My preferred solution is:

Actually fully implement support for optimization UnusedInterfaces - that is both if it's on and if it's off (currently linker really only implements the "on" behavior).
Define the UnusedInterfaces optimization as meaning "Ability to remove interface implementations from a type if the interface implementation is provably not being used on this type". This means:
- If the optimization is off types will keep implementations of all interfaces
- That is, if type implements interface IA and IA is not used anywhere else, linker should still preserve IA (this is already handled in here:
  
  linker/src/linker/Linker.Steps/MarkStep.cs
  
  Line 3480 in b9b72d1
  
  if (!Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, type))
  
  ) (@jtschuster this is different from our discussion, I just noticed that linker already implements this behavior).
- This also means that if the optimization is off, interfaces are kept regardless of their visibility
Implement the off behavior as described above

For the implementation I would keep it simple for now, that is very close to the current state of this PR - keep the delayed processing of types with interfaces, and simply disable the "is instantiated" check if the optimization is off. The alternative would be to mark the interface implementations eagerly, but that would introduce much larger difference in behavior with the optimization on/off which is more risky (and really adds complexity where it's unnecessary).

This means we need the new code which iterates over all methods on a type if its interfaces should be kept and uses IsMethodNeededByTypeDueToPreservedScope to keep additional methods. The main reason why we need this is that assemblies which are not "link" (so for example all "copy" assemblies) are not "marked" - that is methods on interfaces in those assemblies are not "marked" and are not processed by the MarkStep in lot of cases. So this loops basically makes sure that we keep all methods which implement interface methods coming from assemblies which are not "link".

I also debugged the static interface case and that one seems to be a simple case of "forgetting". The Annotations class already handles static interface methods correctly (so the "get bases" and "get overrides" already cover static interface methods as well). The bug is in https://github.com/dotnet/linker/pull/2711/files#diff-f3ab7d627296ba105613b9cc039ca6f4ddc7a9aa66c6060ca82f6456ae0ede4fL2314:
This line only allows virtual methods to be marked, but this also needs to allow static methods through as they may have "base methods" now as well.

src/linker/Linker.Steps/MarkStep.cs

Check for optimization before skipping marking interface methods for PreservedScope Add doc comments

jtschuster · 2022-04-05T16:55:54Z

src/linker/Linker.Steps/MarkStep.cs

-				// if the type is never instantiated, interfaces will be removed
-				if (@base.DeclaringType.IsInterface)
+				// if the type is never instantiated, interfaces will be removed - but only if the optimization is enabled
+				if (@base.DeclaringType.IsInterface && Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, method.DeclaringType))


This check here eliminates the necessity for marking methods in IsVirtualMethodNeededByInstantiatedTypeDueToPreservedScope. Since we don't exit early here when the optimization is disabled, we mark methods where the interface comes from an IgnoreScope assembly.

jtschuster · 2022-04-05T23:02:25Z

Finally got managed to get sizes comparing this branch vs main on runtime that I am fairly confident in.

path	FixLibraryInterfaces Size (bytes)	main Size (bytes)	difference (bytes)
artifacts/bin	4000565435	4000553307	+12128
artifacts/bin/testhost	652157901	652155889	+2012
artifacts/packages/Release	860664283	860665167	-884
artifacts/packages/Release/Shipping/Microsoft.NETCore.App.Runtime.linux-x64.7.0.0-dev.nupkg	31891817	31890389	+ 1428

I have no idea why the packages would be smaller after the fix, and I can drill down into this further, but just not 100% sure which outputs are trimmed or not.

vitek-karas · 2022-04-06T09:36:46Z

The testhost is the interesting measurement (or the nupkg but that's compressed) - so the impact is minimal (+2K). Can you please also measure this on Windows build? (I assume this is a Release build, right?)

vitek-karas · 2022-04-06T09:41:14Z

src/linker/Linker.Steps/MarkStep.cs

@@ -1931,7 +1922,7 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in
 			MarkGenericParameterProvider (type);

 			// There are a number of markings we can defer until later when we know it's possible a reference type could be instantiated
-			// For example, if no instance of a type exist, then we don't need to mark the interfaces on that type
+			// For example, if no instance of a type exist, then we don't need to mark the interfaces on that type -- Note this is not true for static interfaces


While the added comment is correct, I think it makes this whole section confusing. Maybe you should add that the MarkRequiermentsForInstantiatedTypes also works for static interfaces - and maybe we should rename that method then.

vitek-karas · 2022-04-06T09:44:13Z

src/linker/Linker.Steps/MarkStep.cs

 		{
-			if (!method.IsVirtual)
+			if (!(method.IsVirtual || method.IsStatic))


Maybe add a comment that this handles static interface methods (it's not obvious why statics are interesting here otherwise).

vitek-karas · 2022-04-06T09:46:23Z

src/linker/Linker.Steps/MarkStep.cs

+		/// Returns true if any of the base methods of <paramref name="method" /> is defined in an assembly that is not trimmed (i.e. action!=trim)
+		/// </summary>
+		/// <remarks>This is very similar to <see cref="IsVirtualNeededByTypeDueToPreservedScope(MethodDefinition)"/>, except this also checks if the base method is an interface.</remarks>
+		bool IsVirtualNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method)


The new name is confusing - since this method explicitly handles interface methods as well (not just virtuals). I personally don't mind the previous method, it's not super descriptive, but at least it's not wrong.

Actually with the removal of the call to this method from the delayed processing, this method should not handle static interface methods. Now it's only called when the type is instantiated - and in that case only virtual methods are affected. static interface methods must be marked regardless of instantiation.

So the new name seems correct, but the method should not deal with statics at all.

vitek-karas · 2022-04-06T09:56:22Z

src/linker/Linker.Steps/MarkStep.cs

+		/// <summary>
+		/// Returns true if any of the base methods of the <paramref name="method"/> passed is in an assembly that is not trimmed (i.e. action != trim)
+		/// </summary>
+		/// <remarks>This ignores any base methods defined in interfaces. To also check methods defined in interfaces</remarks>
 		bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method)
 		{
 			if (!method.IsVirtual)


I think with the current changes this will have to handle static interface methods as well.

jtschuster · 2022-04-06T22:06:28Z

Can you please also measure this on Windows build? (I assume this is a Release build, right?)

Yes, this is a release build. Here's the results for windows for the artifacts/bin/teshost folder (using the same runtime commit as before: 6ffb7f0)

FixLibraryInterface branch	main branch	difference
248062280	248061288	+992

Rename IsVirtualNeededByInstantiatedTypeDueToPreservedScope to IsOverrideNeededByInstantiatedTypeDueToPreservedScope Added more comments describing purpose of methods Moved the static interface method check to the method that is called on all types regardless of instantiation

jtschuster · 2022-04-06T23:27:27Z

I added some more comments on the methods with my current understanding, please correct me if they're wrong.

This doesn't change product behavior, only renamed methods and improved comments. Added new tests for the RootLibrary: - Added a dependency to an "copy" assembly (mainly because I can define a static interface in it) - Added more combinations to the interfaces/classes in the test - Since this uses static interface methods I had to enable "preview" language features for the test project and for the test infra.

vitek-karas · 2022-04-08T18:52:28Z

Validated this change against the static interface issue found in runtime with Numerics.
dotnet/runtime#67453

With this change the static interface methods are correctly preserved.

vitek-karas · 2022-04-07T11:02:55Z

src/linker/Linker.Steps/MarkStep.cs

 		/// </summary>
-		/// <remarks>This ignores any base methods defined in interfaces. To also check methods defined in interfaces</remarks>
+		/// <remarks>
+		/// When the unusedinterfaces optimization is on, this is used to mark methods that override a virtual from a non-link assembly and must be kept.


Suggested change

/// When the unusedinterfaces optimization is on, this is used to mark methods that override a virtual from a non-link assembly and must be kept.

/// When the unusedinterfaces optimization is on, this is used to mark methods that override an abstract method from a non-link assembly and must be kept.

vitek-karas · 2022-04-07T11:04:35Z

src/linker/Linker.Steps/MarkStep.cs

+		/// When the unusedinterfaces optimization is on, this is used to mark methods that override a virtual from a non-link assembly and must be kept.
+		/// When the unusedinterfaces optimization is off, this will do the same as when on but will also mark interface methods from interfaces defined in a non-link assembly.
+		/// If the containing type is instantiated, the caller should use <see cref="IsOverrideNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition)" />
+		/// </remarks>
 		bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method)


I would rename this to IsMethodNeededByTypeDueToPreservedScope - the caller uses it that way (the caller doesn't filter on virtuals - and it must not anyway).

This will make sure that we keep all interfaces and all interface method implementations on such type. * Change the solution to rely on the optimization setting only * wip * Add implicit interface implementation case * Edit tests to remove issue-specific code and names * Mark members of CollectedType as kept * Add private interface test and simplify external interface example * Replace early exit for non-interfaces * License headers and use IsMethodNeededByTypeDueToPreservedScope instead of Interface specific version * Add check for static methods before skipping virtual marking Check for optimization before skipping marking interface methods for PreservedScope Add doc comments * Add more clarifying comments, move static method check, rename method Rename IsVirtualNeededByInstantiatedTypeDueToPreservedScope to IsOverrideNeededByInstantiatedTypeDueToPreservedScope Added more comments describing purpose of methods Moved the static interface method check to the method that is called on all types regardless of instantiation * Renames and comment cleanup + tests This doesn't change product behavior, only renamed methods and improved comments. Added new tests for the RootLibrary: - Added a dependency to an "copy" assembly (mainly because I can define a static interface in it) - Added more combinations to the interfaces/classes in the test - Since this uses static interface methods I had to enable "preview" language features for the test project and for the test infra. * More tests for interface behavior Co-authored-by: vitek-karas <[email protected]> Co-authored-by: vitek-karas <[email protected]> Commit migrated from dotnet/linker@8c33ad3

vitek-karas and others added 5 commits March 25, 2022 11:47

Consider all public types in a library as instantiated

0388e48

This will make sure that we keep all interfaces and all interface method implementations on such type.

Change the solution to rely on the optimization setting only

39ffb9f

wip

e5b3788

Add implicit interface implementation case

61115f6

Edit tests to remove issue-specific code and names

c4ebd45

jtschuster requested review from agocke, sbomer and vitek-karas March 28, 2022 19:16

jtschuster requested a review from marek-safar as a code owner March 28, 2022 19:16

Mark members of CollectedType as kept

d7cc881

sbomer reviewed Mar 29, 2022

View reviewed changes

marek-safar reviewed Mar 31, 2022

View reviewed changes

jtschuster added 4 commits April 1, 2022 12:41

Add private interface test and simplify external interface example

3a8b76d

Replace early exit for non-interfaces

1c04ad2

Merge branch 'main' into FixLibraryInterface

be37029

License headers and use IsMethodNeededByTypeDueToPreservedScope inste…

7ad5afc

…ad of Interface specific version

vitek-karas reviewed Apr 5, 2022

View reviewed changes

src/linker/Linker.Steps/MarkStep.cs Show resolved Hide resolved

Add check for static methods before skipping virtual marking

07143dd

Check for optimization before skipping marking interface methods for PreservedScope Add doc comments

jtschuster commented Apr 5, 2022

View reviewed changes

vitek-karas reviewed Apr 6, 2022

View reviewed changes

vitek-karas added 2 commits April 7, 2022 07:27

More tests for interface behavior

4ea2991

jtschuster requested a review from vitek-karas April 12, 2022 19:41

vitek-karas approved these changes Apr 12, 2022

View reviewed changes

jtschuster merged commit 8c33ad3 into dotnet:main Apr 12, 2022

dakersnar mentioned this pull request Jun 24, 2022

Incorrect trimming of static properties when DIM is involved #2865

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't remove unused interfaces in library mode #2711

Don't remove unused interfaces in library mode #2711

jtschuster commented Mar 28, 2022

sbomer Mar 29, 2022

vitek-karas Mar 30, 2022

sbomer Mar 30, 2022

sbomer Mar 29, 2022

vitek-karas Mar 30, 2022

vitek-karas commented Mar 30, 2022

vitek-karas commented Mar 30, 2022

marek-safar Mar 31, 2022

vitek-karas commented Apr 5, 2022

jtschuster Apr 5, 2022 •

edited

Loading

jtschuster commented Apr 5, 2022 •

edited

Loading

vitek-karas commented Apr 6, 2022

vitek-karas Apr 6, 2022

vitek-karas Apr 6, 2022

vitek-karas Apr 6, 2022

vitek-karas Apr 6, 2022

vitek-karas Apr 6, 2022

vitek-karas Apr 6, 2022

jtschuster commented Apr 6, 2022

jtschuster commented Apr 6, 2022

vitek-karas commented Apr 8, 2022

vitek-karas Apr 7, 2022

vitek-karas Apr 7, 2022

	/// When the unusedinterfaces optimization is on, this is used to mark methods that override a virtual from a non-link assembly and must be kept.
	/// When the unusedinterfaces optimization is on, this is used to mark methods that override an abstract method from a non-link assembly and must be kept.

Don't remove unused interfaces in library mode #2711

Don't remove unused interfaces in library mode #2711

Conversation

jtschuster commented Mar 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitek-karas commented Mar 30, 2022

vitek-karas commented Mar 30, 2022

Choose a reason for hiding this comment

vitek-karas commented Apr 5, 2022

jtschuster Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

jtschuster commented Apr 5, 2022 • edited Loading

vitek-karas commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtschuster commented Apr 6, 2022

jtschuster commented Apr 6, 2022

vitek-karas commented Apr 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtschuster Apr 5, 2022 •

edited

Loading

jtschuster commented Apr 5, 2022 •

edited

Loading