Skip to content

[Java.Interop.Tools.JavaCallableWrappers] fix more places to use TypeDefinitionCache #1069

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 7, 2023

Conversation

jonathanpeppers
Copy link
Member

Reviewing code in JavaCallableWrapperGenerator, I found places we weren't using the supplied TypeDefinitionCache or IMetadataResolver. Thus we appeared to be calling TypeReference.Resolve() potentially on the same types.

For example, dotnet trace output of an incremental build of a dotnet new maui project:

41.65ms xamarin.android.cecil!Mono.Cecil.TypeReference.Resolve()

I created a new TypeDefinitionRocks.ResolveCached() method to be used everywhere instead. Resulting with:

23.89ms xamarin.android.cecil!Mono.Cecil.TypeReference.Resolve()

Additionally, I updated to places to use plain foreach loops instead of System.Linq.

Before:
1.03s xamarin.android.build.tasks!Xamarin.Android.Tasks.GenerateJavaStubs.RunTask()
After:
944.48ms xamarin.android.build.tasks!Xamarin.Android.Tasks.GenerateJavaStubs.RunTask()

I think this likely saves about ~50ms off incremental builds of a dotnet new maui project.

…DefinitionCache

Reviewing code in `JavaCallableWrapperGenerator`, I found places we
weren't using the supplied `TypeDefinitionCache` or
`IMetadataResolver`. Thus we appeared to be calling
`TypeReference.Resolve()` potentially on the same types.

For example, `dotnet trace` output of an incremental build of a
`dotnet new maui` project:

    41.65ms xamarin.android.cecil!Mono.Cecil.TypeReference.Resolve()

I created a new `TypeDefinitionRocks.ResolveCached()` method to be
used everywhere instead. Resulting with:

    23.89ms xamarin.android.cecil!Mono.Cecil.TypeReference.Resolve()

Additionally, I updated to places to use plain `foreach` loops instead
of System.Linq.

    Before:
    1.03s xamarin.android.build.tasks!Xamarin.Android.Tasks.GenerateJavaStubs.RunTask()
    After:
    944.48ms xamarin.android.build.tasks!Xamarin.Android.Tasks.GenerateJavaStubs.RunTask()

I think this likely saves about ~50ms off incremental builds of a
`dotnet new maui` project.
@jonpryor
Copy link
Contributor

jonpryor commented Jan 4, 2023

At what point do we decide that IMetadataResolver is required and not optional? I feel like we're playing "wack-a-mole" with callsites which should be providing a cache, but aren't, because we overlook them.

I think we should thus require IMetadataResolver instances, i.e. IMetadataResolver resolver instead of IMetadataResolver? resolver, and fix the fallout.

@jonpryor
Copy link
Contributor

jonpryor commented Jan 4, 2023

@jonpryor wrote:

I think we should thus require IMetadataResolver instances, i.e. IMetadataResolver resolver instead of IMetadataResolver? resolver, and fix the fallout.

…and yes, this would be a partial "API break", in that currently non-warning code may start producing warnings, and /warnaserror would produce errors. That's the point. We should now require the caches, because we know it results in significant performance improvements.

This raises followup questions, such as what to do with our existing "compatibility overloads" such as https://github.com/xamarin/java.interop/blob/f8d77faf55347a58030a694332ba97f0dee88246/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs#L10-L12

We can't remove them; that would be an ABI break.

We should instead update them to be be an error to use:

		[Obsolete ("Use the TypeDefinitionCache overload for better performance.", error:true)]
		public static TypeDefinition? GetBaseType (this TypeDefinition type) =>
			throw new NotSupportedException();

Any place we had:

    TypeDefinitionCache?
    IMetadataResolver?

These no longer allow nulls, and so overloads that use `Obsolete` now
emit errors instead of warnings:

    [Obsolete ("Use the TypeDefinitionCache overload for better performance.", error: true)]
    public static MethodDefinition GetBaseDefinition (this MethodDefinition method) =>
        GetBaseDefinition (method, resolver: null!);

