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

Bump to libZipSharp 1.0.10 #4320

Merged
merged 1 commit into from
Mar 6, 2020

Conversation

jonathanpeppers
Copy link
Member

@jonathanpeppers jonathanpeppers commented Feb 25, 2020

Context: dotnet/android-libzipsharp#50

When profiling xamarin-android builds with the Mono profiler, I noticed
libZipSharp is one of the biggest allocators of byte[]:

Allocation summary
    Bytes      Count  Average Type name
251089192      57759     4347 System.Byte[]
    94852584 bytes from:
        Xamarin.Tools.Zip.ZipArchive:Close ()
        (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_close (intptr)
        (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
        Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
    25673176 bytes from:
        Xamarin.Android.Tasks.ResolveLibraryProjectImports:Extract (System.Collections.Generic.IDictionary`2<string, Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>)
        Xamarin.Android.Tools.Files:ExtractAll (Xamarin.Tools.Zip.ZipArchive,string,System.Action`2<int, int>,System.Func`2<string, string>,System.Func`2<string, bool>)
        Xamarin.Tools.Zip.ZipEntry:Extract (System.IO.Stream)
        Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
    3780312 bytes from:
        Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
        (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_fread (intptr,byte[],ulong)
        (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
        Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)

This seems like a lot of byte[] allocations.

To improve this, I added a <PackageReference/> to System.Buffers
and could make use of ArrayPool.

libZipSharp will likely still allocate some large byte[], but things
should improve because many will be reused.

The changes appear to have saved ~122,523,744 bytes of allocations:

Allocation summary
    Bytes      Count  Average Type name
Before:
251089192      57759     4347 System.Byte[]
After:
128565448      31764     4047 System.Byte[]

I saw build performance improvements for the Xamarin.Forms integration
project, the two tasks heavily using libZipSharp:

Before:
1881 ms  ResolveLibraryProjectImports               1 calls
3406 ms  BuildApk                                   1 calls
After:
1795 ms  ResolveLibraryProjectImports               1 calls
3150 ms  BuildApk                                   1 calls

I would guess this saves ~350ms on an initial build. Incremental builds
won't be allocating byte[] as heavily, but should see some improvement.

I also need to make sure our installers include System.Buffers.dll
alongside everywhere we have a copy of libZipSharp.dll.

@jonathanpeppers
Copy link
Member Author

Things generally seem OK for me locally, but I will need to include System.Buffers.dll in the installer:

System.IO.FileLoadException : Could not load file or assembly 'System.Buffers, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)

@jonpryor
Copy link
Member

Apparently more work is required. BuildAotApplicationAndBundleAndÜmläüts (among many others* is failing:

System.IO.FileLoadException : Could not load file or assembly 'System.Buffers, Version=4.0.3.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)

@jonathanpeppers
Copy link
Member Author

The latest CI build is getting closer, but I think I must have broken something in libZipSharp:

Xamarin.Tools.Zip.ZipIOException : Stream is not a ZIP archive

Looking into it.

jonathanpeppers added a commit to jonathanpeppers/LibZipSharp that referenced this pull request Feb 26, 2020
Context: dotnet/android#4320

After using libZipSharp 1.0.9 in xamarin-android, one of our tests
surfaced a bug:

    Xamarin.Tools.Zip.ZipIOException : Stream is not a ZIP archive

The test created a small text file inside a zip using a `MemoryStream`.

Something like:

    using (var zip = ZipArchive.Create (zipStream)) {
        zip.AddEntry ("foo", "bar", encoding);
    }

In 750c780, there was one mistake:

    case SourceCommand.Write:
        buffer = ArrayPool<byte>.Shared.Rent (length);
        try {
            Marshal.Copy (data, buffer, 0, length);
            stream.Write (buffer, 0, length);
            return buffer.Length; // Whoops!
        } finally {
            ArrayPool<byte>.Shared.Return (buffer);
        }

In the test case above, the size of the buffer will be very small: the
equivalent of `Encoding.UTF8.GetBytes("bar")`. If `ArrayPool` returns
a *larger* buffer than we asked for, the code is actually wrong.

We need to return `length` instead of `buffer.Length`. I would suspect
that almost everything worked with this bug in place, except for this
case. `xaprepare` was able to do its work: downloading and unzipping
the Android SDK/NDK.

I was able to add a unit test showing this problem.
@dellis1972
Copy link
Contributor

We are going to have to use version 1.0.10 as it contains the fix from dotnet/android-libzipsharp#52

@jonathanpeppers jonathanpeppers changed the title Bump to libZipSharp 1.0.9 Bump to libZipSharp 1.0.10 Feb 27, 2020
@brendanzagaeski
Copy link
Contributor

Draft release notes

Here's a candidate release note for this change. Feel free to edit as desired and add it to this pull request. (Additional info about preparing draft release notes can now be found in the release notes README.)

### Build and deployment performance

  * [GitHub PR 4320](https://github.com/xamarin/xamarin-android/pull/4320):
    Bring in an update to LibZipSharp that uses `System.Buffers.ArrayPool`
    instead of `byte[]` to save on allocations.  This reduced the combined time
    for the `ResolveLibraryProjectImports` and `BuildApk` tasks from about 5.3
    seconds to about 5.0 seconds for a small test Xamarin.Forms app on an
    initial clean build.

Context: dotnet/android-libzipsharp#50

When profiling xamarin-android builds with the Mono profiler, I noticed
`libZipSharp` is one of the biggest allocators of `byte[]`:

    Allocation summary
        Bytes      Count  Average Type name
    251089192      57759     4347 System.Byte[]
        94852584 bytes from:
            Xamarin.Tools.Zip.ZipArchive:Close ()
            (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_close (intptr)
            (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
            Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
            (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
            (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
        25673176 bytes from:
            Xamarin.Android.Tasks.ResolveLibraryProjectImports:Extract (System.Collections.Generic.IDictionary`2<string, Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>)
            Xamarin.Android.Tools.Files:ExtractAll (Xamarin.Tools.Zip.ZipArchive,string,System.Action`2<int, int>,System.Func`2<string, string>,System.Func`2<string, bool>)
            Xamarin.Tools.Zip.ZipEntry:Extract (System.IO.Stream)
            Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
            (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
            (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
        3780312 bytes from:
            Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
            (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_fread (intptr,byte[],ulong)
            (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
            Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
            (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
            (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)

This seems like *a lot* of `byte[]` allocations.

To improve this, I added a `<PackageReference/>` to `System.Buffers`
and could make use of `ArrayPool`.

`libZipSharp` will likely still allocate some large `byte[]`, but things
should improve because many will be reused.

The changes appear to have saved ~122,523,744 bytes of allocations:

    Allocation summary
        Bytes      Count  Average Type name
    Before:
    251089192      57759     4347 System.Byte[]
    After:
    128565448      31764     4047 System.Byte[]

I saw build performance improvements for the Xamarin.Forms integration
project, the two tasks heavily using `libZipSharp`:

    Before:
    1881 ms  ResolveLibraryProjectImports               1 calls
    3406 ms  BuildApk                                   1 calls
    After:
    1795 ms  ResolveLibraryProjectImports               1 calls
    3150 ms  BuildApk                                   1 calls

I would guess this saves ~350ms on an initial build. Incremental builds
won't be allocating `byte[]` as heavily, but should see some improvement.

I also need to make sure our installers include `System.Buffers.dll`
alongside everywhere we have a copy of `libZipSharp.dll`.
Copy link
Contributor

@brendanzagaeski brendanzagaeski left a comment

Choose a reason for hiding this comment

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

Release note approved.

@jonpryor jonpryor merged commit 99b7d50 into dotnet:master Mar 6, 2020
@jonathanpeppers jonathanpeppers deleted the libzipsharp-1.0.9 branch March 6, 2020 02:53
jonathanpeppers added a commit to jonathanpeppers/xamarin-android that referenced this pull request Mar 6, 2020
Changes: dotnet/android-libzipsharp@1.0.8...1.0.10

Context: dotnet/android-libzipsharp#50

When profiling xamarin-android builds with the Mono profiler, I
noticed `libZipSharp` is one of the biggest allocators of `byte[]`:

	Allocation summary
	    Bytes      Count  Average Type name
	251089192      57759     4347 System.Byte[]
	    94852584 bytes from:
	        Xamarin.Tools.Zip.ZipArchive:Close ()
	        (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_close (intptr)
	        (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
	        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
	    25673176 bytes from:
	        Xamarin.Android.Tasks.ResolveLibraryProjectImports:Extract (System.Collections.Generic.IDictionary`2<string, Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>)
	        Xamarin.Android.Tools.Files:ExtractAll (Xamarin.Tools.Zip.ZipArchive,string,System.Action`2<int, int>,System.Func`2<string, string>,System.Func`2<string, bool>)
	        Xamarin.Tools.Zip.ZipEntry:Extract (System.IO.Stream)
	        Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
	        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
	        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
	    3780312 bytes from:
	        Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
	        (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_fread (intptr,byte[],ulong)
	        (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
	        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)

This seems like *a lot* of `byte[]` allocations.

To improve this, I added a `@(PackageReference)` to `System.Buffers`
to make use of `ArrayPool`.

`libZipSharp` will likely still allocate some large `byte[]`, but
things should improve because many arrays will be reused.

The changes appear to have saved ~122,523,744 bytes of allocations:

  * Before:

        Allocation summary
            Bytes      Count  Average Type name
        251089192      57759     4347 System.Byte[]

  * After:

        Allocation summary
            Bytes      Count  Average Type name
        128565448      31764     4047 System.Byte[]

I saw build performance improvements for the Xamarin.Forms integration
project, the two tasks heavily using `libZipSharp`:

  * Before:

        1881 ms  ResolveLibraryProjectImports               1 calls
        3406 ms  BuildApk                                   1 calls

  * After:

        1795 ms  ResolveLibraryProjectImports               1 calls
        3150 ms  BuildApk                                   1 calls

I would guess this saves ~350ms on an initial build.  Incremental
builds won't be allocating `byte[]` as heavily, but should see some
improvement.

I also need to make sure our installers include `System.Buffers.dll`
alongside everywhere we have a copy of `libZipSharp.dll`.
jonpryor pushed a commit that referenced this pull request Mar 10, 2020
Changes: dotnet/android-libzipsharp@1.0.8...1.0.10

Context: dotnet/android-libzipsharp#50

When profiling xamarin-android builds with the Mono profiler, I
noticed `libZipSharp` is one of the biggest allocators of `byte[]`:

	Allocation summary
	    Bytes      Count  Average Type name
	251089192      57759     4347 System.Byte[]
	    94852584 bytes from:
	        Xamarin.Tools.Zip.ZipArchive:Close ()
	        (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_close (intptr)
	        (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
	        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
	    25673176 bytes from:
	        Xamarin.Android.Tasks.ResolveLibraryProjectImports:Extract (System.Collections.Generic.IDictionary`2<string, Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>,System.Collections.Generic.ICollection`1<Microsoft.Build.Framework.ITaskItem>)
	        Xamarin.Android.Tools.Files:ExtractAll (Xamarin.Tools.Zip.ZipArchive,string,System.Action`2<int, int>,System.Func`2<string, string>,System.Func`2<string, bool>)
	        Xamarin.Tools.Zip.ZipEntry:Extract (System.IO.Stream)
	        Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
	        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
	        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)
	    3780312 bytes from:
	        Xamarin.Tools.Zip.ZipEntry:DoExtract (intptr,System.IO.Stream,Xamarin.Tools.Zip.EntryExtractEventArgs)
	        (wrapper managed-to-native) Xamarin.Tools.Zip.Native:zip_fread (intptr,byte[],ulong)
	        (wrapper native-to-managed) Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        Xamarin.Tools.Zip.ZipArchive:stream_callback (intptr,intptr,ulong,Xamarin.Tools.Zip.SourceCommand)
	        (wrapper alloc) object:ProfilerAllocVector (intptr,intptr)
	        (wrapper managed-to-native) object:__icall_wrapper_mono_gc_alloc_vector (intptr,intptr,intptr)

This seems like *a lot* of `byte[]` allocations.

To improve this, I added a `@(PackageReference)` to `System.Buffers`
to make use of `ArrayPool`.

`libZipSharp` will likely still allocate some large `byte[]`, but
things should improve because many arrays will be reused.

The changes appear to have saved ~122,523,744 bytes of allocations:

  * Before:

        Allocation summary
            Bytes      Count  Average Type name
        251089192      57759     4347 System.Byte[]

  * After:

        Allocation summary
            Bytes      Count  Average Type name
        128565448      31764     4047 System.Byte[]

I saw build performance improvements for the Xamarin.Forms integration
project, the two tasks heavily using `libZipSharp`:

  * Before:

        1881 ms  ResolveLibraryProjectImports               1 calls
        3406 ms  BuildApk                                   1 calls

  * After:

        1795 ms  ResolveLibraryProjectImports               1 calls
        3150 ms  BuildApk                                   1 calls

I would guess this saves ~350ms on an initial build.  Incremental
builds won't be allocating `byte[]` as heavily, but should see some
improvement.

I also need to make sure our installers include `System.Buffers.dll`
alongside everywhere we have a copy of `libZipSharp.dll`.
@github-actions github-actions bot locked and limited conversation to collaborators Jan 24, 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.

4 participants