Skip to content
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

[Xamarin.Android.Build.Tasks] no temp files in GenerateJavaStubs #2535

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Dec 14, 2018

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.

@jonathanpeppers
Copy link
Member Author

Logs from third run: logs.zip

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.
@jonathanpeppers
Copy link
Member Author

build

@jonathanpeppers
Copy link
Member Author

jonathanpeppers commented Dec 18, 2018

The test failure just seems to say this from the build log:

/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/build-tools/scripts/TestApks.targets(189,5):
   Process '/Users/builder/android-toolchain/sdk/platform-tools/adb -s emulator-5570  shell am instrument  -e "loglevel Verbose" -w "Xamarin.Android.Bcl_Tests/xamarin.android.bcltests.NUnitInstrumentation"' failed to exit within the timeout of 1800000ms, killing the process [/Users/builder/jenkins/workspace/xamarin-android-pr-builder-release/xamarin-android/tests/RunApkTests.targets]

I'm going to retry it.

@jonpryor jonpryor merged commit c362fe5 into dotnet:master Dec 19, 2018
@jonathanpeppers jonathanpeppers deleted the generatejavastubs-tempfiles branch December 19, 2018 16:34
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.
@github-actions github-actions bot locked and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants