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

[XABT] Use ZipArchive to build APKs. #9623

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[XABT] Use ZipArchive to build APKs. #9623

wants to merge 2 commits into from

Conversation

jpobst
Copy link
Contributor

@jpobst jpobst commented Dec 16, 2024

It would appear we no longer require features not available in System.IO.Compression.ZipArchive in order to build .apk/.aab files. Additionally, it appears that ZipArchive is noticeably faster than our current usage of LibZipSharp. Switch the BuildArchive task to preferring ZipArchive over LibZipSharp when possible for building APKs. The LibZipSharp implementation is maintained as a fallback and in case future new .apk/.aab requirements necessitate its use.

Archive sizes for .apk increase 1.3%-2.7% which seems acceptable. .aab sizes remain roughly the same, likely because bundletool repacks them.

Implementation Notes

  • netstandard2.0 does not expose API for examining a CRC or CompressionMethod of an existing entry in a Zip file, though both net472 and net9.0 have private fields. As such, we use reflection to access these private fields. If the runtime we are using does not have these fields, we will fall back to using LibZipSharp as we do now.
  • Abstract our required Zip API to IZipArchive so that we can switch between System.IO.Compression.ZipFile and LibZipSharp as needed.
  • Can force fallback to LibZipSharp with $(_AndroidUseLibZipSharp)=true.

Performance

Measurements of the BuildArchive task when using the android template for initial and incremental build scenarios.

Debug - FastDev

Scenario main This PR
Full 2.428 s 339 ms
NoChanges not run not run
ChangeResource 34 ms 19 ms
AddResource 23 ms 17 ms
ChangeCSharp not run not run
ChangeCSharpJLO not run not run
Archive Size 5,390,140 bytes 5,537,596 bytes

Debug - EmbedAssembliesInApk

Scenario main This PR
Full 34.856 s 4.313 s
NoChanges not run not run
ChangeResource 33.385 s 4.165 s
AddResource 32.206 s 3.963 s
ChangeCSharp 32.060 s 3.979 s
ChangeCSharpJLO 33.161 s 3.997 s
Archive Size 76,653,152 bytes 77,710,097 bytes

Release

Scenario main This PR
Full 2.195 s 387 ms
NoChanges not run not run
ChangeResource 134 ms 73 ms
AddResource 685 ms 182 ms
ChangeCSharp 705 ms 142 ms
ChangeCSharpJLO 703 ms 149 ms
Archive Size 6,917,153 bytes 6,917,319 bytes

CI build that falls back to LibZipSharp to ensure it still passes our tests: https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=10720142

@jpobst jpobst marked this pull request as ready for review December 17, 2024 17:29
@jpobst jpobst requested review from jonpryor and grendello December 17, 2024 17:29
Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Can we also make a backup MSBuild property to fallback to libZipSharp? Something private like $(_AndroidUseLibZipSharp)?

This is just in case something goes wrong in a release, we can give a customer this property to work around something broken.

@dellis1972
Copy link
Contributor

One of the reasons we didn't use System.IO.Compression was because it IGNORED per file compression settings it also failed to actually STORE things when NoCompression was defined (it would always compress files), this would break out native library support in all sorts of weird ways.

If the plan is to use this when we are going to need to make sure the test coverage is making sure we can add files which have no compression and that they are actually stored as such.

//cc @grendello

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Dec 17, 2024

@dellis1972 I think he figured out how to set the compression method now:

netstandard2.0 does not expose API for examining a CRC or CompressionMethod of an existing entry in a Zip file, though both net472 and net9.0 have private fields. As such, we use reflection to access these private fields. If the runtime we are using does not have these fields, we will fall back to using LibZipSharp as we do now.

@jpobst
Copy link
Contributor Author

jpobst commented Dec 17, 2024

One of the reasons we didn't use System.IO.Compression was because it IGNORED per file compression settings it also failed to actually STORE things when NoCompression was defined (it would always compress files), this would break out native library support in all sorts of weird ways.

Thanks for the context. I'll do some testing around NoCompression to verify what .NET actually does.

@jonathanpeppers
Copy link
Member

@jpobst it might be enough to verify in some of our build tests that the compression method is correct.

We have a couple like this:

using (var baseApk = ZipArchive.Open (stream)) {
foreach (var file in baseApk) {
foreach (var ext in uncompressed) {
if (file.FullName.EndsWith (ext, StringComparison.OrdinalIgnoreCase)) {
Assert.AreEqual (CompressionMethod.Store, file.CompressionMethod, $"{file.FullName} should be uncompressed!");
}
}
}
}

@jpobst
Copy link
Contributor Author

jpobst commented Dec 17, 2024

I believe I see the issue that @dellis1972 is referring to.

Building with NoCompression on .NET 9 looks correct:

  • Size matches Packed Size
  • Method is Store

image

However, using NoCompression on .NET Framework 4.7.2:

  • The contents are "uncompressed" but the sizes are slightly off
  • Method is Deflate

image

It's like it's running it through the compression algorithm with a compression level of 0 instead of placing it unmodified inside the archive.

EDIT: Indeed it seems like this was the original behavior and it was fixed in .NET Core 3.0: dotnet/runtime#26185

I think we have 2 options here:

  • Use LibZipSharp when running inside VS Win (.NET Framework).
  • Move the compression out of process so we can always run on .NET "Core".

@jonathanpeppers
Copy link
Member

Could we include the source from the “good” System.IO.Compression in src-ThirdParty? Then we could also avoid System.Reflection by making some new public member.

@jpobst
Copy link
Contributor Author

jpobst commented Dec 17, 2024

Could we include the source from the “good” System.IO.Compression in src-ThirdParty? Then we could also avoid System.Reflection by making some new public member.

I think the biggest issue is we need it to run on netstandard2.0 in order to be usable by VS MSBuild. It is an unknown, but this could be non-trivial to port from net9.0. Also, the modern .NET version uses zlib, which .NET redistributes on Windows because it isn't provided by the OS, so we would be on the hook for that too.

I think we would likely be venturing into undesirable territory trying to ship our own.


If we moved it out-of-process, we could eliminate the CRC reflection because a new property exists. There still isn't a property for the compression method, but we could possibly apply some pressure on this issue for .NET 10.

(We use the CRC and CompressionMethod to check if we need to update existing entries in an archive during incremental builds.)

@jonathanpeppers
Copy link
Member

jonathanpeppers commented Dec 18, 2024

modern .NET version uses zlib,

Yeah, so I wouldn't try to bring their code in.

Maybe out-of-process is a good option? They are doing this for components inside Visual Studio as well. A new console app could do the work of the MSBuild task, and could always run under .NET 10+ and not .NET framework at all.

@samhouts samhouts requested a review from Copilot December 19, 2024 19:29

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets: Language not supported
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants