[Java.Interop.Tools.*] IMetadataResolver not TypeDefinitionCache #842
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
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
to sbomer/xamarin-android
that referenced
this pull request
May 19, 2021
This implements @jonathanpepper's idea to use the linker's built-in Resolve cache without duplicating the extension methods from java.interop. By overriding TypeDefinitionCache, we can continue using the existing extension methods that take TypeDefinitionCache, and this leaves open the option to modify the extension method signatures to instead take an IMetadataResolver as in dotnet/java-interop#842.
jonathanpeppers
approved these changes
May 19, 2021
jonpryor
force-pushed
the
jonp-typecache-resolver
branch
from
7d15007 to
0c46395
Compare
Context: b81cfbb Context: dotnet/android#5748 Context: dotnet/android#5748 (comment) Commit b81cfbb introduced `TypeDefinitionCache`, which caches `TypeReference.Resolve()` invocations so as to speed things up. Enter dotnet/android#5748: we want to adopt some linker API changes, and mono/linker's [`LinkContext` API][0] *also* has a `TypeDefinition` cache construct. Consequently, to "fully embrace" the new `LinkContext` API changes, *large portions* of `Java.Interop.Tools.Cecil.dll` are copied so that `LinkContext`'s caching can be used instead of `TypeDefinitionCache`'s caching, because mono/linker doesn't use Java.Interop, and thus can't use `TypeDefinitionCache`. Clean this up and split the difference: "duplicate" the APIs in `Java.Interop.Tools.Cecil.dll`, `Java.Interop.Tools.JavaCallableWrappers.dll`, and `src/Java.Interop.Tools.TypeNameMappings` so that instead of optionally using `TypeDefinitionCache`, we instead permit the use of the [`Mono.Cecil.IMetadataResolver` interface][1], which is a "superset" of `TypeDefinitionCache` functionality. Update `TypeDefinitionCache` to implement the `IMetadataResolver` interface, implementing `IMetadataResolver.Resolve()` so that previous caching functionality is preserved. This *should* result in no breakage of existing xamarin-android code, while allowing for a reasonable integration point between `Java.Interop.Tools.Cecil.dll` and mono/linker, by way of `IMetadataResolver`. [0]: https://github.com/mono/linker/blob/30f2498c2a3de1f7e236d5793f5f1aca6e5ba456/src/linker/Linker/LinkContext.cs [1]: https://github.com/mono/cecil/blob/e069cd8d25d5b61b0e28fe65e75959c20af7aa80/Mono.Cecil/MetadataResolver.cs#L22-L26
jonpryor
force-pushed
the
jonp-typecache-resolver
branch
from
0c46395 to
5f8d44d
Compare
sbomer
added a commit
to dotnet/linker
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.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Context: b81cfbb
Context: dotnet/android#5748
Context: dotnet/android#5748 (comment)
Commit b81cfbb introduced
TypeDefinitionCache, which cachesTypeReference.Resolve()invocations so as to speed things up.Enter dotnet/android#5748: we want to adopt some linker API
changes, and mono/linker's
LinkContextAPI also has aTypeDefinitioncache construct.Consequently, to "fully embrace" the new
LinkContextAPI changes,large portions of
Java.Interop.Tools.Cecil.dllare copied so thatLinkContext's caching can be used instead ofTypeDefinitionCache'scaching, because mono/linker doesn't use Java.Interop, and thus
can't use
TypeDefinitionCache.Clean this up and split the difference: "duplicate" the APIs in
Java.Interop.Tools.Cecil.dll,Java.Interop.Tools.JavaCallableWrappers.dll, andsrc/Java.Interop.Tools.TypeNameMappingsso that instead ofoptionally using
TypeDefinitionCache, we instead permit the useof the
Mono.Cecil.IMetadataResolverinterface, which is a"superset" of
TypeDefinitionCachefunctionality.Update
TypeDefinitionCacheimplement theIMetadataResolverinterface, implementing
IMetadataResolver.Resolver()so thatprevious caching functionality is preserved.
This should result in no breakage of existing xamarin-android code,
while allowing for a reasonable integration point between
Java.Interop.Tools.Cecil.dlland mono/linker, by way ofIMetadataResolver.