Conversation
...Cases/Inheritance.Interfaces/OnReferenceType/InterfaceTypeInOtherUsedOnlyByLinkedAssembly.cs
Outdated
Show resolved
Hide resolved
|
This needs more work. The change https://github.com/mono/linker/pull/657/files#diff-83d9d9aa53da0b0737513a2561f82bc8L2231 seems to be causing perf issues with larger link sets (like the framework). I'm thinking about changing how we mark interfaceimpls to avoid iterating over all types with interfaces (and all impls on used types) in edit: The problem was that re-processing interface impls can re-queue ctors of custom attributes on the impls, resulting in an infinite loop. I've added a more specific check to prevent this re-processing, while still allowing impls that were marked in ResolveFromAssemblyStep to be processed in MarkStep. This does a little extra work compared to previously (since more logic from I think it's outside the scope of this change, but I still wonder whether there's a better way to track |
This adds a cache for interfaces processed in ResolveFromAssemblyStep (one cache per ResolveFromAssemblyStep, so the same interface may be processed multiple times if there are multiple ResolveFromAssemblySteps). Static methods were converted to instance methods to help with this.
MarkStep used to have logic that would only mark an interface impl if it had not been marked previously. Because interface impls can now be marked up-front in ResolveFromAssemblyStep, MarkStep needs to be able to process impls that are already marked. However, when marking custom attributes on the interface impl, we add attribute ctors to the method queue, which can result in an infinite loop. This adds back a check to prevent duplicate processing of interface impls, by checking whether it has been previously processed (instead of marked). It looks like this will skip marking custom attributes on interface impls if the attribute type is marked after the impl is encountered. This seems to be a bug, but it looks like the behavior was present before this change. dotnet#780
e9907c6 to
8fbf255
Compare
To ensure that the referenced assembly still gets placed in the output directory
|
@marek-safar PTAL |
The context needs to continue being passed through the mark methods to support the case where it is passed in from another step in LoadI18nAssemblies.
...Cases/Inheritance.Interfaces/OnReferenceType/InterfaceTypeInOtherUsedOnlyByLinkedAssembly.cs
Outdated
Show resolved
Hide resolved
| bool MarkType (LinkContext context, TypeDefinition type, RootVisibility rootVisibility) | ||
| { | ||
| if (type.IsInterface) { | ||
| if (_markedInterfaces == null) |
There was a problem hiding this comment.
I don't like this logic hear, we are spreading Marking logic into a different step which will be hard to keep in sync
There was a problem hiding this comment.
I think this is easier to maintain than having MarkStep deal with both the public surface area "roots" and the conditional interface marking - see my other comment. LMK what you think.
| if (type.HasNestedTypes) | ||
| foreach (var nested in type.NestedTypes) | ||
| MarkType (context, nested, rootVisibility); | ||
| if (type.HasInterfaces) |
There was a problem hiding this comment.
I get this is the only change from this whole PR but I am not sure this is right. Why do you need to mark the base types explicitly here and not to do it in standard MarkStep?
There was a problem hiding this comment.
MarkStep won't mark the interfaces unless it sees that the type implementing it has been instantiated. We need some way to cause interface impls to be kept if they are considered part of the public surface area. Since we already have some logic in ResolveFromAssemblyStep to deal with marking public surface area based on visibility, I thought it made the most sense to do it here.
There was a problem hiding this comment.
I think we need to think about a different approach. You are trying to disable optimizations we are added and will keep adding to make linking smarter/better. With this approach, it will be constant work of undoing improvements in marking step (e.g. if we introduce base types reduction, or replacing interfaces with objects, etc).
Secondly, even this change is incomplete. What about cases where an interface is used as a constraint, or as attribute argument or typeof expression?
I think it'd be better to have for this mode a new step which replaces ResolveFromAssemblyStep and MarkStep as used now for -r called for example MarkVisibleSurfaceStep
There was a problem hiding this comment.
I'm not sure I understand the first concern - this doesn't disable any of the optimizations and they will still kick in for interface impls that aren't considered public surface area (or for all interface impls when linking apps using -a). For the scenario where we're linking individual corefx libraries, markstep needs to be told what to keep as public surface area.
an interface is used as a constraint, or as attribute argument
Good point, I can work on adding those as long as we agree on the approach.
typeof expression
I'm not sure that would be considered public surface area of a library, thoughts?
a new step which replaces ResolveFromAssemblyStep and MarkStep as used now for -r
To me this seems like it would cause more duplication than the current approach - for example the logic for rooting exported types up-front is shared between -r and -a.
There was a problem hiding this comment.
My point is that when we add more optimizations which change metadata we'll have to undo them here and therefore would be easier to just have a different pipeline from this "mode".
I'm not sure that would be considered public surface area of a library, thoughts?
That's exactly the point. I think nobody knows that right now and it's very hard to get that from the code. You are running linker as IL optimizer which I think needs more thinking about how to approach going forward.
To me this seems like it would cause more duplication
I don't think there will be any duplication, we can refactor any code as we need.
for example the logic for rooting exported types up-front is shared
We should never mark exported types upfront when in linking mode unless they are needed.
I am happy to talk about this in more details.
There was a problem hiding this comment.
I started trying the alternative we discussed in #793.
|
Related to #1610 |
This makes ResolveFromAssemblyStep the source of truth for what is considered "public API" when linking a library, by marking interface implementations up-front. This needs to be done carefully since the implications of marking the interfaceimpl (marking custom attributes on it, and marking the interface type itself) are usually done later, in MarkStep, which has optional optimizations like only marking custom attributes for used attribute types.
If in the future we add features like base type sweeping, similar adjustments will probably need to be made in ResolveFromAssemblyStep to keep base types when marking public surface area.
I think this is almost ready to go - I need to do some more validation for custom attributes on the .interfaceimpls.