This results in 3 compiler errors in xamarin-android:

    src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(65,11): error CS0619: 'TypeDefinitionRocks.IsSubclassOf(TypeDefinition, string)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
    src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(268,12): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
    src\Xamarin.Android.Build.Tasks\Utilities\ManifestDocumentElement.cs(28,11): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'

After these changes, it appears we will improve the performance
further of apps that use:

* `ApplicationAttribute.BackupAgent`
* `ApplicationAttribute.Name`
* `AndroidManifest.xml` attributes that take a `System.Type` values

The full scope of changes required in xamarin/xamarin-android will be:

dotnet/android@main...jonathanpeppers:JavaInteropBreakingChanges

Additionally, this found a couple places in `generator` where changes
were needed.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Jan 4, 2023
Context: dotnet/java-interop#1069

Any place we had:

    TypeDefinitionCache?
    IMetadataResolver?

These no longer allow nulls, and so overloads that use `Obsolete` now
emit errors instead of warnings:

    [Obsolete ("Use the TypeDefinitionCache overload for better performance.", error: true)]
    public static MethodDefinition GetBaseDefinition (this MethodDefinition method) =>
        GetBaseDefinition (method, resolver: null!);

This results in 3 compiler errors in xamarin-android:

    src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(65,11): error CS0619: 'TypeDefinitionRocks.IsSubclassOf(TypeDefinition, string)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
    src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(268,12): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
    src\Xamarin.Android.Build.Tasks\Utilities\ManifestDocumentElement.cs(28,11): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'

After these changes, it appears we will improve the performance
further of apps that use:

* `ApplicationAttribute.BackupAgent`
* `ApplicationAttribute.Name`
* `AndroidManifest.xml` attributes that take a `System.Type` values
@jonpryor
Copy link
Contributor

jonpryor commented Jan 5, 2023

Draft commit message:

Context: b81cfbb9ab8efa647a3bfcc2b2b97a5f1b1fa71e

Reviewing code in `JavaCallableWrapperGenerator`, I found codepaths
where we weren't caching `TypeReference.Resolve()` calls *at all*,
e.g. `JavaNativeTypeManager.GetPrimitiveClass(TypeDefinition)`.
We thus appear to be calling `TypeReference.Resolve()` potentially on
the same types.

For example, `dotnet trace` output of an incremental build of a `dotnet new maui` project:

	41.65ms xamarin.android.cecil!Mono.Cecil.TypeReference.Resolve()

It appears that, in trying to make our maintenance lives easier by
preserving backward API compatibility with callers that couldn't
provide a `TypeDefinitionCache` or `IMetadataResolver` instance -- by
providing method overloads which took e.g.
`IMetadataResolver? resolver` parameters which could be `null` -- we
made our optimization and performance lives *harder*, because not
all codepaths which needed to use caching *were* using caching, as
they were overlooked.

As the `Java.Interop.Tools.*` assemblies are internal API, introduce
an *API break* while preserving *ABI*:

`IMetadataResolver` is now required.

Thus, previous/current "compatibility methods" which allow *not*
providing an `IMetadataResolver` instance such as:

	// old and busted
	partial class TypeDefinitionRocks {
	    [Obsolete ("Use the TypeDefinitionCache overload for better performance.")] 
	    public static TypeDefinition? GetBaseType (this TypeDefinition type) => GetBaseType (type, resolver: null); 
	}

will now become *errors* to use:

	// new hawtness
	partial class TypeDefinitionRocks {
	    [Obsolete ("Use the TypeDefinitionCache overload for better performance.", error:true)] 
	    public static TypeDefinition? GetBaseType (this TypeDefinition type) => throw new NotSupportedException ();
	}

This allows the C# compiler to help us audit our codebase, ensuring
that *all* codepaths which call `TypeReference.Resolve()` instead use
`IMetadataResolver.Resolve()`, including:

  * `JavaCallableWrapperGenerator.GetAnnotationsString()`
  * `JavaCallableWrapperGenerator.WriteAnnotations()`
  * `JavaNativeTypeManager.GetPrimitiveClass()`

Requiring caching results in:

	23.89ms xamarin.android.cecil!Mono.Cecil.TypeReference.Resolve()

Additionally, I updated two places to use plain `foreach` loops
instead of using System.Linq.

  * Before:

        1.03s xamarin.android.build.tasks!Xamarin.Android.Tasks.GenerateJavaStubs.RunTask()

  * After:

        944.48ms xamarin.android.build.tasks!Xamarin.Android.Tasks.GenerateJavaStubs.RunTask()

I think this likely saves about ~50ms off incremental builds of a
`dotnet new maui` project.

@jonpryor jonpryor merged commit cf80deb into dotnet:main Jan 7, 2023
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Jan 9, 2023
Context: dotnet/java-interop#1069

Any place we had:

    TypeDefinitionCache?
    IMetadataResolver?

These no longer allow nulls, and so overloads that use `Obsolete` now
emit errors instead of warnings:

    [Obsolete ("Use the TypeDefinitionCache overload for better performance.", error: true)]
    public static MethodDefinition GetBaseDefinition (this MethodDefinition method) =>
        GetBaseDefinition (method, resolver: null!);

This results in 3 compiler errors in xamarin-android:

    src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(65,11): error CS0619: 'TypeDefinitionRocks.IsSubclassOf(TypeDefinition, string)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
    src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(268,12): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
    src\Xamarin.Android.Build.Tasks\Utilities\ManifestDocumentElement.cs(28,11): error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'

After these changes, it appears we will improve the performance
further of apps that use:

* `ApplicationAttribute.BackupAgent`
* `ApplicationAttribute.Name`
* `AndroidManifest.xml` attributes that take a `System.Type` values

