Skip to content

Commit a3d4ee5

Browse files
jonathanpeppersjonathanpeppers
committed
[Xamarin.Android.Build.Tasks] no temp files in GenerateJavaStubs
Context: #2505 This is based on #2505, but I incorporated the java.interop changes so `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. - `ManifestDocument` now has an overload for `Save` that takes a `Stream` - `Generator` now uses `CopyIfStreamChanged` 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` unecessarily. 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.
1 parent b0aaaf2 commit a3d4ee5

File tree

8 files changed

+373
-144
lines changed

8 files changed

+373
-144
lines changed

‎src/Xamarin.Android.Build.Tasks/Generator/Generator.cs‎

Lines changed: 39 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
using Java.Interop.Tools.Diagnostics;
99
using Java.Interop.Tools.JavaCallableWrappers;
10+
using System.IO;
1011

1112
namespace Xamarin.Android.Tasks
1213
{
@@ -15,45 +16,47 @@ class Generator
1516
public static bool CreateJavaSources (TaskLoggingHelper log, IEnumerable<TypeDefinition> javaTypes, string outputPath, string applicationJavaClass, bool useSharedRuntime, bool generateOnCreateOverrides, bool hasExportReference)
1617
{
1718
bool ok = true;
18-
foreach (var t in javaTypes) {
19-
try {
20-
GenerateJavaSource (log, t, outputPath, applicationJavaClass, useSharedRuntime, generateOnCreateOverrides, hasExportReference);
21-
} catch (XamarinAndroidException xae) {
22-
ok = false;
23-
log.LogError (
24-
subcategory: "",
25-
errorCode: "XA" + xae.Code,
26-
helpKeyword: string.Empty,
27-
file: xae.SourceFile,
28-
lineNumber: xae.SourceLine,
29-
columnNumber: 0,
30-
endLineNumber: 0,
31-
endColumnNumber: 0,
32-
message: xae.MessageWithoutCode,
33-
messageArgs: new object [0]
34-
);
35-
}
36-
}
37-
return ok;
38-
}
19+
using (var memoryStream = new MemoryStream ())
20+
using (var writer = new StreamWriter (memoryStream)) {
21+
foreach (var t in javaTypes) {
22+
//Reset for reuse
23+
memoryStream.SetLength (0);
3924

40-
static void GenerateJavaSource (TaskLoggingHelper log, TypeDefinition t, string outputPath, string applicationJavaClass, bool useSharedRuntime, bool generateOnCreateOverrides, bool hasExportReference)
41-
{
42-
try {
43-
var jti = new JavaCallableWrapperGenerator (t, log.LogWarning) {
44-
UseSharedRuntime = useSharedRuntime,
45-
GenerateOnCreateOverrides = generateOnCreateOverrides,
46-
ApplicationJavaClass = applicationJavaClass,
47-
};
25+
try {
26+
var jti = new JavaCallableWrapperGenerator (t, log.LogWarning) {
27+
UseSharedRuntime = useSharedRuntime,
28+
GenerateOnCreateOverrides = generateOnCreateOverrides,
29+
ApplicationJavaClass = applicationJavaClass,
30+
};
31+
32+
jti.Generate (writer);
33+
writer.Flush ();
4834

49-
jti.Generate (outputPath);
50-
if (jti.HasExport && !hasExportReference)
51-
Diagnostic.Error (4210, "You need to add a reference to Mono.Android.Export.dll when you use ExportAttribute or ExportFieldAttribute.");
52-
} catch (Exception ex) {
53-
if (ex is XamarinAndroidException)
54-
throw;
55-
Diagnostic.Error (4209, "Failed to create JavaTypeInfo for class: {0} due to {1}", t.FullName, ex);
35+
var path = jti.GetDestinationPath (outputPath);
36+
MonoAndroidHelper.CopyIfStreamChanged (memoryStream, path);
37+
if (jti.HasExport && !hasExportReference)
38+
Diagnostic.Error (4210, "You need to add a reference to Mono.Android.Export.dll when you use ExportAttribute or ExportFieldAttribute.");
39+
} catch (XamarinAndroidException xae) {
40+
ok = false;
41+
log.LogError (
42+
subcategory: "",
43+
errorCode: "XA" + xae.Code,
44+
helpKeyword: string.Empty,
45+
file: xae.SourceFile,
46+
lineNumber: xae.SourceLine,
47+
columnNumber: 0,
48+
endLineNumber: 0,
49+
endColumnNumber: 0,
50+
message: xae.MessageWithoutCode,
51+
messageArgs: new object [0]
52+
);
53+
} catch (Exception ex) {
54+
ok = false;
55+
Diagnostic.Error (4209, "Failed to create JavaTypeInfo for class: {0} due to {1}", t.FullName, ex);
56+
}
57+
}
5658
}
59+
return ok;
5760
}
5861
}
5962
}

‎src/Xamarin.Android.Build.Tasks/Tasks/GenerateJavaStubs.cs‎

Lines changed: 91 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -76,8 +76,7 @@ public override bool Execute ()
7676
using (var res = new DirectoryAssemblyResolver (this.CreateTaskLogger (), loadDebugSymbols: true)) {
7777
Run (res);
7878
}
79-
}
80-
catch (XamarinAndroidException e) {
79+
} catch (XamarinAndroidException e) {
8180
Log.LogCodedError (string.Format ("XA{0:0000}", e.Code), e.MessageWithoutCode);
8281
if (MonoAndroidHelper.LogInternalExceptions)
8382
Log.LogMessage (e.ToString ());
@@ -99,8 +98,6 @@ void Run (DirectoryAssemblyResolver res)
9998
{
10099
PackageNamingPolicy pnp;
101100
JavaNativeTypeManager.PackageNamingPolicy = Enum.TryParse (PackageNamingPolicy, out pnp) ? pnp : PackageNamingPolicyEnum.LowercaseHash;
102-
var temp = Path.Combine (Path.GetTempPath (), Path.GetRandomFileName ());
103-
Directory.CreateDirectory (temp);
104101

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

112109
// Put every assembly we'll need in the resolver
113110
foreach (var assembly in ResolvedAssemblies) {
114-
res.Load (Path.GetFullPath (assembly.ItemSpec));
111+
var assemblyFullPath = Path.GetFullPath (assembly.ItemSpec);
112+
res.Load (assemblyFullPath);
115113
if (MonoAndroidHelper.FrameworkAttributeLookupTargets.Any (a => Path.GetFileName (assembly.ItemSpec) == a))
116-
selectedWhitelistAssemblies.Add (Path.GetFullPath (assembly.ItemSpec));
114+
selectedWhitelistAssemblies.Add (assemblyFullPath);
117115
}
118116

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

131129
WriteTypeMappings (all_java_types);
132130

133-
var java_types = all_java_types.Where (t => !JavaTypeScanner.ShouldSkipJavaCallableWrapperGeneration (t));
131+
var java_types = all_java_types
132+
.Where (t => !JavaTypeScanner.ShouldSkipJavaCallableWrapperGeneration (t))
133+
.ToArray ();
134134

135135
// Step 2 - Generate Java stub code
136-
var keep_going = Generator.CreateJavaSources (
136+
var success = Generator.CreateJavaSources (
137137
Log,
138138
java_types,
139-
temp,
139+
Path.Combine (OutputDirectory, "src"),
140140
ApplicationJavaClass,
141141
UseSharedRuntime,
142142
int.Parse (AndroidSdkPlatform) <= 10,
143143
ResolvedAssemblies.Any (assembly => Path.GetFileName (assembly.ItemSpec) == "Mono.Android.Export.dll"));
144-
145-
var temp_map_file = Path.Combine (temp, "acw-map.temp");
144+
if (!success)
145+
return;
146146

147147
// We need to save a map of .NET type -> ACW type for resource file fixups
148-
var managed = new Dictionary<string, TypeDefinition> ();
149-
var java = new Dictionary<string, TypeDefinition> ();
150-
var acw_map = new StreamWriter (temp_map_file);
151-
152-
foreach (var type in java_types) {
153-
string managedKey = type.FullName.Replace ('/', '.');
154-
string javaKey = JavaNativeTypeManager.ToJniName (type).Replace ('/', '.');
155-
156-
acw_map.WriteLine ("{0};{1}", type.GetPartialAssemblyQualifiedName (), javaKey);
157-
158-
TypeDefinition conflict;
159-
if (managed.TryGetValue (managedKey, out conflict)) {
160-
Log.LogWarning (
161-
"Duplicate managed type found! Mappings between managed types and Java types must be unique. " +
162-
"First Type: '{0}'; Second Type: '{1}'.",
163-
conflict.GetAssemblyQualifiedName (),
164-
type.GetAssemblyQualifiedName ());
165-
Log.LogWarning (
166-
"References to the type '{0}' will refer to '{1}'.",
167-
managedKey, conflict.GetAssemblyQualifiedName ());
168-
continue;
169-
}
170-
if (java.TryGetValue (javaKey, out conflict)) {
171-
Log.LogError (
172-
"Duplicate Java type found! Mappings between managed types and Java types must be unique. " +
173-
"First Type: '{0}'; Second Type: '{1}'",
174-
conflict.GetAssemblyQualifiedName (),
175-
type.GetAssemblyQualifiedName ());
176-
keep_going = false;
177-
continue;
148+
var managed = new Dictionary<string, TypeDefinition> (java_types.Length, StringComparer.Ordinal);
149+
var java = new Dictionary<string, TypeDefinition> (java_types.Length, StringComparer.Ordinal);
150+
// Allocate a MemoryStream with a reasonable guess at its capacity
151+
using (var stream = new MemoryStream (java_types.Length * 32))
152+
using (var acw_map = new StreamWriter (stream)) {
153+
foreach (var type in java_types) {
154+
string managedKey = type.FullName.Replace ('/', '.');
155+
string javaKey = JavaNativeTypeManager.ToJniName (type).Replace ('/', '.');
156+
157+
acw_map.Write (type.GetPartialAssemblyQualifiedName ());
158+
acw_map.Write (';');
159+
acw_map.Write (javaKey);
160+
acw_map.WriteLine ();
161+
162+
TypeDefinition conflict;
163+
if (managed.TryGetValue (managedKey, out conflict)) {
164+
Log.LogWarning (
165+
"Duplicate managed type found! Mappings between managed types and Java types must be unique. " +
166+
"First Type: '{0}'; Second Type: '{1}'.",
167+
conflict.GetAssemblyQualifiedName (),
168+
type.GetAssemblyQualifiedName ());
169+
Log.LogWarning (
170+
"References to the type '{0}' will refer to '{1}'.",
171+
managedKey, conflict.GetAssemblyQualifiedName ());
172+
continue;
173+
}
174+
if (java.TryGetValue (javaKey, out conflict)) {
175+
Log.LogError (
176+
"Duplicate Java type found! Mappings between managed types and Java types must be unique. " +
177+
"First Type: '{0}'; Second Type: '{1}'",
178+
conflict.GetAssemblyQualifiedName (),
179+
type.GetAssemblyQualifiedName ());
180+
success = false;
181+
continue;
182+
}
183+
184+
managed.Add (managedKey, type);
185+
java.Add (javaKey, type);
186+
187+
acw_map.Write (managedKey);
188+
acw_map.Write (';');
189+
acw_map.Write (javaKey);
190+
acw_map.WriteLine ();
191+
192+
acw_map.Write (JavaNativeTypeManager.ToCompatJniName (type).Replace ('/', '.'));
193+
acw_map.Write (';');
194+
acw_map.Write (javaKey);
195+
acw_map.WriteLine ();
178196
}
179-
managed.Add (managedKey, type);
180-
java.Add (javaKey, type);
181-
acw_map.WriteLine ("{0};{1}", managedKey, javaKey);
182-
acw_map.WriteLine ("{0};{1}", JavaNativeTypeManager.ToCompatJniName (type).Replace ('/', '.'), javaKey);
183-
}
184-
185-
acw_map.Close ();
186-
187-
//The previous steps found an error, so we must abort and not generate any further output
188-
//We must do so subsequent unchanged builds fail too.
189-
if (!keep_going) {
190-
File.Delete (temp_map_file);
191-
return;
192-
}
193-
194-
MonoAndroidHelper.CopyIfChanged (temp_map_file, AcwMapFile);
195-
196-
try { File.Delete (temp_map_file); } catch (Exception) { }
197-
198-
// Only overwrite files if the contents actually changed
199-
foreach (var file in Directory.GetFiles (temp, "*", SearchOption.AllDirectories)) {
200-
var dest = Path.GetFullPath (Path.Combine (OutputDirectory, "src", file.Substring (temp.Length + 1)));
201197

202-
MonoAndroidHelper.CopyIfChanged (file, dest);
198+
acw_map.Flush ();
199+
MonoAndroidHelper.CopyIfStreamChanged (stream, AcwMapFile);
203200
}
204201

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

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

222-
var temp_manifest = Path.Combine (temp, "AndroidManifest.xml");
223-
var real_manifest = Path.GetFullPath (MergedAndroidManifestOutput);
219+
using (var stream = new MemoryStream ()) {
220+
manifest.Save (stream);
224221

225-
manifest.Save (temp_manifest);
226-
227-
// Only write the new manifest if it actually changed
228-
MonoAndroidHelper.CopyIfChanged (temp_manifest, real_manifest);
222+
// Only write the new manifest if it actually changed
223+
MonoAndroidHelper.CopyIfStreamChanged (stream, MergedAndroidManifestOutput);
224+
}
229225

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

234230
foreach (var provider in additionalProviders) {
235-
var temp_provider = Path.Combine (temp, provider + ".java");
236-
File.WriteAllText (temp_provider, providerTemplate.Replace ("MonoRuntimeProvider", provider));
237-
var real_provider_dir = Path.GetFullPath (Path.Combine (OutputDirectory, "src", "mono"));
238-
Directory.CreateDirectory (real_provider_dir);
239-
var real_provider = Path.Combine (real_provider_dir, provider + ".java");
240-
MonoAndroidHelper.CopyIfChanged (temp_provider, real_provider);
231+
var contents = providerTemplate.Replace ("MonoRuntimeProvider", provider);
232+
var real_provider = Path.Combine (OutputDirectory, "src", "mono", provider + ".java");
233+
MonoAndroidHelper.CopyIfStringChanged (contents, real_provider);
241234
}
242235

243236
// Create additional application java sources.
244-
245-
Action<string,string,string,Func<string,string>> save = (resource, filename, destDir, applyTemplate) => {
246-
string temp_file = Path.Combine (temp, filename);
247-
string template = applyTemplate (new StreamReader (typeof (GenerateJavaStubs).Assembly.GetManifestResourceStream (resource)).ReadToEnd ());
248-
File.WriteAllText (temp_file, template);
249-
Directory.CreateDirectory (destDir);
250-
var real_file = Path.Combine (destDir, filename);
251-
MonoAndroidHelper.CopyIfChanged (temp_file, real_file);
252-
};
253-
254237
StringWriter regCallsWriter = new StringWriter ();
255238
regCallsWriter.WriteLine ("\t\t// Application and Instrumentation ACWs must be registered first.");
256239
foreach (var type in java_types) {
@@ -262,17 +245,28 @@ void Run (DirectoryAssemblyResolver res)
262245
}
263246
regCallsWriter.Close ();
264247

265-
var real_app_dir = Path.GetFullPath (Path.Combine (OutputDirectory, "src", "mono", "android", "app"));
248+
var real_app_dir = Path.Combine (OutputDirectory, "src", "mono", "android", "app");
266249
string applicationTemplateFile = "ApplicationRegistration.java";
267-
save (applicationTemplateFile, applicationTemplateFile, real_app_dir,
250+
SaveResource (applicationTemplateFile, applicationTemplateFile, real_app_dir,
268251
template => template.Replace ("// REGISTER_APPLICATION_AND_INSTRUMENTATION_CLASSES_HERE", regCallsWriter.ToString ()));
269252

270253
// Create NotifyTimeZoneChanges java sources.
271254
string notifyTimeZoneChangesFile = "NotifyTimeZoneChanges.java";
272-
save (notifyTimeZoneChangesFile, notifyTimeZoneChangesFile, real_app_dir, template => template);
273-
274-
// Delete our temp directory
275-
try { Directory.Delete (temp, true); } catch (Exception) { }
255+
SaveResource (notifyTimeZoneChangesFile, notifyTimeZoneChangesFile, real_app_dir, template => template);
256+
}
257+
258+
string GetResource <T> (string resource)
259+
{
260+
using (var stream = typeof (T).Assembly.GetManifestResourceStream (resource))
261+
using (var reader = new StreamReader (stream))
262+
return reader.ReadToEnd ();
263+
}
264+
265+
void SaveResource (string resource, string filename, string destDir, Func<string, string> applyTemplate)
266+
{
267+
string template = GetResource<GenerateJavaStubs> (resource);
268+
template = applyTemplate (template);
269+
MonoAndroidHelper.CopyIfStringChanged (template, Path.Combine (destDir, filename));
276270
}
277271

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

288282
void UpdateWhenChanged (string path, Action<Stream> generator)
289283
{
290-
var np = path + ".new";
291-
using (var o = File.OpenWrite (np))
292-
generator (o);
293-
MonoAndroidHelper.CopyIfChanged (np, path);
294-
File.Delete (np);
284+
using (var stream = new MemoryStream ()) {
285+
generator (stream);
286+
MonoAndroidHelper.CopyIfStreamChanged (stream, path);
287+
}
295288
}
296289
}
297290
}

‎src/Xamarin.Android.Build.Tasks/Tasks/GenerateResourceDesigner.cs‎

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -239,12 +239,8 @@ private void WriteFile (string file, CodeTypeDeclaration resources, string langu
239239
}
240240
}
241241
}
242-
243-
var temp_o = Path.Combine (Path.GetDirectoryName (file), "__" + Path.GetFileName (file) + ".new");
244-
using (TextWriter o = File.CreateText (temp_o))
245-
o.Write (code);
246-
MonoAndroidHelper.CopyIfChanged (temp_o, file);
247-
try { File.Delete (temp_o); } catch (Exception) { }
242+
243+
MonoAndroidHelper.CopyIfStringChanged (code, file);
248244
}
249245

250246
private void AddRename (string android, string user)

0 commit comments

Comments
 (0)