-
Notifications
You must be signed in to change notification settings - Fork 529
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
Conversation
Things generally seem OK for me locally, but I will need to include
|
3d2de00
to
67b1b84
Compare
Apparently more work is required.
|
67b1b84
to
d57818a
Compare
The latest CI build is getting closer, but I think I must have broken something in
Looking into it. |
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.
We are going to have to use version 1.0.10 as it contains the fix from dotnet/android-libzipsharp#52 |
379e531
to
bc42606
Compare
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.)
|
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`.
bc42606
to
d5ee61f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release note approved.
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`.
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`.
Context: dotnet/android-libzipsharp#50
When profiling xamarin-android builds with the Mono profiler, I noticed
libZipSharp
is one of the biggest allocators ofbyte[]
:This seems like a lot of
byte[]
allocations.To improve this, I added a
<PackageReference/>
toSystem.Buffers
and could make use of
ArrayPool
.libZipSharp
will likely still allocate some largebyte[]
, but thingsshould improve because many will be reused.
The changes appear to have saved ~122,523,744 bytes of allocations:
I saw build performance improvements for the Xamarin.Forms integration
project, the two tasks heavily using
libZipSharp
: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
.