Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Xamarin.Android.Build.Tasks] don't use so many temp files #2505

Merged
merged 1 commit into from
Dec 9, 2018

Conversation

jonathanpeppers
Copy link
Member

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 the places except where Generator.CreateJavaSources
    writes Java code to an output directory.

I made other general refactoring in GenerateJavaStubs:

  • The top-level temp directory we still use, should be deleted in a
    try-finally!
  • 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.

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. 1587 ms  GenerateJavaStubs                          1 calls
2. 1503 ms  GenerateJavaStubs                          1 calls
3. 1553 ms  GenerateJavaStubs                          1 calls

After (Clean Build):
1. 1424 ms  GenerateJavaStubs                          1 calls
2. 1461 ms  GenerateJavaStubs                          1 calls
3. 1529 ms  GenerateJavaStubs                          1 calls

Before (Incremental):
1. 1775 ms  GenerateJavaStubs                          1 calls
2. 1490 ms  GenerateJavaStubs                          1 calls
3. 1664 ms  GenerateJavaStubs                          1 calls

After (Incremental):
1. 1443 ms  GenerateJavaStubs                          1 calls
2. 1480 ms  GenerateJavaStubs                          1 calls
3. 1589 ms  GenerateJavaStubs                          1 calls

GenerateJavaStubs is now consistently faster, but it's hard to tell
how much. I feel it is likely 100ms better. I have an SSD, so I'd
expect developers with an HDD to see even bigger improvments.

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 Java.Interop so we could get
the same CopyIfStreamChanged behavior for all generated Java
files. There are likely other MSBuild tasks to change as well.

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 the places except where `Generator.CreateJavaSources`
  writes Java code to an output directory.

I made other general refactoring in `GenerateJavaStubs`:

- The top-level `temp` directory we still use, should be deleted in a
  `try-finally`!
- 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.

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. 1587 ms  GenerateJavaStubs                          1 calls
    2. 1503 ms  GenerateJavaStubs                          1 calls
    3. 1553 ms  GenerateJavaStubs                          1 calls

    After (Clean Build):
    1. 1424 ms  GenerateJavaStubs                          1 calls
    2. 1461 ms  GenerateJavaStubs                          1 calls
    3. 1529 ms  GenerateJavaStubs                          1 calls

    Before (Incremental):
    1. 1775 ms  GenerateJavaStubs                          1 calls
    2. 1490 ms  GenerateJavaStubs                          1 calls
    3. 1664 ms  GenerateJavaStubs                          1 calls

    After (Incremental):
    1. 1443 ms  GenerateJavaStubs                          1 calls
    2. 1480 ms  GenerateJavaStubs                          1 calls
    3. 1589 ms  GenerateJavaStubs                          1 calls

`GenerateJavaStubs` is now consistently faster, but it's hard to tell
how much. I feel it is likely 100ms better. I have an SSD, so I'd
expect developers with an HDD to see even bigger improvments.

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 Java.Interop so we could get
the same `CopyIfStreamChanged` behavior for *all* generated Java
files. There are likely other MSBuild tasks to change as well.
@jonathanpeppers jonathanpeppers added the do-not-merge PR should not be merged. label Dec 7, 2018
@jonathanpeppers
Copy link
Member Author

We can wait until after branching to merge this.

@jonathanpeppers
Copy link
Member Author

Actually looks like I could use a TextWriter overload here: https://github.com/xamarin/xamarin-android/blob/24517ef0095dbcb8bdd2e88edb41227215c96b37/src/Xamarin.Android.Build.Tasks/Generator/Generator.cs#L49

So I could remove all the temp files in GenerateJavaStubs, without a Java.Interop change... I'll try it.

@jonathanpeppers
Copy link
Member Author

So I could remove all the temp files in GenerateJavaStubs, without a Java.Interop change... I'll try it.

Nope: d510ca0

Will need to add one public method here in Java.Interop, something like:

public string GetDestinationPath (string outputPath)
{
	string path = outputPath;
	foreach (string dir in package.Split ('.'))
		path = Path.Combine (path, dir);

	if (!Directory.Exists (path))
		Directory.CreateDirectory (path);

	path = Path.Combine (path, name + ".java");
	return path;
}

package is private to JavaCallableWrapperGenerator.

So we can take this PR as a first step, and do more later.

