Skip to content

Commit

Permalink
[Xamarin.Android.Build.Tasks] no temp files in GenerateJavaStubs (#2535)
Browse files Browse the repository at this point in the history 
Context: #2505

This is based on #2505, but I incorporated the Java.Interop changes
so that `<GenerateJavaStubs/>` uses no temp files at all.

In many places throughout the Xamarin.Android build, we have a
pattern of:

  - Generate a temp file.
  - Use `MonoAndroidHelper.CopyIfChanged()` to put the file in the
    destination.  This reads *both* files, doing a hash comparison
    before deciding to write or not.
  - Delete the temp file.

Sometimes the temp file is actually in `%TEMP%`, but other times we
append `.new` to the destination file.  The case of `.new` can
collide if two builds are running at once (example caused by a known
VS for Mac issue).

Thinking about this, in cases with small files, we can make a simple
optimization:

  - Generate the file in-memory.
  - Use a new `MonoAndroidHelper.CopyStreamIfChanged()` method.

This has several benefits:

  - We never write a file to disk when there are no changes.
  - We don't have to *remember* to delete the file.
  - The code, in general, is slightly simpler.

The only place we likely shouldn't use this new pattern, would be if
the file was huge.

~~ Changes ~~

I added new APIs for:

  - `Files.HasStreamChanged()` - to compare a `Stream` in memory
  - `Files.HasBytesChanged()` - to compare a `byte[]` in memory
  - `Files.CopyIfStreamChanged()`
  - `Files.CopyIfStringChanged()` - some cases we have a `string`
  - `Files.CopyIfBytesChanged()` - this supports `string`
  - `MonoAndroidHelper.CopyIfStreamChanged()`
  - `MonoAndroidHelper.CopyIfStringChanged()`
  - `MonoAndroidHelper.CopyIfBytesChanged()`

I changed the following MSBuild tasks, mostly to test out the new
behavior:

  - `<GenerateResourceDesigner/>` was using a `.new` file.
  - `<GenerateJavaStubs/>` was using temp files in many places.
    I was able to fix up all of these.
  - There is now a `ManifestDocument.Save(Stream)` overload.
  - `Generator` now uses `CopyIfStreamChanged()` and a new
    `GetDestinationPath()` method from Java.Interop.  It reuses a
    single `MemoryStream`, and I moved the `<GenerateJavaSource/>`
    method into `<CreateJavaSources/>` for simplicity.

I made other general refactoring in `<GenerateJavaStubs/>`:

  - Since we don't have to worry about deleting a `temp_map_file`, we
    can return earlier if `Generator.CreateJavaSources()` fails.
  - A `GetResource<T>()` method, cleans up the places reading from
    `@(EmbeddedResource)` files.  I also changed it up to properly
    `Dispose()` things.
  - A `save` anonymous method/delegate should just be a
    `SaveResource()` *regular* method.
  - A few places were calling `Path.GetFullPath()` unnecessarily.
    Since this method accesses the file system, we should skip it
    unless the full path is actually needed.
  - Avoid using `StringWriter` and `string.Format()`.
  - Use capacity and `StringComparer.Ordinal` when creating
    dictionaries: https://www.dotnetperls.com/dictionary-stringcomparer
  - Preallocate `MemoryStream` with `java_types.Length * 32`

I also added some tests for the new APIs in `MonoAndroidHelper`.

~~ Results ~~

I did three test runs, because I was getting varying times for
`<GenerateJavaStubs/>`.  This is the Xamarin.Forms integration
project in this repo:

	Before (Clean Build):
	1. 1433 ms  GenerateJavaStubs                          1 calls
	2. 1594 ms  GenerateJavaStubs                          1 calls
	3. 1353 ms  GenerateJavaStubs                          1 calls

	After (Clean Build):
	1. 1201 ms  GenerateJavaStubs                          1 calls
	2. 1137 ms  GenerateJavaStubs                          1 calls
	3. 1136 ms  GenerateJavaStubs                          1 calls

	Before (Incremental):
	1. 1184 ms  GenerateJavaStubs                          1 calls
	2. 1181 ms  GenerateJavaStubs                          1 calls
	3. 1172 ms  GenerateJavaStubs                          1 calls

	After (Incremental):
	1. 1035 ms  GenerateJavaStubs                          1 calls
	2. 1049 ms  GenerateJavaStubs                          1 calls
	3. 1036 ms  GenerateJavaStubs                          1 calls

`<GenerateJavaStubs/>` is now about 250ms faster on initial build and
150ms faster on incremental builds.

I could not see a difference in `<GenerateResourceDesigner/>`, likely
since it wrote only a single temp file.

Next steps would be to make changes in other MSBuild tasks as well.
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Jan 2, 2019
1 parent 1d031dc commit 7088bd2
Show file tree
Hide file tree
Showing 8 changed files with 373 additions and 144 deletions.
75 changes: 39 additions & 36 deletions src/Xamarin.Android.Build.Tasks/Generator/Generator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

using Java.Interop.Tools.Diagnostics;
using Java.Interop.Tools.JavaCallableWrappers;
using System.IO;

namespace Xamarin.Android.Tasks
{
Expand All @@ -15,45 +16,47 @@ class Generator
public static bool CreateJavaSources (TaskLoggingHelper log, IEnumerable<TypeDefinition> javaTypes, string outputPath, string applicationJavaClass, bool useSharedRuntime, bool generateOnCreateOverrides, bool hasExportReference)
{
bool ok = true;
foreach (var t in javaTypes) {
try {
GenerateJavaSource (log, t, outputPath, applicationJavaClass, useSharedRuntime, generateOnCreateOverrides, hasExportReference);
} catch (XamarinAndroidException xae) {
ok = false;
log.LogError (
subcategory: "",
errorCode: "XA" + xae.Code,
helpKeyword: string.Empty,
file: xae.SourceFile,
lineNumber: xae.SourceLine,
columnNumber: 0,
endLineNumber: 0,
endColumnNumber: 0,
message: xae.MessageWithoutCode,
messageArgs: new object [0]
);
}
}
return ok;
}
using (var memoryStream = new MemoryStream ())
using (var writer = new StreamWriter (memoryStream)) {
foreach (var t in javaTypes) {
//Reset for reuse
memoryStream.SetLength (0);

static void GenerateJavaSource (TaskLoggingHelper log, TypeDefinition t, string outputPath, string applicationJavaClass, bool useSharedRuntime, bool generateOnCreateOverrides, bool hasExportReference)
{
try {
var jti = new JavaCallableWrapperGenerator (t, log.LogWarning) {
UseSharedRuntime = useSharedRuntime,
GenerateOnCreateOverrides = generateOnCreateOverrides,
ApplicationJavaClass = applicationJavaClass,
};
try {
var jti = new JavaCallableWrapperGenerator (t, log.LogWarning) {
UseSharedRuntime = useSharedRuntime,
GenerateOnCreateOverrides = generateOnCreateOverrides,
ApplicationJavaClass = applicationJavaClass,
};

jti.Generate (writer);
writer.Flush ();

jti.Generate (outputPath);
if (jti.HasExport && !hasExportReference)
Diagnostic.Error (4210, "You need to add a reference to Mono.Android.Export.dll when you use ExportAttribute or ExportFieldAttribute.");
} catch (Exception ex) {
if (ex is XamarinAndroidException)
throw;
Diagnostic.Error (4209, "Failed to create JavaTypeInfo for class: {0} due to {1}", t.FullName, ex);
var path = jti.GetDestinationPath (outputPath);
MonoAndroidHelper.CopyIfStreamChanged (memoryStream, path);
if (jti.HasExport && !hasExportReference)
Diagnostic.Error (4210, "You need to add a reference to Mono.Android.Export.dll when you use ExportAttribute or ExportFieldAttribute.");
} catch (XamarinAndroidException xae) {
ok = false;
log.LogError (
subcategory: "",
errorCode: "XA" + xae.Code,
helpKeyword: string.Empty,
file: xae.SourceFile,
lineNumber: xae.SourceLine,
columnNumber: 0,
endLineNumber: 0,
endColumnNumber: 0,
message: xae.MessageWithoutCode,
messageArgs: new object [0]
);
} catch (Exception ex) {
ok = false;
Diagnostic.Error (4209, "Failed to create JavaTypeInfo for class: {0} due to {1}", t.FullName, ex);
}
}
}
return ok;
}
}
}
189 changes: 91 additions & 98 deletions src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ public override bool Execute ()
using (var res = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: true)) {
Run (res);
}
}
catch (XamarinAndroidException e) {
} catch (XamarinAndroidException e) {
Log.LogCodedError (string.Format ("XA{0:0000}", e.Code), e.MessageWithoutCode);
if (MonoAndroidHelper.LogInternalExceptions)
Log.LogMessage (e.ToString ());
Expand All @@ -99,8 +98,6 @@ void Run (DirectoryAssemblyResolver res)
{
PackageNamingPolicy pnp;
JavaNativeTypeManager.PackageNamingPolicy = Enum.TryParse (PackageNamingPolicy, out pnp) ? pnp : PackageNamingPolicyEnum.LowercaseHash;
var temp = Path.Combine (Path.GetTempPath (), Path.GetRandomFileName ());
Directory.CreateDirectory (temp);

foreach (var dir in FrameworkDirectories) {
if (Directory.Exists (dir.ItemSpec))
Expand All @@ -111,9 +108,10 @@ void Run (DirectoryAssemblyResolver res)

// Put every assembly we'll need in the resolver
foreach (var assembly in ResolvedAssemblies) {
res.Load (Path.GetFullPath (assembly.ItemSpec));
var assemblyFullPath = Path.GetFullPath (assembly.ItemSpec);
res.Load (assemblyFullPath);
if (MonoAndroidHelper.FrameworkAttributeLookupTargets.Any (a => Path.GetFileName (assembly.ItemSpec) == a))
selectedWhitelistAssemblies.Add (Path.GetFullPath (assembly.ItemSpec));
selectedWhitelistAssemblies.Add (assemblyFullPath);
}

// However we only want to look for JLO types in user code
Expand All @@ -130,76 +128,75 @@ void Run (DirectoryAssemblyResolver res)

WriteTypeMappings (all_java_types);

var java_types = all_java_types.Where (t => !JavaTypeScanner.ShouldSkipJavaCallableWrapperGeneration (t));
var java_types = all_java_types
.Where (t => !JavaTypeScanner.ShouldSkipJavaCallableWrapperGeneration (t))
.ToArray ();

// Step 2 - Generate Java stub code
var keep_going = Generator.CreateJavaSources (
var success = Generator.CreateJavaSources (
Log,
java_types,
temp,
Path.Combine (OutputDirectory, "src"),
ApplicationJavaClass,
UseSharedRuntime,
int.Parse (AndroidSdkPlatform) <= 10,
ResolvedAssemblies.Any (assembly => Path.GetFileName (assembly.ItemSpec) == "Mono.Android.Export.dll"));

var temp_map_file = Path.Combine (temp, "acw-map.temp");
if (!success)
return;

// We need to save a map of .NET type -> ACW type for resource file fixups
var managed = new Dictionary<string, TypeDefinition> ();
var java = new Dictionary<string, TypeDefinition> ();
var acw_map = new StreamWriter (temp_map_file);

foreach (var type in java_types) {
string managedKey = type.FullName.Replace ('/', '.');
string javaKey = JavaNativeTypeManager.ToJniName (type).Replace ('/', '.');

acw_map.WriteLine ("{0};{1}", type.GetPartialAssemblyQualifiedName (), javaKey);

TypeDefinition conflict;
if (managed.TryGetValue (managedKey, out conflict)) {
Log.LogWarning (
"Duplicate managed type found! Mappings between managed types and Java types must be unique. " +
"First Type: '{0}'; Second Type: '{1}'.",
conflict.GetAssemblyQualifiedName (),
type.GetAssemblyQualifiedName ());
Log.LogWarning (
"References to the type '{0}' will refer to '{1}'.",
managedKey, conflict.GetAssemblyQualifiedName ());
continue;
}
if (java.TryGetValue (javaKey, out conflict)) {
Log.LogError (
"Duplicate Java type found! Mappings between managed types and Java types must be unique. " +
"First Type: '{0}'; Second Type: '{1}'",
conflict.GetAssemblyQualifiedName (),
type.GetAssemblyQualifiedName ());
keep_going = false;
continue;
var managed = new Dictionary<string, TypeDefinition> (java_types.Length, StringComparer.Ordinal);
var java = new Dictionary<string, TypeDefinition> (java_types.Length, StringComparer.Ordinal);
// Allocate a MemoryStream with a reasonable guess at its capacity
using (var stream = new MemoryStream (java_types.Length * 32))
using (var acw_map = new StreamWriter (stream)) {
foreach (var type in java_types) {
string managedKey = type.FullName.Replace ('/', '.');
string javaKey = JavaNativeTypeManager.ToJniName (type).Replace ('/', '.');

acw_map.Write (type.GetPartialAssemblyQualifiedName ());
acw_map.Write (';');
acw_map.Write (javaKey);
acw_map.WriteLine ();

TypeDefinition conflict;
if (managed.TryGetValue (managedKey, out conflict)) {
Log.LogWarning (
"Duplicate managed type found! Mappings between managed types and Java types must be unique. " +
"First Type: '{0}'; Second Type: '{1}'.",
conflict.GetAssemblyQualifiedName (),
type.GetAssemblyQualifiedName ());
Log.LogWarning (
"References to the type '{0}' will refer to '{1}'.",
managedKey, conflict.GetAssemblyQualifiedName ());
continue;
}
if (java.TryGetValue (javaKey, out conflict)) {
Log.LogError (
"Duplicate Java type found! Mappings between managed types and Java types must be unique. " +
"First Type: '{0}'; Second Type: '{1}'",
conflict.GetAssemblyQualifiedName (),
type.GetAssemblyQualifiedName ());
success = false;
continue;
}

managed.Add (managedKey, type);
java.Add (javaKey, type);

acw_map.Write (managedKey);
acw_map.Write (';');
acw_map.Write (javaKey);
acw_map.WriteLine ();

acw_map.Write (JavaNativeTypeManager.ToCompatJniName (type).Replace ('/', '.'));
acw_map.Write (';');
acw_map.Write (javaKey);
acw_map.WriteLine ();
}
managed.Add (managedKey, type);
java.Add (javaKey, type);
acw_map.WriteLine ("{0};{1}", managedKey, javaKey);
acw_map.WriteLine ("{0};{1}", JavaNativeTypeManager.ToCompatJniName (type).Replace ('/', '.'), javaKey);
}

acw_map.Close ();

//The previous steps found an error, so we must abort and not generate any further output
//We must do so subsequent unchanged builds fail too.
if (!keep_going) {
File.Delete (temp_map_file);
return;
}

MonoAndroidHelper.CopyIfChanged (temp_map_file, AcwMapFile);

try { File.Delete (temp_map_file); } catch (Exception) { }

// Only overwrite files if the contents actually changed
foreach (var file in Directory.GetFiles (temp, "*", SearchOption.AllDirectories)) {
var dest = Path.GetFullPath (Path.Combine (OutputDirectory, "src", file.Substring (temp.Length + 1)));

MonoAndroidHelper.CopyIfChanged (file, dest);
acw_map.Flush ();
MonoAndroidHelper.CopyIfStreamChanged (stream, AcwMapFile);
}

// Step 3 - Merge [Activity] and friends into AndroidManifest.xml
Expand All @@ -219,38 +216,24 @@ void Run (DirectoryAssemblyResolver res)

var additionalProviders = manifest.Merge (all_java_types, selectedWhitelistAssemblies, ApplicationJavaClass, EmbedAssemblies, BundledWearApplicationName, MergedManifestDocuments);

var temp_manifest = Path.Combine (temp, "AndroidManifest.xml");
var real_manifest = Path.GetFullPath (MergedAndroidManifestOutput);
using (var stream = new MemoryStream ()) {
manifest.Save (stream);

manifest.Save (temp_manifest);

// Only write the new manifest if it actually changed
MonoAndroidHelper.CopyIfChanged (temp_manifest, real_manifest);
// Only write the new manifest if it actually changed
MonoAndroidHelper.CopyIfStreamChanged (stream, MergedAndroidManifestOutput);
}

// Create additional runtime provider java sources.
string providerTemplateFile = UseSharedRuntime ? "MonoRuntimeProvider.Shared.java" : "MonoRuntimeProvider.Bundled.java";
string providerTemplate = new StreamReader (typeof (JavaCallableWrapperGenerator).Assembly.GetManifestResourceStream (providerTemplateFile)).ReadToEnd ();
string providerTemplate = GetResource<JavaCallableWrapperGenerator> (providerTemplateFile);

foreach (var provider in additionalProviders) {
var temp_provider = Path.Combine (temp, provider + ".java");
File.WriteAllText (temp_provider, providerTemplate.Replace ("MonoRuntimeProvider", provider));
var real_provider_dir = Path.GetFullPath (Path.Combine (OutputDirectory, "src", "mono"));
Directory.CreateDirectory (real_provider_dir);
var real_provider = Path.Combine (real_provider_dir, provider + ".java");
MonoAndroidHelper.CopyIfChanged (temp_provider, real_provider);
var contents = providerTemplate.Replace ("MonoRuntimeProvider", provider);
var real_provider = Path.Combine (OutputDirectory, "src", "mono", provider + ".java");
MonoAndroidHelper.CopyIfStringChanged (contents, real_provider);
}

// Create additional application java sources.

Action<string,string,string,Func<string,string>> save = (resource, filename, destDir, applyTemplate) => {
string temp_file = Path.Combine (temp, filename);
string template = applyTemplate (new StreamReader (typeof (GenerateJavaStubs).Assembly.GetManifestResourceStream (resource)).ReadToEnd ());
File.WriteAllText (temp_file, template);
Directory.CreateDirectory (destDir);
var real_file = Path.Combine (destDir, filename);
MonoAndroidHelper.CopyIfChanged (temp_file, real_file);
};

StringWriter regCallsWriter = new StringWriter ();
regCallsWriter.WriteLine ("\t\t// Application and Instrumentation ACWs must be registered first.");
foreach (var type in java_types) {
Expand All @@ -262,17 +245,28 @@ void Run (DirectoryAssemblyResolver res)
}
regCallsWriter.Close ();

var real_app_dir = Path.GetFullPath (Path.Combine (OutputDirectory, "src", "mono", "android", "app"));
var real_app_dir = Path.Combine (OutputDirectory, "src", "mono", "android", "app");
string applicationTemplateFile = "ApplicationRegistration.java";
save (applicationTemplateFile, applicationTemplateFile, real_app_dir,
SaveResource (applicationTemplateFile, applicationTemplateFile, real_app_dir,
template => template.Replace ("// REGISTER_APPLICATION_AND_INSTRUMENTATION_CLASSES_HERE", regCallsWriter.ToString ()));

// Create NotifyTimeZoneChanges java sources.
string notifyTimeZoneChangesFile = "NotifyTimeZoneChanges.java";
save (notifyTimeZoneChangesFile, notifyTimeZoneChangesFile, real_app_dir, template => template);

// Delete our temp directory
try { Directory.Delete (temp, true); } catch (Exception) { }
SaveResource (notifyTimeZoneChangesFile, notifyTimeZoneChangesFile, real_app_dir, template => template);
}

string GetResource <T> (string resource)
{
using (var stream = typeof (T).Assembly.GetManifestResourceStream (resource))
using (var reader = new StreamReader (stream))
return reader.ReadToEnd ();
}

void SaveResource (string resource, string filename, string destDir, Func<string, string> applyTemplate)
{
string template = GetResource<GenerateJavaStubs> (resource);
template = applyTemplate (template);
MonoAndroidHelper.CopyIfStringChanged (template, Path.Combine (destDir, filename));
}

void WriteTypeMappings (List<TypeDefinition> types)
Expand All @@ -287,11 +281,10 @@ void WriteTypeMappings (List<TypeDefinition> types)

void UpdateWhenChanged (string path, Action<Stream> generator)
{
var np = path + ".new";
using (var o = File.OpenWrite (np))
generator (o);
MonoAndroidHelper.CopyIfChanged (np, path);
File.Delete (np);
using (var stream = new MemoryStream ()) {
generator (stream);
MonoAndroidHelper.CopyIfStreamChanged (stream, path);
}
}
}
}
8 changes: 2 additions & 6 deletions src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesigner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,8 @@ private void WriteFile (string file, CodeTypeDeclaration resources, string langu
}
}
}

var temp_o = Path.Combine (Path.GetDirectoryName (file), "__" + Path.GetFileName (file) + ".new");
using (TextWriter o = File.CreateText (temp_o))
o.Write (code);
MonoAndroidHelper.CopyIfChanged (temp_o, file);
try { File.Delete (temp_o); } catch (Exception) { }

MonoAndroidHelper.CopyIfStringChanged (code, file);
}

private void AddRename (string android, string user)
Expand Down
Loading

0 comments on commit 7088bd2

Please # to comment.