Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] use Java.Interop's TypeDefinitionCache
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Feb 19, 2020
1 parent 87ee20c commit 4fe1f8b
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 29 deletions.
2 changes: 1 addition & 1 deletion external/Java.Interop
Submodule Java.Interop updated from 8f3093 to cd57e9
29 changes: 15 additions & 14 deletions src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,8 @@ void Run (DirectoryAssemblyResolver res)
}

// Step 1 - Find all the JLO types
var scanner = new JavaTypeScanner (this.CreateTaskLogger ()) {
var cache = new TypeDefinitionCache ();
var scanner = new JavaTypeScanner (this.CreateTaskLogger (), cache) {
ErrorOnCustomJavaObject = ErrorOnCustomJavaObject,
};

Expand All @@ -175,15 +176,15 @@ void Run (DirectoryAssemblyResolver res)

var javaTypes = new List<TypeDefinition> ();
foreach (TypeDefinition td in allJavaTypes) {
if (!userAssemblies.ContainsKey (td.Module.Assembly.Name.Name) || JavaTypeScanner.ShouldSkipJavaCallableWrapperGeneration (td)) {
if (!userAssemblies.ContainsKey (td.Module.Assembly.Name.Name) || JavaTypeScanner.ShouldSkipJavaCallableWrapperGeneration (td, cache)) {
continue;
}

javaTypes.Add (td);
}

// Step 3 - Generate Java stub code
var success = CreateJavaSources (javaTypes);
var success = CreateJavaSources (javaTypes, cache);
if (!success)
return;

Expand All @@ -199,7 +200,7 @@ void Run (DirectoryAssemblyResolver res)
string managedKey = type.FullName.Replace ('/', '.');
string javaKey = JavaNativeTypeManager.ToJniName (type).Replace ('/', '.');

acw_map.Write (type.GetPartialAssemblyQualifiedName ());
acw_map.Write (type.GetPartialAssemblyQualifiedName (cache));
acw_map.Write (';');
acw_map.Write (javaKey);
acw_map.WriteLine ();
Expand All @@ -208,14 +209,14 @@ void Run (DirectoryAssemblyResolver res)
bool hasConflict = false;
if (managed.TryGetValue (managedKey, out conflict)) {
if (!managedConflicts.TryGetValue (managedKey, out var list))
managedConflicts.Add (managedKey, list = new List<string> { conflict.GetPartialAssemblyName () });
list.Add (type.GetPartialAssemblyName ());
managedConflicts.Add (managedKey, list = new List<string> { conflict.GetPartialAssemblyName (cache) });
list.Add (type.GetPartialAssemblyName (cache));
hasConflict = true;
}
if (java.TryGetValue (javaKey, out conflict)) {
if (!javaConflicts.TryGetValue (javaKey, out var list))
javaConflicts.Add (javaKey, list = new List<string> { conflict.GetAssemblyQualifiedName () });
list.Add (type.GetAssemblyQualifiedName ());
javaConflicts.Add (javaKey, list = new List<string> { conflict.GetAssemblyQualifiedName (cache) });
list.Add (type.GetAssemblyQualifiedName (cache));
success = false;
hasConflict = true;
}
Expand All @@ -228,7 +229,7 @@ void Run (DirectoryAssemblyResolver res)
acw_map.Write (javaKey);
acw_map.WriteLine ();

acw_map.Write (JavaNativeTypeManager.ToCompatJniName (type).Replace ('/', '.'));
acw_map.Write (JavaNativeTypeManager.ToCompatJniName (type, cache).Replace ('/', '.'));
acw_map.Write (';');
acw_map.Write (javaKey);
acw_map.WriteLine ();
Expand Down Expand Up @@ -265,7 +266,7 @@ void Run (DirectoryAssemblyResolver res)
manifest.NeedsInternet = NeedsInternet;
manifest.InstantRunEnabled = InstantRunEnabled;

var additionalProviders = manifest.Merge (Log, allJavaTypes, ApplicationJavaClass, EmbedAssemblies, BundledWearApplicationName, MergedManifestDocuments);
var additionalProviders = manifest.Merge (Log, cache, allJavaTypes, ApplicationJavaClass, EmbedAssemblies, BundledWearApplicationName, MergedManifestDocuments);

// Only write the new manifest if it actually changed
if (manifest.SaveIfChanged (Log, MergedAndroidManifestOutput)) {
Expand All @@ -286,10 +287,10 @@ void Run (DirectoryAssemblyResolver res)
StringWriter regCallsWriter = new StringWriter ();
regCallsWriter.WriteLine ("\t\t// Application and Instrumentation ACWs must be registered first.");
foreach (var type in javaTypes) {
if (JavaNativeTypeManager.IsApplication (type) || JavaNativeTypeManager.IsInstrumentation (type)) {
if (JavaNativeTypeManager.IsApplication (type, cache) || JavaNativeTypeManager.IsInstrumentation (type, cache)) {
string javaKey = JavaNativeTypeManager.ToJniName (type).Replace ('/', '.');
regCallsWriter.WriteLine ("\t\tmono.android.Runtime.register (\"{0}\", {1}.class, {1}.__md_methods);",
type.GetAssemblyQualifiedName (), javaKey);
type.GetAssemblyQualifiedName (cache), javaKey);
}
}
regCallsWriter.Close ();
Expand All @@ -300,7 +301,7 @@ void Run (DirectoryAssemblyResolver res)
template => template.Replace ("// REGISTER_APPLICATION_AND_INSTRUMENTATION_CLASSES_HERE", regCallsWriter.ToString ()));
}

bool CreateJavaSources (IEnumerable<TypeDefinition> javaTypes)
bool CreateJavaSources (IEnumerable<TypeDefinition> javaTypes, TypeDefinitionCache cache)
{
string outputPath = Path.Combine (OutputDirectory, "src");
string monoInit = GetMonoInitSource (AndroidSdkPlatform, UseSharedRuntime);
Expand All @@ -311,7 +312,7 @@ bool CreateJavaSources (IEnumerable<TypeDefinition> javaTypes)
foreach (var t in javaTypes) {
using (var writer = MemoryStreamPool.Shared.CreateStreamWriter ()) {
try {
var jti = new JavaCallableWrapperGenerator (t, Log.LogWarning) {
var jti = new JavaCallableWrapperGenerator (t, Log.LogWarning, cache) {
GenerateOnCreateOverrides = generateOnCreateOverrides,
ApplicationJavaClass = ApplicationJavaClass,
MonoRuntimeInitialization = monoInit,
Expand Down
28 changes: 14 additions & 14 deletions src/Xamarin.Android.Build.Tasks/Utilities/ManifestDocument.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ void ReorderActivityAliases (TaskLoggingHelper log, XElement app)
}
}

public IList<string> Merge (TaskLoggingHelper log, List<TypeDefinition> subclasses, string applicationClass, bool embed, string bundledWearApplicationName, IEnumerable<string> mergedManifestDocuments)
public IList<string> Merge (TaskLoggingHelper log, TypeDefinitionCache cache, List<TypeDefinition> subclasses, string applicationClass, bool embed, string bundledWearApplicationName, IEnumerable<string> mergedManifestDocuments)
{
string applicationName = ApplicationName;

Expand All @@ -241,7 +241,7 @@ public IList<string> Merge (TaskLoggingHelper log, List<TypeDefinition> subclass
if (manifest.Attribute (androidNs + "versionName") == null)
manifest.SetAttributeValue (androidNs + "versionName", "1.0");

app = CreateApplicationElement (manifest, applicationClass, subclasses);
app = CreateApplicationElement (manifest, applicationClass, subclasses, cache);

if (app.Attribute (androidNs + "label") == null && applicationName != null)
app.SetAttributeValue (androidNs + "label", applicationName);
Expand Down Expand Up @@ -296,12 +296,12 @@ public IList<string> Merge (TaskLoggingHelper log, List<TypeDefinition> subclass
PackageName = t.Namespace;

var name = JavaNativeTypeManager.ToJniName (t).Replace ('/', '.');
var compatName = JavaNativeTypeManager.ToCompatJniName (t).Replace ('/', '.');
var compatName = JavaNativeTypeManager.ToCompatJniName (t, cache).Replace ('/', '.');
if (((string) app.Attribute (attName)) == compatName) {
app.SetAttributeValue (attName, name);
}

Func<TypeDefinition, string, int, XElement> generator = GetGenerator (t);
Func<TypeDefinition, string, int, XElement> generator = GetGenerator (t, cache);
if (generator == null)
continue;

Expand Down Expand Up @@ -395,7 +395,7 @@ public IList<string> Merge (TaskLoggingHelper log, List<TypeDefinition> subclass
}
}

AddInstrumentations (manifest, subclasses, targetSdkVersionValue);
AddInstrumentations (manifest, subclasses, targetSdkVersionValue, cache);
AddPermissions (app);
AddPermissionGroups (app);
AddPermissionTrees (app);
Expand Down Expand Up @@ -492,20 +492,20 @@ IEnumerable<XNode> FixupNameElements(string packageName, IEnumerable<XNode> node
return nodes;
}

Func<TypeDefinition, string, int, XElement> GetGenerator (TypeDefinition type)
Func<TypeDefinition, string, int, XElement> GetGenerator (TypeDefinition type, TypeDefinitionCache cache)
{
if (type.IsSubclassOf ("Android.App.Activity"))
if (type.IsSubclassOf ("Android.App.Activity", cache))
return ActivityFromTypeDefinition;
if (type.IsSubclassOf ("Android.App.Service"))
if (type.IsSubclassOf ("Android.App.Service", cache))
return (t, name, v) => ToElement (t, name, ServiceAttribute.FromTypeDefinition, x => x.ToElement (PackageName));
if (type.IsSubclassOf ("Android.Content.BroadcastReceiver"))
if (type.IsSubclassOf ("Android.Content.BroadcastReceiver", cache))
return (t, name, v) => ToElement (t, name, BroadcastReceiverAttribute.FromTypeDefinition, x => x.ToElement (PackageName));
if (type.IsSubclassOf ("Android.Content.ContentProvider"))
if (type.IsSubclassOf ("Android.Content.ContentProvider", cache))
return (t, name, v) => ToProviderElement (t, name);
return null;
}

XElement CreateApplicationElement (XElement manifest, string applicationClass, List<TypeDefinition> subclasses)
XElement CreateApplicationElement (XElement manifest, string applicationClass, List<TypeDefinition> subclasses, TypeDefinitionCache cache)
{
var application = manifest.Descendants ("application").FirstOrDefault ();

Expand Down Expand Up @@ -534,7 +534,7 @@ XElement CreateApplicationElement (XElement manifest, string applicationClass, L
if (aa == null)
continue;

if (!t.IsSubclassOf ("Android.App.Application"))
if (!t.IsSubclassOf ("Android.App.Application", cache))
throw new InvalidOperationException (string.Format ("Found [Application] on type {0}. [Application] can only be used on subclasses of Application.", t.FullName));

typeAttr.Add (aa);
Expand Down Expand Up @@ -860,7 +860,7 @@ void AddSupportsGLTextures (XElement application)
}
}

void AddInstrumentations (XElement manifest, IList<TypeDefinition> subclasses, int targetSdkVersion)
void AddInstrumentations (XElement manifest, IList<TypeDefinition> subclasses, int targetSdkVersion, TypeDefinitionCache cache)
{
var assemblyAttrs =
Assemblies.SelectMany (path => InstrumentationAttribute.FromCustomAttributeProvider (Resolver.GetAssembly (path)));
Expand All @@ -874,7 +874,7 @@ void AddInstrumentations (XElement manifest, IList<TypeDefinition> subclasses, i
}

foreach (var type in subclasses)
if (type.IsSubclassOf ("Android.App.Instrumentation")) {
if (type.IsSubclassOf ("Android.App.Instrumentation", cache)) {
var xe = InstrumentationFromTypeDefinition (type, JavaNativeTypeManager.ToJniName (type).Replace ('/', '.'), targetSdkVersion);
if (xe != null)
manifest.Add (xe);
Expand Down

0 comments on commit 4fe1f8b

Please sign in to comment.