Unified handling of Resolve calls #1979
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This change introduces a commonplace for resolving definition for types, methods and fields. This is useful for following reasons - Speeds up linker by 20% for default Blazor app (more for large apps) - Any custom step can avoid building local mapping cache - Custom steps could support `--skip-unresolved` linker option - Consistent error handling for unresolved references Most of the changes are just mechanical method replacement and Link context passing. Contributes to dotnet#918 Fixes dotnet#1953
vitek-karas
approved these changes
Apr 20, 2021
Comment on lines
+792
to
+800
| if (typeReference is TypeSpecification ts) { | ||
| if (typeReference is FunctionPointerType) { | ||
| td = null; | ||
| } else { | ||
| // | ||
| // It returns element-type for arrays and also element type for wrapping types like ByReference, PinnedType, etc | ||
| // | ||
| td = TryResolveTypeDefinition (ts.GetElementType ()); | ||
| } |
There was a problem hiding this comment.
Should we create an issue to handle at least some of these at the callsite - resolving to element-type is dangerous. Not counting that it also hides generic instantiations.
There was a problem hiding this comment.
I actually experimented with it a bit and I think this logic is fine. Most code still needs to resolve to the element so the separation would not have benefits beyond perhaps type system purity.
Co-authored-by: Vitek Karas <[email protected]>
vitek-karas
reviewed
Apr 20, 2021
vitek-karas
approved these changes
Apr 20, 2021
This was referenced Apr 27, 2021
MichalStrehovsky
added a commit
to MichalStrehovsky/runtimelab
that referenced
this pull request
Apr 29, 2021
This just gets dotnet/linker#1979 out of the way since it's very noisy and doesn't apply to us.
jkotas
pushed a commit
to dotnet/runtimelab
that referenced
this pull request
Apr 29, 2021
This just gets dotnet/linker#1979 out of the way since it's very noisy and doesn't apply to us.
sbomer
added a commit
to sbomer/linker
that referenced
this pull request
May 19, 2021
This will allow extension methods that take an `IMetadataResolver` like in dotnet/java-interop#842 to use the LinkContext Resolve cache. Context: dotnet/android#5748 (comment) The Resolve cache added in dotnet#1979 requires calling `Resolve*Definition` methods directly on `LinkContext`, which means that any extension methods that do resolution logic need to take a `LinkContext`. This doesn't work well with the layering in xamarin-android, where java.interop uses a resolution cache with cecil, but doesn't depend on the linker. Instead it uses a custom `TypeResolutionCache` for extension methods like `GetBaseDefinition`: https://github.com/xamarin/java.interop/blob/main/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L16 These extension methods are also used from xamarin-android, but there's a desire to use the `LinkContext` cache in this case. @jonpryor had the idea to change the extension methods to use cecil's `IMetadataResolver`, which can be implemented by `TypeDefinitionCache` and by `LinkContext`. Java.interop will continue using their `TypeDefinitionCache`, and xamarin-android will use `LinkContext`. One limitation of this approach is that `LinkContext.TryResolve*Definition` (renamed to just `TryResolve` for consistency) methods aren't usable from the extension methods.
sbomer
added a commit
that referenced
this pull request
May 20, 2021
This will allow extension methods that take an `IMetadataResolver` like in dotnet/java-interop#842 to use the LinkContext Resolve cache. Context: dotnet/android#5748 (comment) The Resolve cache added in #1979 requires calling `Resolve*Definition` methods directly on `LinkContext`, which means that any extension methods that do resolution logic need to take a `LinkContext`. This doesn't work well with the layering in xamarin-android, where java.interop uses a resolution cache with cecil, but doesn't depend on the linker. Instead it uses a custom `TypeResolutionCache` for extension methods like `GetBaseDefinition`: https://github.com/xamarin/java.interop/blob/main/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L16 These extension methods are also used from xamarin-android, but there's a desire to use the `LinkContext` cache in this case. @jonpryor had the idea to change the extension methods to use cecil's `IMetadataResolver`, which can be implemented by `TypeDefinitionCache` and by `LinkContext`. Java.interop will continue using their `TypeDefinitionCache`, and xamarin-android will use `LinkContext`. One limitation of this approach is that `LinkContext.TryResolve*Definition` (renamed to just `TryResolve` for consistency) methods aren't usable from the extension methods.
jonpryor
pushed a commit
to dotnet/android
that referenced
this pull request
Jun 2, 2021
Context: dotnet/linker#1953 Context: dotnet/linker#1774 Context: dotnet/linker#1774 (comment) Context: dotnet/linker#1979 Context: dotnet/linker#2019 Context: dotnet/linker#2045 Context: dotnet/java-interop@2573dc8 Context: #5870 Context: #5878 Update the custom linker steps to use the new `IMarkHandler` API which runs logic during "MarkStep". (See [this list][0] of pipeline steps for additional context.) As part of this, I've removed the workaround that loads all referenced assemblies up-front and simplified some of the linker MSBuild targets. Some of the `MonoDroid.Tuner` steps were duplicated and changed to implement the new `IMarkHandler` interface, to avoid touching the `MonoDroid.Tuner` code. In my analysis of the custom steps, most of them "just work" if we call them only for marked members, because they ultimately either: - Just call `AddPreserved*()` to conditionally keep members of types (which with the new API will just happen when the type is marked) - In the case of `FixAbstractMethodsStep()`, inject missing interface implementations which will only be kept if they are marked for some other reason. Most of the steps have been updated in a straightforward way based on the above. The exceptions are: - `AddKeepAlivesStep` needs to run on all code that survived linking, and can run as a normal step. - `ApplyPreserveAttribute`: this step globally marks members with `PreserveAttribute`. - The step is only active for non-SDK link assemblies. Usually we root non-SDK assemblies, but it will be a problem if linking all assemblies. - For now, I updated it to use the new custom step API on assembly mark -- so it will scan for the attribute in all marked assemblies -- but we should investigate whether this is good enough. - This can't be migrated to use `IMarkHandler` because it needs to scan code for attributes, including unmarked code. - `PreserveExportedTypes`: similar to the above, this globally marks members with `Export*Attribute`. It's used for java interop in cases where the java methods aren't referenced statically, like from xml, or for special serialization methods. - It's only active for non-SDK assemblies, so like above it will be a problem only if linking all assemblies. - Like above, I've made it scan marked assemblies. - `PreserveApplications`: globally scans for `ApplicationAttribute` on types/assemblies, and sets the `TypePreserve` annotation for any types referenced by the attribute. - This step technically does a global scan, but it's likely to work on "mark type"/"mark assembly", as `ApplicationAttribute` is only used on types/assembies that are already kept. - I've updated it to use the new `IMarkHandler` API. Additionally, as per dotnet/java-interop@2573dc8c, stop using `TypeDefinitionCache` everywhere and instead use `IMetadataResolver` to support caching `TypeDefinition`s across shared steps. For .NET 6, `LinkContextMetadataResolver` implements the `IMetadataResolver` interface in terms of `LinkContext`. [0]: https://github.com/mono/linker/blob/main/src/linker/Linker/Driver.cs#L714-L730
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
* Unified handling of Resolve calls This change introduces a commonplace for resolving definition for types, methods and fields. This is useful for following reasons - Speeds up linker by 20% for default Blazor app (more for large apps) - Any custom step can avoid building local mapping cache - Custom steps could support `--skip-unresolved` linker option - Consistent error handling for unresolved references Most of the changes are just mechanical method replacement and Link context passing. Contributes to dotnet/linker#918 Fixes dotnet/linker#1953 * Update src/linker/Linker/LinkContext.cs Co-authored-by: Vitek Karas <[email protected]> * PR feedback * Simplify MethodReturnValue node construction Co-authored-by: Vitek Karas <[email protected]> Commit migrated from dotnet/linker@012efef
agocke
pushed a commit
to dotnet/runtime
that referenced
this pull request
Nov 16, 2022
This will allow extension methods that take an `IMetadataResolver` like in dotnet/java-interop#842 to use the LinkContext Resolve cache. Context: dotnet/android#5748 (comment) The Resolve cache added in dotnet/linker#1979 requires calling `Resolve*Definition` methods directly on `LinkContext`, which means that any extension methods that do resolution logic need to take a `LinkContext`. This doesn't work well with the layering in xamarin-android, where java.interop uses a resolution cache with cecil, but doesn't depend on the linker. Instead it uses a custom `TypeResolutionCache` for extension methods like `GetBaseDefinition`: https://github.com/xamarin/java.interop/blob/main/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs#L16 These extension methods are also used from xamarin-android, but there's a desire to use the `LinkContext` cache in this case. @jonpryor had the idea to change the extension methods to use cecil's `IMetadataResolver`, which can be implemented by `TypeDefinitionCache` and by `LinkContext`. Java.interop will continue using their `TypeDefinitionCache`, and xamarin-android will use `LinkContext`. One limitation of this approach is that `LinkContext.TryResolve*Definition` (renamed to just `TryResolve` for consistency) methods aren't usable from the extension methods. Commit migrated from dotnet/linker@aaf4880
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This change introduces a commonplace for resolving definition for types,
methods and fields. This is useful for following reasons
--skip-unresolvedlinker optionMost of the changes are just mechanical method replacement and
Link context passing.
Contributes to #918
Fixes #1953