-
Notifications
You must be signed in to change notification settings - Fork 564
[Xamarin.Android.Build.Tasks] no temp files in GenerateJavaStubs #2535
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
Merged
jonpryor
merged 1 commit into
dotnet:master
from
jonathanpeppers:generatejavastubs-tempfiles
Dec 19, 2018
Merged
[Xamarin.Android.Build.Tasks] no temp files in GenerateJavaStubs #2535
jonpryor
merged 1 commit into
dotnet:master
from
jonathanpeppers:generatejavastubs-tempfiles
Dec 19, 2018
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
Logs from third run: logs.zip |
Therzok
reviewed
Dec 17, 2018
Therzok
reviewed
Dec 17, 2018
Therzok
reviewed
Dec 17, 2018
Therzok
reviewed
Dec 17, 2018
Therzok
reviewed
Dec 17, 2018
893574f to
a3d4ee5
Compare
Context: dotnet#2505 This is based on dotnet#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.
a3d4ee5 to
c956724
Compare
Member
Author
|
build |
Member
Author
|
The test failure just seems to say this from the build log: I'm going to retry it. |
jonathanpeppers
added a commit
that referenced
this pull request
Jan 2, 2019
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.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: #2505
This is based on #2505, but I incorporated the java.interop changes so
GenerateJavaStubsuses no temp files at all.In many places throughout the Xamarin.Android build, we have a pattern
of:
MonoAndroidHelper.CopyIfChangedto put the file in thedestination. This reads both files, doing a hash comparison before
deciding to write or not.
Sometimes the temp file is actually in
%TEMP%, but other times weappend
.newto the destination file. The case of.newcan collideif 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:
MonoAndroidHelper.CopyStreamIfChangedmethod`.This has several benefits:
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 aStreamin memoryFiles.HasBytesChanged- to compare abyte[]in memoryFiles.CopyIfStreamChangedFiles.CopyIfStringChanged- some cases we have astringFiles.CopyIfBytesChanged- this supportsstringMonoAndroidHelper.CopyIfStreamChangedMonoAndroidHelper.CopyIfStringChangedMonoAndroidHelper.CopyIfBytesChangedI changed the following MSBuild tasks, mostly to test out the new
behavior:
GenerateResourceDesignerwas using a.newfile.GenerateJavaStubswas using temp files in many places. I was ableto fix up all of these.
ManifestDocumentnow has an overload forSavethat takes aStreamGeneratornow usesCopyIfStreamChangeda newGetDestinationPathmethod from java.interop. It reuses a singleMemoryStream, and I moved theGenerateJavaSourcemethod intoCreateJavaSourcesfor simplicity.I made other general refactoring in
GenerateJavaStubs:temp_map_file, wecan return earlier if
Generator.CreateJavaSourcesfails.GetResource<T>method, cleans up the places reading fromEmbeddedResourcefiles. I also changed it up to properlyDisposethings.
saveanonymous method/delegate should just be aSaveResourceregular method.
Path.GetFullPathunecessarily. Sincethis method accesses the file system, we should skip it unless the
full path is actually needed.
StringWriterandstring.Format.StringComparer.Ordinalwhen creatingdictionaries: https://www.dotnetperls.com/dictionary-stringcomparer
MemoryStreamwithjava_types.Length * 32I 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 inthis repo:
GenerateJavaStubsis now about 250ms faster on initial build and150ms faster on incremental builds.
I could not see a difference in
GenerateResourceDesigner, likelysince it wrote only a single temp file.
Next steps would be to make changes in other MSBuild tasks as well.