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

assemblies.ARCH.blob should be per-ARCH .so files #8168

Closed
jonpryor opened this issue Jul 5, 2023 · 11 comments
Closed

assemblies.ARCH.blob should be per-ARCH .so files #8168

jonpryor opened this issue Jul 5, 2023 · 11 comments
Assignees
Labels
Area: App+Library Build Issues when building Library projects or Application projects.

Comments

@jonpryor
Copy link
Member

jonpryor commented Jul 5, 2023

Context: #8165 (comment)

assemblies.x86.blob and similar are always stored in the assemblies directory, meaning our per-arch assemblies*.blob files are present when downloaded for every architecture. This means that arch-specific .apk files are larger than is necessary.

We should move the per-arch assemblies.*.blob files into per-arch lib/ARCH/libassemblies.so files. This way when a per-arch .apk is produced, assemblies for other architectures can be properly skipped/removed from the .apk.

@jonpryor jonpryor added Area: App+Library Build Issues when building Library projects or Application projects. needs-triage Issues that need to be assigned. labels Jul 5, 2023
@jonathanpeppers
Copy link
Member

Or should there only be 1 set of BCL assemblies instead?

In theory, we could make:

  • Microsoft.NETCore.App.Runtime.Mono.android would contain portable, managed code
  • The 4 Microsoft.NETCore.App.Runtime.Mono.android-RID packs would contain only native libraries

Benefits:

  • dotnet workload install size is smaller
  • We run the linker once
  • Our custom inner/outer build per-RID goes away
  • We'd have only one assemblies.blob file

Drawbacks:

  • We would remove some architecture-specific linker substitutions, so System.Private.CoreLib.dll would be slightly larger. However, we have 4x copies of this file currently, anyway.
  • Some checks would happen at runtime, to decide if things like architecture-specific intrinsics are used?

@mosammo
Copy link

mosammo commented Jul 6, 2023

please note same problem is also in "lib" folder inside the apk which has even larger size for unselected platforms as shown below
image

@jonathanpeppers
Copy link
Member

@mosammo we brought what you noticed here up with Google: google/bundletool#190

There is not a way to split up custom files per-architecture for App Bundles. Our only option appears to rework .NET assemblies to be .so files as this issue mentions.

(Or make it so there are no architecture-specific files at all 😄)

@jonpryor
Copy link
Member Author

jonpryor commented Jul 6, 2023

@mosammo wrote:

please note same problem is also in "lib" folder inside the apk which has even larger size for unselected platforms as shown below

Yes and no: Yes, you are absolutely correct that the .apk contains lib/* directories which increase the size of the .apk and may not be relevant for the target device.

However, "No" because .apk is no longer the preferred App Distribution method, Android App Bundle (.aab) files are. The .aab will be "huge", but the Google Play store will take the .aab file as input, and strip out entries that are not relevant for the target device.

For example, you upload a .aab containing all ABIs to the Google Play store. When you download the app onto a Pixel 6, the downloaded .apk will contain only arm64-v8a; it will not contain lib/x86, lib/x86_64, or lib/armeabi-v7a. Similarly, Android Resources will contain only resources applicable to the target device, e.g. no "tablet sized" resources. (Unfortunately, it will still contain all assemblies/assemblies.*.blob, and thus will be larger than desired, thus issue #8168.)

The Amazon Appstore also supports Android App Bundles.

Consequently, you (generally) shouldn't care about .apk files; those are mostly for local debugging.

Finally, it looks like the Archive Manager supports App Bundles; see: https://learn.microsoft.com/xamarin/android/deploy-test/release-prep/?tabs=windows#android-app-bundles

@jonpryor
Copy link
Member Author

jonpryor commented Jul 6, 2023

@jonathanpeppers asked:

Or should there only be 1 set of BCL assemblies instead?

Why not both?

The glorious thing about using .so files instead of .blob files is that we won't need to parse the .apk to find the assemblies/assemblies*.blob entries during startup, we could (maybe) just have direct symbol references. I'm sure @grendello will be thrilled! (HHOS)

There could thus be benefits to using .so files instead of assemblies*.blob files, even if we did have only 1 set of assemblies.

However, System.Private.CoreLib.dll is full of target-specific code, e.g.:

https://github.com/dotnet/runtime/blob/0224f86a886e0aaf72acb6a4f5573236ba929292/src/libraries/System.Private.CoreLib/src/System/Array.cs#L800-L802

Last time I heard, there was no intention to have an "architecture neutral" System.Private.CoreLib.dll a'la mono's mscorlib.dll, so we're always going to have an architecture-specific System.Private.CoreLib.dll. We could request dotnet/runtime to remove the ILLink Descriptors when building for Android, but (1) I'm not sure if they'd want to do so, (2) such a change might not land in time for .NET 8, and (3) such a change could be moot anyway, because anybody can provide those XML descriptors, reintroducing e.g. Issue #8155.

"1 set of assemblies" is only truly viable if there is a way to tell the linker to ignore all link descriptors (or all link descriptors which would introduce architecture-specific changes, which can't currently be done). This would also mean "no use of linker feature flags", which I suspect will go over like a lead balloon.

My "fear", then, is that "1 set of assemblies" is not viable, multi-arch assemblies are the present and future, and we need to ensure that we support them. (Note: We do support them! It's just that how we support them has various tradeoffs wrt .apk size.)

@jonpryor
Copy link
Member Author

jonpryor commented Jul 6, 2023

To expand upon "System.Private.CoreLib.dll is full of target-specific code":

% pwd
…/dotnet/runtime/src/libraries/System.Private.CoreLib

% git grep '#if ' | wc -l
     773

System.Private.CoreLib has 773 uses of #if, for:

% git grep -h '#if ' | awk '{ print $2 }' | sort | uniq -c | sort -n
   1 !((TARGET_BROWSER
   1 !(TARGET_BROWSER
   1 !NETSTANDARD2_1
   1 !TARGET_ARM64
   1 !TARGET_MACCATALYST
   1 !TARGET_OSX
   1 !TARGET_WASI
   1 (!TARGET_BROWSER
   1 EVENTSOURCE_GENERICS
   1 EVENT_SOURCE_LEGACY_NAMESPACE_SUPPORT
   1 NETSTANDARD2_0
   1 TARGET_ANDROID
   1 TARGET_FREEBSD
   1 TARGET_LINUX
   1 TARGET_MACCATALYST
   1 TARGET_WATCHOS
   1 TARGET_X86
   1 WINDOWS
   2 ENTITY_ENCODE_HIGH_ASCII_CHARS
   2 FEATURE_EVENTSOURCE_XPLAT
   2 MICROSOFT_INTEROP_SOURCEGENERATION
   2 REGISTRY_ASSEMBLY
   2 TARGET_AMD64
   2 TARGET_IOS
   2 TARGET_TVOS
   3 !DEBUG
   3 !RESOURCES_EXTENSIONS
   3 FEATURE_OBJCMARSHAL
   3 TARGET_ARM
   3 TARGET_ARM64
   4 !MONO
   4 !TARGET_BROWSER
   4 (!NETSTANDARD2_0
   4 FASTLOOP
   4 TARGET_UNIX
   6 !ES_BUILD_STANDALONE
   6 !FEATURE_WASM_PERFTRACING
   6 TARGET_WASI
   7 FEATURE_ADVANCED_MANAGED_ETW_CHANNELS
   7 HAS_CUSTOM_BLOCKS
   7 _LOGGING
   8 !CORECLR
  10 FEATURE_COMINTEROP
  10 RESOURCES_EXTENSIONS
  12 BIGENDIAN
  13 !FEATURE_WASM_THREADS
  15 NATIVEAOT
  19 !NATIVEAOT
  20 false
  23 FEATURE_MANAGED_ETW_CHANNELS
  23 TARGET_BROWSER
  25 RARE_ENUMS
  25 TARGET_WINDOWS
  27 FEATURE_MANAGED_ETW
  28 TARGET_32BIT
  28 TARGET_OSX
  32 CORECLR
  42 FEATURE_PERFTRACING
  47 MONO
  59 SYSTEM_PRIVATE_CORELIB
  87 DEBUG
 114 TARGET_64BIT

In particular note the 32 instances of TARGET_32BIT and 114 instances of TARGET_64BIT. An architecture-neutral System.Private.CoreLib.dll is not viable.

@grendello
Copy link
Contributor

@jonathanpeppers asked:

Or should there only be 1 set of BCL assemblies instead?

Why not both?

The glorious thing about using .so files instead of .blob files is that we won't need to parse the .apk to find the assemblies/assemblies*.blob entries during startup, we could (maybe) just have direct symbol references. I'm sure @grendello will be thrilled! (HHOS)

One downside of the .so idea is its potential to double memory usage for the assemblies. The blobs we have now are mmapped and only data that's actually needed is read from them. Even if the assemblies are compressed, we won't waste much more memory over what would normally be used by the runtime loading the assembly from disk, since we decmpress the mmapped data to a pre-allocated (at build time) memory buffer, which we then pass as a pointer to the runtime. However, with an .so we will have the entire library loaded into memory (since data will be kept in a R/O data section, that's entirely loaded) and, if compression is involved, we'll repeat the same action as with blobs, but the original compressed data will remain in memory. We won't be able to unload the shared library since we won't know when all the assemblies have been decompressed. If compression isn't involved, we'll have to keep the .so with assemblies around forever, since the runtime will use its data but the memory usage won't be doubled. However, in either case we will pay the up-front cost of allocated memory whether or not all the assemblies will be used by the application.

@mosammo
Copy link

mosammo commented Jul 9, 2023

@mosammo wrote:

please note same problem is also in "lib" folder inside the apk which has even larger size for unselected platforms as shown below

Yes and no: Yes, you are absolutely correct that the .apk contains lib/* directories which increase the size of the .apk and may not be relevant for the target device.

However, "No" because .apk is no longer the preferred App Distribution method, Android App Bundle (.aab) files are. The .aab will be "huge", but the Google Play store will take the .aab file as input, and strip out entries that are not relevant for the target device.

For example, you upload a .aab containing all ABIs to the Google Play store. When you download the app onto a Pixel 6, the downloaded .apk will contain only arm64-v8a; it will not contain lib/x86, lib/x86_64, or lib/armeabi-v7a. Similarly, Android Resources will contain only resources applicable to the target device, e.g. no "tablet sized" resources. (Unfortunately, it will still contain all assemblies/assemblies.*.blob, and thus will be larger than desired, thus issue #8168.)

The Amazon Appstore also supports Android App Bundles.

Consequently, you (generally) shouldn't care about .apk files; those are mostly for local debugging.

Finally, it looks like the Archive Manager supports App Bundles; see: https://learn.microsoft.com/xamarin/android/deploy-test/release-prep/?tabs=windows#android-app-bundles

The problem is some projects do not use Google Play store for app distribution and instead using the .apk files which on Xamarin android didn't include such files of unselected architectures and were much smaller than the apk files currently generated by .net 7+ for android. Such unselected architectures files (like in "assemblies" and "lib" folders) should be excluded or removed from the generated apk.

@dellis1972
Copy link
Contributor

@mosammo

Are you using the RuntimeIdentifiers property group instead of AndroidSupportedAbis in your .net 7+ projects?
see https://github.com/xamarin/xamarin-android/blob/main/Documentation/guides/OneDotNet.md#changes-to-msbuild-properties.

If you are only targeting say intel x86/64 you should be using the following

<PropertyGroup>
  <!-- Used going forward in .NET -->
  <RuntimeIdentifiers>android-x86;android-x64</RuntimeIdentifiers>
</PropertyGroup>

This should exclude the arm based abi's from the final apk. If it doesn't that is a bug.

@mosammo
Copy link

mosammo commented Jul 10, 2023

@mosammo

Are you using the RuntimeIdentifiers property group instead of AndroidSupportedAbis in your .net 7+ projects? see https://github.com/xamarin/xamarin-android/blob/main/Documentation/guides/OneDotNet.md#changes-to-msbuild-properties.

If you are only targeting say intel x86/64 you should be using the following

<PropertyGroup>
  <!-- Used going forward in .NET -->
  <RuntimeIdentifiers>android-x86;android-x64</RuntimeIdentifiers>
</PropertyGroup>

This should exclude the arm based abi's from the final apk. If it doesn't that is a bug.

Thank you!! it works!! turns out it's just a UI bug in visual studio, selecting "Platform target" in visual studio project settings should set the related settings in the project file, but it only sets
"<PlatformTarget>ARM64</PlatformTarget>"
and doesn't set
"<RuntimeIdentifiers>android-arm;android-arm64;</RuntimeIdentifiers>"
platform target option 64

After setting "<RuntimeIdentifiers>" manually in the android project file now it works perfectly and correctly excludes unselected platforms from the apk from both "assemblies" and "lib"!
Thanks a lot 👍

@dellis1972
Copy link
Contributor

@mosammo Great :) On a side note you probably want to leave the PlatformTarget as AnyCPU.

@jpobst jpobst removed the needs-triage Issues that need to be assigned. label Jan 9, 2024
jonpryor pushed a commit that referenced this issue Jan 18, 2024
Context: c927026
Context: 6836818
Context: 929e701
Context: #8478
Context: #8155
Context: #8168

Our build process has a bit of a "consistent sanity" problem: large
portions of the build process assume that the output of a `.csproj`
is a single assembly, and that single assembly is (eventually)
embedded into the `.apk`, to be loaded at runtime.

Unfortunately, that wasn't strictly true starting with .NET 5:
there were *multiple* `System.Private.CoreLib.dll` assemblies, which
needed to be treated specially; see also c927026.

We discovered that this assumption was even *less* true because of
the linker, which would quite happily *replace* `IntPtr.get_Size()`
method invocations with a *constant*, specific for the target ABI;
see also 6836818, 929e701.  This in turn could be responsible for
all sorts of weirdness if e.g. a 64-bit assembly were used in a
32-bit process, or vice versa.

Which brings us to the original assumption: there is (usually) only
one "source" assembly, which is (optionally) linked into one "linked"
assembly, which is packaged into one assembly in the `.apk`.

With the linker, though, we can have one "source" assembly, which
when linked becomes *N* assemblies, which *should be* separately
packaged as per-ABI assemblies within the `.apk`, but
*probably aren't*, which we *think* is the "root cause" of #8155.

PR #8478 is attempting fix this assumption, imbuing the build system
with knowledge that the linker may produce *multiple outputs* for a
single input assembly.

Unfortunately, in trying to fix things, various intermediate assembly
locations have *changed*, which in turn breaks
[AndroidX Migration in Xamarin.Forms][0], as it makes assumptions
about where various assemblies are located within `obj`:

	(_AndroidXCecilfy target) ->
	/Users/runner/.nuget/packages/xamarin.androidx.migration/1.0.8/buildTransitive/monoandroid90/Xamarin.AndroidX.Migration.targets(227,9): error : Source assembly does not exist: 'obj/Debug/android/assets/UnnamedProject.dll'.

as [`@(_AndroidXFileToCecilfy)`][1] uses
`$(MonoAndroidIntermediateAssetsDir)`, which does not account for the
ABI which is now in the path:

	<ItemGroup>
	  <_AndroidXFileToCecilfy Include="@(ResolvedUserAssemblies->'$(MonoAndroidIntermediateAssetsDir)%(Filename)%(Extension)')"
	    Condition="('%(ResolvedUserAssemblies.TargetFrameworkIdentifier)' == 'MonoAndroid' or '%(ResolvedUserAssemblies.HasMonoAndroidReference)' == 'true') and ('%(ResolvedUserAssemblies.AndroidXSkipAndroidXMigration)' != 'true')" />
	</ItemGroup>

Given that AndroidX Migration is mostly for Xamarin.Forms customers
(and *kinda* buggy, and unmaintained), and MAUI doesn't support the
Android Support libraries, and thus doesn't need AndroidX Migration,
we'd like to just *not worry about this*.

The problem?  The above error message is not actionable, and doesn't
tell anybody how to fix it.

Introduce a new `XA1039` *actionable* error in .NET 9:

	error XA1039: The Android Support libraries are not supported in .NET 9 and later,
	please migrate to AndroidX. See https://aka.ms/xamarin/androidx for more details.

The XA1039 error is generated if any NuGet packages are found matching:

  * `Xamarin.Android.Support.*`
  * `Xamarin.Android.Arch.*`

TODO: "port" XA1039 to .NET 8 *as a warning*, so that customers will
have some time to migrate off of the Android Support libraries before
.NET 9 is released in 2024-Nov.

	--<AndroidError Code="XA1039"
	++<AndroidWarning Code="XA1039"
	    ResourceName="XA1039"
	    Condition=" '@(_AndroidUnsupportedPackages->Count())' != '0' "
	/>

The biggest impact here is going to be many of our old tests, which
use the support libraries in various forms.

Improvements & general cleanup:

  * Removed all old/unused packages in `KnownPackages.cs`

  * Updated `KnownPackages.cs` to latest versions, including Xamarin.Forms

  * [Android Wear tests are now migrated from support to AndroidX][2]

  * `AndroidUpdateResourcesTest.CheckEmbeddedSupportLibraryResources()`
    is renamed to `.CheckEmbeddedAndroidXResources()`.

  * `BuildTest2.BuildHasNoWarnings()` was not appropriately applying
    `IsRelease`.

  * `Android.Support.v8.RenderScript` removed in favor of an inline
    `.so` file.

  * A few tests that used support libraries to create a project large
    numbers of dependencies, moved to
    `XamarinFormsAndroidApplicationProject`.

  * `IncrementalBuildTest.ResolveLibraryProjectImports()` now sorts
    items before comparing them.

  * `XamarinFormsAndroidApplicationProject` has a workaround for Guava:  
    <xamarin/AndroidX#535>

  * Fix a bug in `AndroidFastDeploymentType=Assemblies::Dexes`;
    see "AndroidFastDeploymentType=Assemblies::Dexes" section, below.

Removed tests:

  * `AndroidXMigration`, `AndroidXMigrationBug`

  * `ResolveLibraryImportsWithReadonlyFiles`, seemed duplicate of other
    Android Wear tests, and used support libraries.

  * `ExtraAaptManifest` as it depends on `Xamarin.Android.Fabric` and
    `Xamarin.Android.Crashlytics`. These are deprecated and depend on
    support libraries.

  * `BuildProguardEnabledProjectSource` now only tests `Release` mode.
    Since updating to AndroidX, `Debug` mode was triggering multi-dex.
    Making it difficult to assert contents of `*.dex` files.


~~ AndroidFastDeploymentType=Assemblies::Dexes ~~

The runtime test in `ApplicationRunsWithDebuggerAndBreaks()` was
crashing at runtime with:

	E monodroid-assembly: typemap: failed to stat TypeMap index file '/data/user/0/com.xamarin.applicationrunswithdebuggerandbreaks/files/.__override__/typemaps/typemap.index': No such file or directory
	F monodroid-assembly: typemap: unable to load TypeMap data index from '/data/user/0/com.xamarin.applicationrunswithdebuggerandbreaks/files/.__override__/typemaps/typemap.index'

This only happens when `AndroidFastDeploymentType=Assemblies::Dexes` is
used, as it is the case when typemap files like this are fast deployed
and used at runtime.

What was even more odd, was the file seems to exist after a
`-t:Install`, but ends up missing after `-t:Run`:

	> adb shell run-as com.xamarin.applicationrunswithdebuggerandbreaks ls -la files/.__override__/typemaps/typemap.index
	ls: files/.__override__/typemaps/typemap.index: No such file or directory

It appears that `-t:Install` successfully deploys the file:

	Pushed 3969 to /data/local/tmp/.xatools/typemap.index
	DEBUG RunShellCommand emulator-5554 "run-as" "com.xamarin.applicationrunswithdebuggerandbreaks" "--user" "0" "files/.__tools__/xamarin.cp" "/data/local/tmp/.xatools/typemap.index" "files/.__override__/typemaps/typemap.index" "1705432079367" [5ms]
	files/.__tools__/xamarin.cp returned: moved [/data/local/tmp/.xatools/typemap.index] to [files/.__override__/typemaps/typemap.index] modifieddate [1705432079367]
	moved /data/local/tmp/.xatools/typemap.index to files/.__override__/typemaps/typemap.index
	Installed files/.__override__/typemaps/typemap.index. [12ms]
	NotifySync CopyFile obj\Debug\android\typemaps\typemap.index. [0ms]

But then `-t:Run` deletes the file!

	Remove redundant file files/.__override__/typemaps/typemap.index
	DEBUG RunShellCommand 0A041FDD400327 "run-as" "com.xamarin.applicationrunswithdebuggerandbreaks" "rm" "-Rf" "files/.__override__/typemaps/typemap.index" [29ms]

This happens because the `@(_AndroidTypeMapping)` item group is empty
during an incremental build:

  * The `<GenerateJavaStubs/>` MSBuild task, during the first build
    outputs `@(_AndroidTypeMapping)` items

  * During an incremental build, the `_GenerateJavaStubs` MSBuild
    *target* is skipped, and so the `@(_AndroidTypeMapping)` item
    group is empty!

  * The `<FastDeploy/>` task happily deletes files that it thinks
    should be removed.

For now, let's add logic to the `_GenerateJavaStubs` target to fill
in the `@(_AndroidTypeMapping)` item group during incremental builds:

	<ItemGroup Condition=" '$(_InstantRunEnabled)' == 'True' and '@(_AndroidTypeMapping->Count())' == '0' ">
	  <_AndroidTypeMapping Include="$(_NativeAssemblySourceDir)typemaps\*" />
	</ItemGroup>

`<ItemGroup>`s are still evaluated when a target is *skipped*,
solving the problem.

I assume this is working in `main`, because Xamarin.AndroidX.Migration
package was involved.  It likely was running the `_GenerateJavaStubs`
target on every build.

[0]: https://learn.microsoft.com/xamarin/xamarin-forms/platform/android/androidx-migration
[1]: https://github.com/xamarin/AndroidX/blob/17e596fafe20331d7feb69240c38e0fbdc3ea640/source/migration/BuildTasks/Xamarin.AndroidX.Migration.targets#L205-L206
[2]: https://android-developers.googleblog.com/2016/04/build-beautifully-for-android-wear.html
@github-actions github-actions bot locked and limited conversation to collaborators Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Area: App+Library Build Issues when building Library projects or Application projects.
Projects
None yet
Development

No branches or pull requests

6 participants