@jonpryor jonpryor merged commit b4c4f66 into dotnet:master Dec 9, 2018
jonpryor added a commit that referenced this pull request Dec 9, 2018
jonpryor added a commit that referenced this pull request Dec 9, 2018
jonathanpeppers added a commit to jonathanpeppers/java.interop that referenced this pull request Dec 10, 2018
Context: dotnet/android#2505 (comment)

In a world order where we are creating less temp files in
Xamarin.Android, it would be nice to leverage an overload of
`JavaCallableWrapperGenerator` that uses a `TextWriter` in memory.

Currently we are doing:

    //jti is a JavaCallableWrapperGenerator instance
    jti.Generate (outputPath);

    //Then later in our MSBuild task
    foreach (var file in Directory.GetFiles (temp, "*", SearchOption.AllDirectories)) {
        ...
        MonoAndroidHelper.CopyIfChanged (file, dest);
    }

So this is slower than it could be:

1. Writes to a bunch of temp files.
2. Recurses the directory where all these temp files exist.
3. Does a hash comparison of the contents of the source and
   destination.
4. Copies the file if needed.
5. Deletes all the temp files.

Instead, we could do something like:

    using (var memoryStream = new MemoryStream ())
    using (var writer = new StreamWriter (memoryStream)) {
        jti.Generate (writer);
        writer.Flush ();
        var path = jti.GetDestinationPath (outputPath);
        MonoAndroidHelper.CopyIfStreamChanged (memoryStream, path);
    }

This is quite a bit streamlined:

1. Generate the file in-memory.
2. Do a hash comparison of the in-memory file versus the destination.
3. Write to the file if needed.

The change we need in Java.Interop here is to add the
`GetDestinationPath` method. Further changes downstream in
xamarin-android will allow us reap the benefits.
jonathanpeppers added a commit to jonathanpeppers/java.interop that referenced this pull request Dec 10, 2018
Context: dotnet/android#2505 (comment)

In a world order where we are creating less temp files in
Xamarin.Android, it would be nice to leverage an overload of
`JavaCallableWrapperGenerator` that uses a `TextWriter` in memory.

Currently we are doing:

    //jti is a JavaCallableWrapperGenerator instance
    jti.Generate (outputPath);

    //Then later in our MSBuild task
    foreach (var file in Directory.GetFiles (temp, "*", SearchOption.AllDirectories)) {
        ...
        MonoAndroidHelper.CopyIfChanged (file, dest);
    }

So this is slower than it could be:

1. Writes to a bunch of temp files.
2. Recurses the directory where all these temp files exist.
3. Does a hash comparison of the contents of the source and
   destination.
4. Copies the file if needed.
5. Deletes all the temp files.

Instead, we could do something like:

    using (var memoryStream = new MemoryStream ())
    using (var writer = new StreamWriter (memoryStream)) {
        jti.Generate (writer);
        writer.Flush ();
        var path = jti.GetDestinationPath (outputPath);
        MonoAndroidHelper.CopyIfStreamChanged (memoryStream, path);
    }

This is quite a bit streamlined:

1. Generate the file in-memory.
2. Do a hash comparison of the in-memory file versus the destination.
3. Write to the file if needed.

The change we need in Java.Interop here is to add the
`GetDestinationPath` method. Further changes downstream in
xamarin-android will allow us reap the benefits.

I also improved the code a bit, no longer using `string.Split`. We can
do a simple `string.Replace` from `.` to `Path.DirectorySeparatorChar`,
and it will allocate less strings.
jonathanpeppers added a commit to jonathanpeppers/java.interop that referenced this pull request Dec 10, 2018
Context: dotnet/android#2505 (comment)

In a world order where we are creating less temp files in
Xamarin.Android, it would be nice to leverage an overload of
`JavaCallableWrapperGenerator` that uses a `TextWriter` in memory.

Currently we are doing:

    //jti is a JavaCallableWrapperGenerator instance
    jti.Generate (outputPath);

    //Then later in our MSBuild task
    foreach (var file in Directory.GetFiles (temp, "*", SearchOption.AllDirectories)) {
        ...
        MonoAndroidHelper.CopyIfChanged (file, dest);
    }

So this is slower than it could be:

1. Writes to a bunch of temp files.
2. Recurses the directory where all these temp files exist.
3. Does a hash comparison of the contents of the source and
   destination.
4. Copies the file if needed.
5. Deletes all the temp files.

Instead, we could do something like:

    using (var memoryStream = new MemoryStream ())
    using (var writer = new StreamWriter (memoryStream)) {
        jti.Generate (writer);
        writer.Flush ();
        var path = jti.GetDestinationPath (outputPath);
        MonoAndroidHelper.CopyIfStreamChanged (memoryStream, path);
    }

This is quite a bit streamlined:

1. Generate the file in-memory.
2. Do a hash comparison of the in-memory file versus the destination.
3. Write to the file if needed.

The change we need in Java.Interop here is to add the
`GetDestinationPath` method. Further changes downstream in
xamarin-android will allow us reap the benefits.

I also improved the code a bit, no longer using `string.Split`. We can
do a simple `string.Replace` from `.` to `Path.DirectorySeparatorChar`,
and it will allocate less strings.
jonpryor pushed a commit to dotnet/java-interop that referenced this pull request Dec 12, 2018
Context: dotnet/android#2505 (comment)

Xamarin.Android is working to reduce the number of temporary files
which are generated during the build, in order to improve build time.
We would like `JavaCallableWrapperGenerator` to assist in that effort.

Currently we are doing:

	// jti is a JavaCallableWrapperGenerator instance
	jti.Generate (outputPath); 

	// Then later in our MSBuild task
	foreach (var file in Directory.GetFiles (temp, "*", SearchOption.AllDirectories)) {
	    ...
	    MonoAndroidHelper.CopyIfChanged (file, dest);
	}

This is slower than it could be:

 1. Writes to a bunch of temp files.
 2. Recurses the directory where all these temp files exist.
 3. Does a hash comparison of the contents of the source and
    destination.
 4. Copies the file if needed.
 5. Deletes all the temp files.

Instead, we want to instead do:

	using (var memoryStream = new MemoryStream ())
	using (var writer       = new StreamWriter (memoryStream)) {
	    jti.Generate (writer);
	    writer.Flush ();
	    var path = jti.GetDestinationPath (outputPath);
	    MonoAndroidHelper.CopyIfStreamChanged (memoryStream, path);
	}

This is more streamlined:

 1. Generate the file in-memory.
 2. Do a hash comparison of the in-memory file vs. the destination.
 3. Write to the file if needed.

The change we need in Java.Interop here is to add the
`JavaCallableWrapperGenerator.GetDestinationPath()` method.  Further
changes downstream in xamarin-android will allow us reap the benefits.

I also improved the code a bit, no longer using `string.Split()`.
We can do a simple `string.Replace()` from `.` to
`Path.DirectorySeparatorChar`, which allocates fewer strings.
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 14, 2018
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.

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 added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 17, 2018
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 added a commit to jonathanpeppers/xamarin-android that referenced this pull request Dec 18, 2018
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.
jonpryor pushed a commit that referenced this pull request Dec 19, 2018
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.
jonathanpeppers added a commit to dotnet/java-interop that referenced this pull request Dec 20, 2018
Context: dotnet/android#2505 (comment)

Xamarin.Android is working to reduce the number of temporary files
which are generated during the build, in order to improve build time.
We would like `JavaCallableWrapperGenerator` to assist in that effort.

Currently we are doing:

	// jti is a JavaCallableWrapperGenerator instance
	jti.Generate (outputPath); 

	// Then later in our MSBuild task
	foreach (var file in Directory.GetFiles (temp, "*", SearchOption.AllDirectories)) {
	    ...
	    MonoAndroidHelper.CopyIfChanged (file, dest);
	}

This is slower than it could be:

 1. Writes to a bunch of temp files.
 2. Recurses the directory where all these temp files exist.
 3. Does a hash comparison of the contents of the source and
    destination.
 4. Copies the file if needed.
 5. Deletes all the temp files.

Instead, we want to instead do:

	using (var memoryStream = new MemoryStream ())
	using (var writer       = new StreamWriter (memoryStream)) {
	    jti.Generate (writer);
	    writer.Flush ();
	    var path = jti.GetDestinationPath (outputPath);
	    MonoAndroidHelper.CopyIfStreamChanged (memoryStream, path);
	}

This is more streamlined:

 1. Generate the file in-memory.
 2. Do a hash comparison of the in-memory file vs. the destination.
 3. Write to the file if needed.

The change we need in Java.Interop here is to add the
`JavaCallableWrapperGenerator.GetDestinationPath()` method.  Further
changes downstream in xamarin-android will allow us reap the benefits.

I also improved the code a bit, no longer using `string.Split()`.
We can do a simple `string.Replace()` from `.` to
`Path.DirectorySeparatorChar`, which allocates fewer strings.
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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
do-not-merge PR should not be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants