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] implement a MemoryStreamPool #4251

Merged
merged 2 commits into from
Feb 19, 2020

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Feb 11, 2020

Context: https://docs.microsoft.com/dotnet/api/system.buffers.arraypool-1

When using MemoryStream, it is better to reuse a MemoryStream
instance rather than creating new ones, such as in a loop:

using (var memoryStream = new MemoryStream ())
    foreach (var foo in bar) {
        //Reset for reuse
        memoryStream.SetLength (0);
        // Use the stream
    }
}

The SetLength(0) call preserves the underlying byte[] and just
sets _length and _position fields. Subsequent writes don't need to
allocate another byte[].

https://github.com/mono/mono/blob/eb85a108a33ba86ffd184689b62ac1f7250c9818/mcs/class/referencesource/mscorlib/system/io/memorystream.cs#L527-L548

To make this pattern even better, we can model after
System.Buffers.ArrayPool and reuse MemoryStream objects throughout
the entire build.

var memoryStream = MemoryStreamPool.Shared.Rent ();
try {
    // Use the stream
} finally {
    MemoryStreamPool.Shared.Return (memoryStream);
}

Note that you will have to take special care to not dispose the
MemoryStream. If we Return a disposed MemoryStream that would be
bad news!

In many cases a StreamWriter or TextWriter are wrapped around a
MemoryStream, so we could implement a convenience method:

using (var writer = MemoryStreamPool.Shared.CreateStreamWriter ()) {
    // Use the writer
    writer.Flush ();
    MonoAndroidHelper.CopyIfStreamChanged (writer.BaseStream, path);
}

In this case writer is a special StreamWriter that returns the
underlying MemoryStream when the writer is disposed. It also takes
care of not disposing the MemoryStream for you.

Results

The reuse of MemoryStream impacts the entire build. Testing on
macOS, a build of the Xamarin.Forms integration project:

./bin/Release/bin/xabuild tests/Xamarin.Forms-Performance-Integration/Droid/Xamarin.Forms.Performance.Integration.Droid.csproj /restore

If I drill down to some of the specific targets that would be affected
here:

Before:
1618 ms  _GenerateJavaStubs                         1 calls
1656 ms  _ResolveLibraryProjectImports              1 calls
  40 ms  _GeneratePackageManagerJava                1 calls
After:
1568 ms  _GenerateJavaStubs                         1 calls
1376 ms  _ResolveLibraryProjectImports              1 calls
  36 ms  _GeneratePackageManagerJava                1 calls

This seems like we could save ~334ms on incremental builds where these
targets run. An example would be an incremental build where
MainActivity.cs changed.

If I compare the memory usage with the Mono profiler:

Before:
Allocation summary
    Bytes      Count  Average Type name
166246184      37846     4392 System.Byte[]
    82480       1031       80 System.IO.MemoryStream

After:
Allocation summary
    Bytes      Count  Average Type name
157191784      37794     4159 System.Byte[]
    77520        969       80 System.IO.MemoryStream

It seems like we created ~62 less MemoryStream in this build and
saved ~9,059,360 bytes of allocations.

After a few runs, I found I could drop the numbers on some of the
times in MSBuildDeviceIntegration.csv. The times seemed to improve a
bit for incremental builds where a lot is happening--such as when the
.csproj changes. Some of the time improved is likely other changes
around <ResolveAssemblies/> or <ResolveAssemblyReference/> such as
97d250b or 1e96c79.

General Refactoring

Any usage of new Utf8Encoding(false) I moved to a static
UTF8withoutBOM field.

The method Generator.CreateJavaSources had eight parameters! I moved
it to be an instance method on <GenerateJavaStubs/>, which could use
the properties of the task. This reduced it to one parameter.

@jonathanpeppers jonathanpeppers force-pushed the memorystreampool branch 2 times, most recently from df6dc2f to b30376b Compare February 12, 2020 21:07
Context: https://docs.microsoft.com/dotnet/api/system.buffers.arraypool-1

When using `MemoryStream`, it is better to reuse a `MemoryStream`
instance rather than creating new ones, such as in a loop:

    using (var memoryStream = new MemoryStream ())
        foreach (var foo in bar) {
            //Reset for reuse
            memoryStream.SetLength (0);
            // Use the stream
        }
    }

The `SetLength(0)` call preserves the underlying `byte[]` and just
sets `_length` and `_position` fields. Subsequent writes don't need to
allocate another `byte[]`.

https://github.com/mono/mono/blob/eb85a108a33ba86ffd184689b62ac1f7250c9818/mcs/class/referencesource/mscorlib/system/io/memorystream.cs#L527-L548

To make this pattern even better, we can model after
`System.Buffers.ArrayPool` and reuse `MemoryStream` objects throughout
the entire build.

    var memoryStream = MemoryStreamPool.Shared.Rent ();
    try {
        // Use the stream
    } finally {
        MemoryStreamPool.Shared.Return (memoryStream);
    }

Note that you will have to take special care to not dispose the
`MemoryStream`. If we `Return` a disposed `MemoryStream` that would be
bad news!

In many cases a `StreamWriter` or `TextWriter` are wrapped around a
`MemoryStream`, so we could implement a convenience method:

    using (var writer = MemoryStreamPool.Shared.CreateStreamWriter ()) {
        // Use the writer
        writer.Flush ();
        MonoAndroidHelper.CopyIfStreamChanged (writer.BaseStream, path);
    }

In this case `writer` is a special `StreamWriter` that returns the
underlying `MemoryStream` when the writer is disposed. It also takes
care of *not* disposing the `MemoryStream` for you.

~~ Results ~~

The reuse of `MemoryStream` impacts the entire build. Testing on
macOS, a build of the Xamarin.Forms integration project:

    ./bin/Release/bin/xabuild tests/Xamarin.Forms-Performance-Integration/Droid/Xamarin.Forms.Performance.Integration.Droid.csproj /restore

If I drill down to some of the specific targets that would be affected
here:

    Before:
    1618 ms  _GenerateJavaStubs                         1 calls
    1656 ms  _ResolveLibraryProjectImports              1 calls
      40 ms  _GeneratePackageManagerJava                1 calls
    After:
    1568 ms  _GenerateJavaStubs                         1 calls
    1376 ms  _ResolveLibraryProjectImports              1 calls
      36 ms  _GeneratePackageManagerJava                1 calls

This seems like we could save ~334ms on incremental builds where these
targets run. An example would be an incremental build where
`MainActivity.cs` changed.

If I compare the memory usage with the Mono profiler:

    Before:
    Allocation summary
        Bytes      Count  Average Type name
    166246184      37846     4392 System.Byte[]
        82480       1031       80 System.IO.MemoryStream

    After:
    Allocation summary
        Bytes      Count  Average Type name
    157191784      37794     4159 System.Byte[]
        77520        969       80 System.IO.MemoryStream

It seems like we created ~62 less `MemoryStream` in this build and
saved ~9,059,360 bytes of allocations.

After a few runs, I found I could drop the numbers on some of the
times in `MSBuildDeviceIntegration.csv`. The times seemed to improve a
bit for incremental builds where a lot is happening--such as when the
`.csproj` changes. Some of the time improved is likely other changes
around `<ResolveAssemblies/>` or `<ResolveAssemblyReference/>` such as
97d250b or 1e96c79.

~~ General Refactoring ~~

Any usage of `new Utf8Encoding(false)` I moved to a `static`
`UTF8withoutBOM` field.

The method `Generator.CreateJavaSources` had eight parameters! I moved
it to be an instance method on `<GenerateJavaStubs/>`, which could use
the properties of the task. This reduced it to one parameter.
/// <summary>
/// Creates a StreamWriter that uses the underlying MemoryStreamPool. Calling Dispose() will Return() the MemoryStream.
/// </summary>
public StreamWriter CreateStreamWriter () => CreateStreamWriter (Encoding.Default);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense to use MonoAndroidHelper.UTF8withoutBOM? We only ever use Encoding.Default once, in a unit test:

% git grep Encoding.Default src/Xamarin.Android.Build.Tasks 
src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/Utilities/FilesTests.cs:              Stream NewStream (string contents) => new MemoryStream (Encoding.Default.GetBytes (contents));

vs. use of new UTF8Encoding, excluding unit tests:

% git grep 'new.*UTF8Encoding' src/Xamarin.Android.Build.Tasks | grep -v Tests/
src/Xamarin.Android.Build.Tasks/Tasks/Aot.cs:			using (var sw = new StreamWriter (responseFile, append: false, encoding: new UTF8Encoding (encoderShouldEmitUTF8Identifier: false))) {
src/Xamarin.Android.Build.Tasks/Tasks/ClassParse.cs:						encoding: new UTF8Encoding (encoderShouldEmitUTF8Identifier: false))) {
src/Xamarin.Android.Build.Tasks/Tasks/CompileToDalvik.cs:					encoding: new UTF8Encoding (encoderShouldEmitUTF8Identifier: false))) {
src/Xamarin.Android.Build.Tasks/Tasks/GenerateLibraryResources.cs:		static readonly Encoding Encoding = new UTF8Encoding (encoderShouldEmitUTF8Identifier: false);
src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs:				var utf8Encoding = new UTF8Encoding (false);
src/Xamarin.Android.Build.Tasks/Tasks/JavaCompileToolTask.cs:						encoding:new UTF8Encoding (encoderShouldEmitUTF8Identifier:false))) {
src/Xamarin.Android.Build.Tasks/Utilities/ManifestDocument.cs:			using (var file = new StreamWriter (filename, append: false, encoding: new UTF8Encoding (false)))
src/Xamarin.Android.Build.Tasks/Utilities/ManifestDocument.cs:			using (var file = new StreamWriter (stream, new UTF8Encoding (false), bufferSize: 1024, leaveOpen: true))
src/Xamarin.Android.Build.Tasks/Utilities/TypeMapGenerator.cs:			outputEncoding = new UTF8Encoding (false);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the default could use Utf8withoutBOM. Most of the time we generate java-related files.

@jonpryor jonpryor merged commit 87ee20c into dotnet:master Feb 19, 2020
@jonathanpeppers jonathanpeppers deleted the memorystreampool branch February 19, 2020 14:33
jonpryor pushed a commit that referenced this pull request Feb 21, 2020
Context: https://docs.microsoft.com/dotnet/api/system.buffers.arraypool-1

When using `MemoryStream`, it is better to reuse a `MemoryStream`
instance rather than creating new ones, such as in a loop:

	using (var memoryStream = new MemoryStream ())
	    foreach (var foo in bar) {
	        //Reset for reuse
	        memoryStream.SetLength (0);
	        // Use the stream
	    }
	}

The `SetLength(0)` call preserves the underlying `byte[]` and just
sets the `_length` and `_position` fields.  Subsequent writes don't
need to allocate another `byte[]`:

	https://github.com/mono/mono/blob/eb85a108a33ba86ffd184689b62ac1f7250c9818/mcs/class/referencesource/mscorlib/system/io/memorystream.cs#L527-L548

To make this pattern even better, we can model after
`System.Buffers.ArrayPool` and reuse `MemoryStream` objects throughout
the entire build.

	var memoryStream = MemoryStreamPool.Shared.Rent ();
	try {
	    // Use the stream
	} finally {
	    MemoryStreamPool.Shared.Return (memoryStream);
	}

Note that you will have to take special care to not dispose the
`MemoryStream`.  If we `Return()` a disposed `MemoryStream`, that
would be bad news!

In many cases a `StreamWriter` or `TextWriter` are wrapped around a
`MemoryStream`, so we implement a convenience method:

	using (var writer = MemoryStreamPool.Shared.CreateStreamWriter ()) {
	    // Use the writer
	    writer.Flush ();
	    MonoAndroidHelper.CopyIfStreamChanged (writer.BaseStream, path);
	}

In this case `writer` is a special `StreamWriter` that returns the
underlying `MemoryStream` when the writer is disposed.  It also takes
care of *not* disposing the `MemoryStream` for you.

~~ Results ~~

The reuse of `MemoryStream` impacts the entire build.  Testing on
macOS, a build of the Xamarin.Forms integration project:

	./bin/Release/bin/xabuild tests/Xamarin.Forms-Performance-Integration/Droid/Xamarin.Forms.Performance.Integration.Droid.csproj /restore

Some of the specific targets that would be affected:

  * Before:

        1618 ms  _GenerateJavaStubs                         1 calls
        1656 ms  _ResolveLibraryProjectImports              1 calls
          40 ms  _GeneratePackageManagerJava                1 calls

  * After:

        1568 ms  _GenerateJavaStubs                         1 calls
        1376 ms  _ResolveLibraryProjectImports              1 calls
          36 ms  _GeneratePackageManagerJava                1 calls

This seems like we could save ~334ms on incremental builds where
these targets run.  An example would be an incremental build where
`MainActivity.cs` changed.

If I compare the memory usage with the Mono profiler:

  * Before:

        Allocation summary
            Bytes      Count  Average Type name
        166246184      37846     4392 System.Byte[]
            82480       1031       80 System.IO.MemoryStream

  * After:

        Allocation summary
            Bytes      Count  Average Type name
        157191784      37794     4159 System.Byte[]
            77520        969       80 System.IO.MemoryStream

It seems like we created ~62 fewer `MemoryStream` in this build and
saved ~9,059,360 bytes of allocations.

After a few runs, I found I could drop the numbers on some of the
times in `MSBuildDeviceIntegration.csv`.  The times seemed to improve
a bit for incremental builds where a lot is happening, such as when
the `.csproj` changes.  Some of the time improved is likely other
changes around `<ResolveAssemblies/>` or `<ResolveAssemblyReference/>`
such as 97d250b or 1e96c79.

~~ General Refactoring ~~

Any usage of `new Utf8Encoding(false)` I moved to a `static`
`MonoAndroidHelper.UTF8withoutBOM` field.

The method `Generator.CreateJavaSources()` had eight parameters!
I moved it to be an instance method on `<GenerateJavaStubs/>`, which
could use the properties of the task, reducing it to one parameter.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 28, 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.

5 participants