Skip to content
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

[Xamarin.Android.Build.Tasks] use Java.Interop's TypeDefinitionCache #4268

Merged
merged 1 commit into from
Feb 22, 2020

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Feb 14, 2020

I was profiling builds with the Mono profiler and noticed:

Method call summary
Total(ms) Self(ms)      Calls Method name
    70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

Almost 90K calls? What is that coming from???

61422 calls from:
    Java.Interop.Tools.Cecil.TypeDefinitionRocks/<GetTypeAndBaseTypes>d__1:MoveNext ()
    Java.Interop.Tools.Cecil.TypeDefinitionRocks:GetBaseType (Mono.Cecil.TypeDefinition)
    Mono.Cecil.TypeReference:Resolve ()
    Mono.Cecil.ModuleDefinition:Resolve (Mono.Cecil.TypeReference)
    Mono.Cecil.MetadataResolver:Resolve (Mono.Cecil.TypeReference)
    Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference)

Ok, this jogged my memory. @StephaneDelcroix had mentioned one of the
big wins for XamlC in Xamarin.Forms was to cache any time
TypeReference.Resolve() was called:

https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/TypeReferenceExtensions.cs#L437-L443

XamlC was able to use static here, because it's using a feature of
MSBuild to run in a separate AppDomain:

https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/XamlTask.cs#L20-L21

However, I think we can simply add a new non-static
TypeDefinitionCache class that would allow callers to control the
caching strategy. Callers will need to control the scope of the
TypeDefinitionCache so it matches any DirectoryAssemblyResolver
being used. Right now most Xamarin.Android builds will open assemblies
with Mono.Cecil twice: once for <GenerateJavaStubs/> and once for
the linker.

So for example, we can add caching in an API-compatible way:

[Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
public static TypeDefinition GetBaseType (this TypeDefinition type) =>
    GetBaseType (type, cache: null);

public static TypeDefinition GetBaseType (this TypeDefinition type, TypeDefinitionCache cache)
{
    if (bt == null)
        return null;
    if (cache != null)
        return cache.Resolve (bt);
    return bt.Resolve ();
}

We just need to ensure cache: null is valid. I took this approach
for any public APIs and made any private or internal APIs
require a TypeDefinitionCache.

I fixed every instance where [Obsolete] would cause a warning here
in Java.Interop. We'll probably see a small improvement in generator
and jnimarshalmethod-gen.

Here in xamarin-android, I fixed all the [Obsolete] warnings
that were called in <GenerateJavaStubs/>. I can make more changes
for the linker in a future PR.

Results

The reduced calls to DirectoryAssemblyResolver.Resolve:

Method call summary
Total(ms) Self(ms)      Calls Method name
Before;
    70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)
After:
    68830       35      26315 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

~63,398 less calls.

In a build of the Xamarin.Forms integration project on macOS / Mono:

Before:
1365 ms  GenerateJavaStubs                          1 calls
After:
 862 ms  GenerateJavaStubs                          1 calls

It is almost a 40% improvement, around ~500ms better.

@jonathanpeppers jonathanpeppers changed the title [Xamarin.Android.Build.Tasks] use Java.Interop's TypeResolverCache [Xamarin.Android.Build.Tasks] use Java.Interop's TypeDefinitionCache Feb 19, 2020
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Feb 20, 2020
Context: dotnet/android#4268

I was profiling builds with the Mono profiler and noticed:

	Method call summary
	Total(ms) Self(ms)      Calls Method name
	    70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

Almost 90K calls to `DirectoryAssemblyResolver.Resolve()`?!
Where is that coming from???

	61422 calls from:
	    Java.Interop.Tools.Cecil.TypeDefinitionRocks/<GetTypeAndBaseTypes>d__1:MoveNext ()
	    Java.Interop.Tools.Cecil.TypeDefinitionRocks:GetBaseType (Mono.Cecil.TypeDefinition)
	    Mono.Cecil.TypeReference:Resolve ()
	    Mono.Cecil.ModuleDefinition:Resolve (Mono.Cecil.TypeReference)
	    Mono.Cecil.MetadataResolver:Resolve (Mono.Cecil.TypeReference)
	    Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference)

OK, this jogged my memory.  @StephaneDelcroix had mentioned one of
the big wins for the `<XamlC/>` task within Xamarin.Forms was to
cache any time `TypeReference.Resolve()` was called:

	https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/TypeReferenceExtensions.cs#L437-L443

`<XamlC/>` was able to use `static` here, because it's using a
feature of MSBuild to run in a separate `AppDomain`:

	https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/XamlTask.cs#L20-L21

However, I think we can simply add a new `TypeDefinitionCache` class
that would allow callers to control the caching strategy.  Callers
will need to control the scope of the `TypeDefinitionCache` so it
matches any `DirectoryAssemblyResolver` being used.  Right now most
Xamarin.Android builds will open assemblies with Mono.Cecil twice:
once for `<GenerateJavaStubs/>` and once for the linker.

We can add caching in an API-compatible way:

	[Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
	public static TypeDefinition GetBaseType (this TypeDefinition type) =>
	    GetBaseType (type, cache: null);

	public static TypeDefinition GetBaseType (this TypeDefinition type, TypeDefinitionCache cache)
	{
	    if (bt == null)
	        return null;
	    if (cache != null)
	        return cache.Resolve (bt);
	    return bt.Resolve ();
	}

We just need to ensure `cache: null` is valid.  I took this approach
for any `public` APIs and made any `private` or `internal` APIs
*require* a `TypeDefinitionCache`.

I fixed every instance where `[Obsolete]` would cause a warning here
in Java.Interop.  We'll probably see a small improvement within
`generator` and `jnimarshalmethod-gen`.

Downstream in xamarin-android, in dotnet/android#4268 I
fixed all the `[Obsolete]` warnings that were present in
`<GenerateJavaStubs/>`.  I can make more changes for the linker in
a future PR.

~~ Results ~~

The reduced calls to `DirectoryAssemblyResolver.Resolve()`:

  * Before:

        Method call summary
        Total(ms) Self(ms)      Calls Method name
            70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

  * After:

        Method call summary
        Total(ms) Self(ms)      Calls Method name
            68830       35      26315 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

~63,398 less calls.

In a build of the Xamarin.Forms integration project on macOS / Mono:

  * Before:

        1365 ms  GenerateJavaStubs                          1 calls

  * After:

         862 ms  GenerateJavaStubs                          1 calls

It is almost a 40% improvement, around ~500ms better.
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Feb 21, 2020
Context: dotnet/android#4268

I was profiling builds with the Mono profiler and noticed:

	Method call summary
	Total(ms) Self(ms)      Calls Method name
	    70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

Almost 90K calls to `DirectoryAssemblyResolver.Resolve()`?!
Where is that coming from???

	61422 calls from:
	    Java.Interop.Tools.Cecil.TypeDefinitionRocks/<GetTypeAndBaseTypes>d__1:MoveNext ()
	    Java.Interop.Tools.Cecil.TypeDefinitionRocks:GetBaseType (Mono.Cecil.TypeDefinition)
	    Mono.Cecil.TypeReference:Resolve ()
	    Mono.Cecil.ModuleDefinition:Resolve (Mono.Cecil.TypeReference)
	    Mono.Cecil.MetadataResolver:Resolve (Mono.Cecil.TypeReference)
	    Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference)

OK, this jogged my memory.  @StephaneDelcroix had mentioned one of
the big wins for the `<XamlC/>` task within Xamarin.Forms was to
cache any time `TypeReference.Resolve()` was called:

	https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/TypeReferenceExtensions.cs#L437-L443

`<XamlC/>` was able to use `static` here, because it's using a
feature of MSBuild to run in a separate `AppDomain`:

	https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/XamlTask.cs#L20-L21

However, I think we can simply add a new `TypeDefinitionCache` class
that would allow callers to control the caching strategy.  Callers
will need to control the scope of the `TypeDefinitionCache` so it
matches any `DirectoryAssemblyResolver` being used.  Right now most
Xamarin.Android builds will open assemblies with Mono.Cecil twice:
once for `<GenerateJavaStubs/>` and once for the linker.

We can add caching in an API-compatible way:

	[Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
	public static TypeDefinition GetBaseType (this TypeDefinition type) =>
	    GetBaseType (type, cache: null);

	public static TypeDefinition GetBaseType (this TypeDefinition type, TypeDefinitionCache cache)
	{
	    if (bt == null)
	        return null;
	    if (cache != null)
	        return cache.Resolve (bt);
	    return bt.Resolve ();
	}

We just need to ensure `cache: null` is valid.  I took this approach
for any `public` APIs and made any `private` or `internal` APIs
*require* a `TypeDefinitionCache`.

I fixed every instance where `[Obsolete]` would cause a warning here
in Java.Interop.  We'll probably see a small improvement within
`generator` and `jnimarshalmethod-gen`.

Downstream in xamarin-android, in dotnet/android#4268 I
fixed all the `[Obsolete]` warnings that were present in
`<GenerateJavaStubs/>`.  I can make more changes for the linker in
a future PR.

~~ Results ~~

The reduced calls to `DirectoryAssemblyResolver.Resolve()`:

  * Before:

        Method call summary
        Total(ms) Self(ms)      Calls Method name
            70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

  * After:

        Method call summary
        Total(ms) Self(ms)      Calls Method name
            68830       35      26315 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

~63,398 less calls.

In a build of the Xamarin.Forms integration project on macOS / Mono:

  * Before:

        1365 ms  GenerateJavaStubs                          1 calls

  * After:

         862 ms  GenerateJavaStubs                          1 calls

It is almost a 40% improvement, around ~500ms better.
I was profiling builds with the Mono profiler and noticed:

    Method call summary
    Total(ms) Self(ms)      Calls Method name
        70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

Almost 90K calls? What is that coming from???

    61422 calls from:
        Java.Interop.Tools.Cecil.TypeDefinitionRocks/<GetTypeAndBaseTypes>d__1:MoveNext ()
        Java.Interop.Tools.Cecil.TypeDefinitionRocks:GetBaseType (Mono.Cecil.TypeDefinition)
        Mono.Cecil.TypeReference:Resolve ()
        Mono.Cecil.ModuleDefinition:Resolve (Mono.Cecil.TypeReference)
        Mono.Cecil.MetadataResolver:Resolve (Mono.Cecil.TypeReference)
        Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference)

Ok, this jogged my memory. Stephane Delcroix had mentioned one of the
big wins for XamlC in Xamarin.Forms was to cache any time
`TypeReference.Resolve()` was called:

https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/TypeReferenceExtensions.cs#L437-L443

XamlC was able to use `static` here, because it's using a feature of
MSBuild to run in a separate `AppDomain`:

https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/XamlTask.cs#L20-L21

However, I think we can simply add a new non-static
`TypeDefinitionCache` class that would allow callers to control the
caching strategy. Callers will need to control the scope of the
`TypeDefinitionCache` so it matches any `DirectoryAssemblyResolver`
being used. Right now most Xamarin.Android builds will open assemblies
with Mono.Cecil twice: once for `<GenerateJavaStubs/>` and once for
the linker.

So for example, we can add caching in an API-compatible way:

    [Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
    public static TypeDefinition GetBaseType (this TypeDefinition type) =>
        GetBaseType (type, cache: null);

    public static TypeDefinition GetBaseType (this TypeDefinition type, TypeDefinitionCache cache)
    {
        if (bt == null)
            return null;
        if (cache != null)
            return cache.Resolve (bt);
        return bt.Resolve ();
    }

We just need to ensure `cache: null` is valid. I took this approach
for any `public` APIs and made any `private` or `internal` APIs
*require* a `TypeDefinitionCache`.

I fixed every instance where `[Obsolete]` would cause a warning here
in Java.Interop. We'll probably see a small improvement in `generator`
and `jnimarshalmethod-gen`.

Here in xamarin-android, I fixed all the `[Obsolete]` warnings
that were called in `<GenerateJavaStubs/>`. I can make more changes
for the linker in a future PR.

~~ Results ~~

The reduced calls to `DirectoryAssemblyResolver.Resolve`:

    Method call summary
    Total(ms) Self(ms)      Calls Method name
    Before;
        70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)
    After:
        68830       35      26315 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

~63,398 less calls.

In a build of the Xamarin.Forms integration project on macOS / Mono:

    Before:
    1365 ms  GenerateJavaStubs                          1 calls
    After:
     862 ms  GenerateJavaStubs                          1 calls

It is almost a 40% improvement, around ~500ms better.
@jonathanpeppers jonathanpeppers marked this pull request as ready for review February 21, 2020 21:12
@jonpryor jonpryor merged commit aff3b52 into master Feb 22, 2020
@jonathanpeppers jonathanpeppers deleted the typeresolvercache branch February 22, 2020 13:54
jonpryor pushed a commit that referenced this pull request Feb 24, 2020
…4268)

Context: dotnet/java-interop@b81cfbb

I was profiling builds with the Mono profiler and noticed:

	Method call summary
	Total(ms) Self(ms)      Calls Method name
	    70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

Almost 90K calls to `DirectoryAssemblyResolver.Resolve()`?!
Where is that coming from???

	61422 calls from:
	    Java.Interop.Tools.Cecil.TypeDefinitionRocks/<GetTypeAndBaseTypes>d__1:MoveNext ()
	    Java.Interop.Tools.Cecil.TypeDefinitionRocks:GetBaseType (Mono.Cecil.TypeDefinition)
	    Mono.Cecil.TypeReference:Resolve ()
	    Mono.Cecil.ModuleDefinition:Resolve (Mono.Cecil.TypeReference)
	    Mono.Cecil.MetadataResolver:Resolve (Mono.Cecil.TypeReference)
	    Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference)

OK, this jogged my memory.  @StephaneDelcroix had mentioned one of
the big wins for the `<XamlC/>` task within Xamarin.Forms was to
cache any time `TypeReference.Resolve()` was called:

	https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/TypeReferenceExtensions.cs#L437-L443

`<XamlC/>` was able to use `static` here, because it's using a
feature of MSBuild to run in a separate `AppDomain`:

	https://github.com/xamarin/Xamarin.Forms/blob/1b9c22b4b9b1c1354a3a5c35ad445a2738c6f6c3/Xamarin.Forms.Build.Tasks/XamlTask.cs#L20-L21

However, I think we can simply add a new `TypeDefinitionCache` class
that would allow callers to control the caching strategy.  Callers
will need to control the scope of the `TypeDefinitionCache` so it
matches any `DirectoryAssemblyResolver` being used.  Right now most
Xamarin.Android builds will open assemblies with Mono.Cecil twice:
once for `<GenerateJavaStubs/>` and once for the linker.

We can add caching in an API-compatible way:

	[Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
	public static TypeDefinition GetBaseType (this TypeDefinition type) =>
	    GetBaseType (type, cache: null);

	public static TypeDefinition GetBaseType (this TypeDefinition type, TypeDefinitionCache cache)
	{
	    if (bt == null)
	        return null;
	    if (cache != null)
	        return cache.Resolve (bt);
	    return bt.Resolve ();
	}

We just need to ensure `cache: null` is valid.  I took this approach
for any `public` APIs and made any `private` or `internal` APIs
*require* a `TypeDefinitionCache`.

I fixed every instance where `[Obsolete]` would cause a warning here
in Java.Interop.  We'll probably see a small improvement within
`generator` and `jnimarshalmethod-gen`.

Here in xamarin-android, I fixed all the `[Obsolete]` warnings
that were called in `<GenerateJavaStubs/>`.  I can make more changes
for the linker in a future PR.

~~ Results ~~

The reduced calls to `DirectoryAssemblyResolver.Resolve()`:

  * Before:

        Method call summary
        Total(ms) Self(ms)      Calls Method name
            70862       97      89713 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

  * After:

        Method call summary
        Total(ms) Self(ms)      Calls Method name
            68830       35      26315 Java.Interop.Tools.Cecil.DirectoryAssemblyResolver:Resolve (Mono.Cecil.AssemblyNameReference,Mono.Cecil.ReaderParameters)

~63,398 less calls.

In a build of the Xamarin.Forms integration project on macOS / Mono:

  * Before:

        1365 ms  GenerateJavaStubs                          1 calls

  * After:

         862 ms  GenerateJavaStubs                          1 calls

It is almost a 40% improvement, around ~500ms better.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 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.

3 participants