Skip to content

Commit

Permalink
[Java.Interop.Tools.Cecil] cache TypeReference.Resolve() calls (dotne…
Browse files Browse the repository at this point in the history 
…t#570)

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.
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Feb 20, 2020
1 parent 95fd014 commit b81cfbb
Show file tree
Hide file tree
Showing 13 changed files with 289 additions and 166 deletions.
57 changes: 32 additions & 25 deletions src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/MethodDefinitionRocks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,37 +9,49 @@ namespace Java.Interop.Tools.Cecil {

public static class MethodDefinitionRocks
{
public static MethodDefinition GetBaseDefinition (this MethodDefinition method)
[Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
public static MethodDefinition GetBaseDefinition (this MethodDefinition method) =>
GetBaseDefinition (method, cache: null);

public static MethodDefinition GetBaseDefinition (this MethodDefinition method, TypeDefinitionCache cache)
{
if (method.IsStatic || method.IsNewSlot || !method.IsVirtual)
return method;

foreach (var baseType in method.DeclaringType.GetBaseTypes ()) {
foreach (var baseType in method.DeclaringType.GetBaseTypes (cache)) {
foreach (var m in baseType.Methods) {
if (!m.IsConstructor &&
m.Name == method.Name &&
(m.IsVirtual || m.IsAbstract) &&
AreParametersCompatibleWith (m.Parameters, method.Parameters)) {
AreParametersCompatibleWith (m.Parameters, method.Parameters, cache)) {
return m;
}
}
}
return method;
}

public static IEnumerable<MethodDefinition> GetOverriddenMethods (MethodDefinition method, bool inherit)
[Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
public static IEnumerable<MethodDefinition> GetOverriddenMethods (MethodDefinition method, bool inherit) =>
GetOverriddenMethods (method, inherit, cache: null);

public static IEnumerable<MethodDefinition> GetOverriddenMethods (MethodDefinition method, bool inherit, TypeDefinitionCache cache)
{
yield return method;
if (inherit) {
MethodDefinition baseMethod = method;
while ((baseMethod = method.GetBaseDefinition ()) != null && baseMethod != method) {
while ((baseMethod = method.GetBaseDefinition (cache)) != null && baseMethod != method) {
yield return method;
method = baseMethod;
}
}
}

public static bool AreParametersCompatibleWith (this Collection<ParameterDefinition> a, Collection<ParameterDefinition> b)
[Obsolete ("Use the TypeDefinitionCache overload for better performance.")]
public static bool AreParametersCompatibleWith (this Collection<ParameterDefinition> a, Collection<ParameterDefinition> b) =>
AreParametersCompatibleWith (a, b, cache: null);

public static bool AreParametersCompatibleWith (this Collection<ParameterDefinition> a, Collection<ParameterDefinition> b, TypeDefinitionCache cache)
{
if (a.Count != b.Count)
return false;
Expand All @@ -48,34 +60,34 @@ public static bool AreParametersCompatibleWith (this Collection<ParameterDefinit
return true;

for (int i = 0; i < a.Count; i++)
if (!IsParameterCompatibleWith (a [i].ParameterType, b [i].ParameterType))
if (!IsParameterCompatibleWith (a [i].ParameterType, b [i].ParameterType, cache))
return false;

return true;
}

static bool IsParameterCompatibleWith (IModifierType a, IModifierType b)
static bool IsParameterCompatibleWith (IModifierType a, IModifierType b, TypeDefinitionCache cache)
{
if (!IsParameterCompatibleWith (a.ModifierType, b.ModifierType))
if (!IsParameterCompatibleWith (a.ModifierType, b.ModifierType, cache))
return false;

return IsParameterCompatibleWith (a.ElementType, b.ElementType);
return IsParameterCompatibleWith (a.ElementType, b.ElementType, cache);
}

static bool IsParameterCompatibleWith (TypeSpecification a, TypeSpecification b)
static bool IsParameterCompatibleWith (TypeSpecification a, TypeSpecification b, TypeDefinitionCache cache)
{
if (a is GenericInstanceType)
return IsParameterCompatibleWith ((GenericInstanceType) a, (GenericInstanceType) b);
return IsParameterCompatibleWith ((GenericInstanceType) a, (GenericInstanceType) b, cache);

if (a is IModifierType)
return IsParameterCompatibleWith ((IModifierType) a, (IModifierType) b);
return IsParameterCompatibleWith ((IModifierType) a, (IModifierType) b, cache);

return IsParameterCompatibleWith (a.ElementType, b.ElementType);
return IsParameterCompatibleWith (a.ElementType, b.ElementType, cache);
}

static bool IsParameterCompatibleWith (GenericInstanceType a, GenericInstanceType b)
static bool IsParameterCompatibleWith (GenericInstanceType a, GenericInstanceType b, TypeDefinitionCache cache)
{
if (!IsParameterCompatibleWith (a.ElementType, b.ElementType))
if (!IsParameterCompatibleWith (a.ElementType, b.ElementType, cache))
return false;

if (a.GenericArguments.Count != b.GenericArguments.Count)
Expand All @@ -85,32 +97,27 @@ static bool IsParameterCompatibleWith (GenericInstanceType a, GenericInstanceTyp
return true;

for (int i = 0; i < a.GenericArguments.Count; i++)
if (!IsParameterCompatibleWith (a.GenericArguments [i], b.GenericArguments [i]))
if (!IsParameterCompatibleWith (a.GenericArguments [i], b.GenericArguments [i], cache))
return false;

return true;
}

static bool IsParameterCompatibleWith (GenericParameter a, GenericParameter b)
{
return a.Position == b.Position;
}

static bool IsParameterCompatibleWith (TypeReference a, TypeReference b)
static bool IsParameterCompatibleWith (TypeReference a, TypeReference b, TypeDefinitionCache cache)
{
if (a is TypeSpecification || b is TypeSpecification) {
if (a.GetType () != b.GetType ())
return false;

return IsParameterCompatibleWith ((TypeSpecification) a, (TypeSpecification) b);
return IsParameterCompatibleWith ((TypeSpecification) a, (TypeSpecification) b, cache);
}

if (a.IsGenericParameter) {
if (b.IsGenericParameter && a.Name == b.Name)
return true;
var gpa = (GenericParameter) a;
foreach (var c in gpa.Constraints) {
if (!c.ConstraintType.IsAssignableFrom (b))
if (!c.ConstraintType.IsAssignableFrom (b, cache))
return false;
}
return true;
Expand Down
21 changes: 21 additions & 0 deletions src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionCache.cs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using System.Collections.Generic;
using Mono.Cecil;

namespace Java.Interop.Tools.Cecil
{
/// <summary>
/// A class for caching lookups from TypeReference -> TypeDefinition.
/// Generally its lifetime should match an AssemblyResolver instance.
/// </summary>
public class TypeDefinitionCache
{
readonly Dictionary<TypeReference, TypeDefinition> cache = new Dictionary<TypeReference, TypeDefinition> ();

public virtual TypeDefinition Resolve (TypeReference typeReference)
{
if (cache.TryGetValue (typeReference, out var typeDefinition))
return typeDefinition;
return cache [typeReference] = typeReference.Resolve ();
}
}
}
78 changes: 59 additions & 19 deletions src/Java.Interop.Tools.Cecil/Java.Interop.Tools.Cecil/TypeDefinitionRocks.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,54 +8,82 @@ namespace Java.Interop.Tools.Cecil {

public static class TypeDefinitionRocks {

public static TypeDefinition GetBaseType (this TypeDefinition type)
[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)
{
var bt = type.BaseType;
return bt == null ? null : bt.Resolve ();
if (bt == null)
return null;
if (cache != null)
return cache.Resolve (bt);
return bt.Resolve ();
}

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

public static IEnumerable<TypeDefinition> GetTypeAndBaseTypes (this TypeDefinition type, TypeDefinitionCache cache)
{
while (type != null) {
yield return type;
type = type.GetBaseType ();
type = type.GetBaseType (cache);
}
}

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

public static IEnumerable<TypeDefinition> GetBaseTypes (this TypeDefinition type, TypeDefinitionCache cache)
{
while ((type = type.GetBaseType ()) != null) {
while ((type = type.GetBaseType (cache)) != null) {
yield return type;
}
}

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

public static bool IsAssignableFrom (this TypeReference type, TypeReference c, TypeDefinitionCache cache)
{
if (type.FullName == c.FullName)
return true;
var d = c.Resolve ();
if (d == null)
return false;
foreach (var t in d.GetTypeAndBaseTypes ()) {
foreach (var t in d.GetTypeAndBaseTypes (cache)) {
if (type.FullName == t.FullName)
return true;
foreach (var ifaceImpl in t.Interfaces) {
var i = ifaceImpl.InterfaceType;
if (IsAssignableFrom (type, i))
if (IsAssignableFrom (type, i, cache))
return true;
}
}
return false;
}

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

public static bool IsSubclassOf (this TypeDefinition type, string typeName, TypeDefinitionCache cache)
{
return type.GetTypeAndBaseTypes ().Any (t => t.FullName == typeName);
return type.GetTypeAndBaseTypes (cache).Any (t => t.FullName == typeName);
}

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

public static bool ImplementsInterface (this TypeDefinition type, string interfaceName, TypeDefinitionCache cache)
{
foreach (var t in type.GetTypeAndBaseTypes ()) {
foreach (var t in type.GetTypeAndBaseTypes (cache)) {
foreach (var i in t.Interfaces) {
if (i.InterfaceType.FullName == interfaceName) {
return true;
Expand All @@ -65,24 +93,36 @@ public static bool ImplementsInterface (this TypeDefinition type, string interfa
return false;
}

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

public static string GetPartialAssemblyName (this TypeReference type, TypeDefinitionCache cache)
{
TypeDefinition def = type.Resolve ();
TypeDefinition def = cache != null ? cache.Resolve (type) : type.Resolve ();
return (def ?? type).Module.Assembly.Name.Name;
}

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

public static string GetPartialAssemblyQualifiedName (this TypeReference type, TypeDefinitionCache cache)
{
return string.Format ("{0}, {1}",
// Cecil likes to use '/' as the nested type separator, while
// Reflection uses '+' as the nested type separator. Use Reflection.
type.FullName.Replace ('/', '+'),
type.GetPartialAssemblyName ());
type.GetPartialAssemblyName (cache));
}

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

public static string GetAssemblyQualifiedName (this TypeReference type, TypeDefinitionCache cache)
{
TypeDefinition def = type.Resolve ();
TypeDefinition def = cache != null ? cache.Resolve (type) : type.Resolve ();
return string.Format ("{0}, {1}",
// Cecil likes to use '/' as the nested type separator, while
// Reflection uses '+' as the nested type separator. Use Reflection.
Expand Down

0 comments on commit b81cfbb

Please sign in to comment.