Additionally, fix NRE in the linker:

    Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
    at Java.Interop.Tools.Cecil.TypeDefinitionRocks.GetBaseType(TypeDefinition type, IMetadataResolver resolver) in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 21
    at Java.Interop.Tools.Cecil.TypeDefinitionRocks.GetTypeAndBaseTypes(TypeDefinition type, IMetadataResolver resolver)+MoveNext() in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 36
    at Java.Interop.Tools.Cecil.TypeDefinitionRocks.IsSubclassOf(TypeDefinition type, String typeName, IMetadataResolver resolver) in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 87
    at MonoDroid.Tuner.FixAbstractMethodsStep.ProcessType(TypeDefinition type) in /Users/builder/azdo/_work/1/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs:line 81
    at Mono.Linker.Steps.MarkStep.MarkType(TypeReference reference, DependencyInfo reason, Nullable`1 origin)
    at Mono.Linker.Steps.MarkStep.MarkField(FieldDefinition field, DependencyInfo& reason, MessageOrigin& origin)
    at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
    at Mono.Linker.Steps.MarkStep.MarkEntireAssembly(AssemblyDefinition assembly)
    at Mono.Linker.Steps.MarkStep.MarkAssembly(AssemblyDefinition assembly, DependencyInfo reason)
    at Mono.Linker.Steps.MarkStep.MarkModule(ModuleDefinition module, DependencyInfo reason)
    at Mono.Linker.Steps.MarkStep.ProcessMarkedPending()
    at Mono.Linker.Steps.MarkStep.Initialize()
    at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
    at Mono.Linker.Pipeline.Process(LinkContext context)
    at Mono.Linker.Driver.Run(ILogger customLogger)
    at Mono.Linker.Driver.Main(String[] args)

This was caused by removing null checks in Java.Interop.
@jonathanpeppers jonathanpeppers deleted the MoreTypeDefinitionCache branch January 9, 2023 14:52
jonpryor pushed a commit to dotnet/android that referenced this pull request Jan 9, 2023
Changes: dotnet/java-interop@f8d77fa...cf80deb

  * dotnet/java-interop@cf80deb7: [Java.Interop.Tools.JavaCallableWrappers] use IMetadataResolver more (dotnet/java-interop#1069)
  * dotnet/java-interop@5c5dc086: [generator] enum map.csv can set `@deprecated-since` for enum values (dotnet/java-interop#1070)

Any place we used:

  * `TypeDefinitionCache?`
  * `IMetadataResolver?`

As of dotnet/java-interop@cf80deb7, Java.Interop no longer allows
`null` values for these parameters, so overloads which were]
`[Obsolete]` now emit errors instead of warnings:

	[Obsolete ("Use the TypeDefinitionCache overload for better performance.", error: true)]
	public static MethodDefinition GetBaseDefinition (this MethodDefinition method) =>
	    GetBaseDefinition (method, resolver: null!);

This results in 3 compiler errors in xamarin-android:

	src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(65,11):
	  error CS0619: 'TypeDefinitionRocks.IsSubclassOf(TypeDefinition, string)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
	src\Xamarin.Android.Build.Tasks\Mono.Android\ApplicationAttribute.Partial.cs(268,12):
	  error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'
	src\Xamarin.Android.Build.Tasks\Utilities\ManifestDocumentElement.cs(28,11):
	  error CS0619: 'JavaNativeTypeManager.ToJniName(TypeDefinition)' is obsolete: 'Use the TypeDefinitionCache overload for better performance.'

After these changes, it appears we will improve the performance
further of apps that use:

  * `ApplicationAttribute.BackupAgent`
  * `ApplicationAttribute.Name`
  * `AndroidManifest.xml` attributes that take a `System.Type` values

Additionally, fix a `NullReferenceException` in the linker:

	Unhandled exception. System.NullReferenceException: Object reference not set to an instance of an object.
	   at Java.Interop.Tools.Cecil.TypeDefinitionRocks.GetBaseType(TypeDefinition type, IMetadataResolver resolver) in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 21
	   at Java.Interop.Tools.Cecil.TypeDefinitionRocks.GetTypeAndBaseTypes(TypeDefinition type, IMetadataResolver resolver)+MoveNext() in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 36
	   at Java.Interop.Tools.Cecil.TypeDefinitionRocks.IsSubclassOf(TypeDefinition type, String typeName, IMetadataResolver resolver) in /Users/builder/azdo/_work/1/s/xamarin-android/external/Java.Interop/src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs:line 87
	   at MonoDroid.Tuner.FixAbstractMethodsStep.ProcessType(TypeDefinition type) in /Users/builder/azdo/_work/1/s/xamarin-android/src/Xamarin.Android.Build.Tasks/Linker/MonoDroid.Tuner/FixAbstractMethodsStep.cs:line 81
	   at Mono.Linker.Steps.MarkStep.MarkType(TypeReference reference, DependencyInfo reason, Nullable`1 origin)
	   at Mono.Linker.Steps.MarkStep.MarkField(FieldDefinition field, DependencyInfo& reason, MessageOrigin& origin)
	   at Mono.Linker.Steps.MarkStep.MarkEntireType(TypeDefinition type, DependencyInfo& reason)
	   at Mono.Linker.Steps.MarkStep.MarkEntireAssembly(AssemblyDefinition assembly)
	   at Mono.Linker.Steps.MarkStep.MarkAssembly(AssemblyDefinition assembly, DependencyInfo reason)
	   at Mono.Linker.Steps.MarkStep.MarkModule(ModuleDefinition module, DependencyInfo reason)
	   at Mono.Linker.Steps.MarkStep.ProcessMarkedPending()
	   at Mono.Linker.Steps.MarkStep.Initialize()
	   at Mono.Linker.Steps.MarkStep.Process(LinkContext context)
	   at Mono.Linker.Pipeline.Process(LinkContext context)
	   at Mono.Linker.Driver.Run(ILogger customLogger)
	   at Mono.Linker.Driver.Main(String[] args)

This was caused by removing `null` checks in Java.Interop.

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Apr 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants