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

[TIMOB-26006] Android: Optimize libkroll-v8 size #10021

Merged
merged 8 commits into from May 18, 2018

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented May 1, 2018

  • Use compiler flags to optimize the size of libkroll-v8 for all supported architectures
  • Tune performance for common architectures
  • Remove x86 library from production builds
libkroll-v8 BEFORE AFTER DIFF
x86 18.6MB 16.3MB -13.4%
armabi-v7a 13.7MB 12.9MB -6.0%
arm64 17.2MB 16.4MB -5.7%
TOTAL 49.5MB 45.6MB -3.9MB

JIRA Ticket

LDLIBS += -Wl,--exclude-libs=libv8_builtins_setup

# tune for common architectures
ifeq ($(TARGET_ARCH_ABI),arm64-v8a)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The -mtune parameter will provide optimizations for the defined processor, but will maintain compatibility.

Most processors should benefit from this, as they derive from these common architectures.

LDLIBS += -Wl,--gc-sections,--strip-all

# exclude v8 libraries
LDLIBS += -Wl,--exclude-libs=libv8_libbase
Copy link
Contributor

Choose a reason for hiding this comment

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

So let me first preface this by saying I know nothing about C/C++ flags, linker flags, etc.

Would a change like this affect the ability to build native modules?

Also to note: in the 8.0 timeframe I'm moving to a newer V8 building that generates a single static library (v8_monolith.a)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tested this with a native module and everything seems to function fine. It's the symbols from libv8_base (commented out) which are required by native modules.

I guess we will need to take another look at this once we move to a single library. We may not be able to optimize it as much?

// by default, remove 'x86' from production builds
// 'x86' devices are scarce; this is predominantly used for emulators
// we can save 16MB+ by removing this from release builds
this.abis.splice(this.abis.indexOf('x86'), 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I could see this being a useful default, but certainly would need some way for users to be able to tune this to explicitly produce a given set of ABIs they wanted to target.

Also, would there be any benefit (in build time, primarily) to a user tuning this to only do x86 on development builds when just testing on an x86 emulator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users can specify the abis they want using the <abi> tag (which overrides the removal of x86 from production builds):

<ti:app xmlns:ti="http://ti.appcelerator.org">
  <android>
    <abi>armeabi-v7a,arm64-v8a</abi>
  </android>
</ti:app>

Only doing x86 when targeting emulators wouldn't yield much benefit, maybe a couple of seconds build time saved? But we can't be sure what architecture the emulator is (you can create ARM emulators)

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's an alternative idea.

Our "tiapp.xml" file supports a <abi/> entry which allows the dev to select which architectures to include. By default, it includes all architectures supported...
https://docs.appcelerator.com/platform/latest/#!/guide/tiapp.xml

So, if someone wants the old behavior and exclude the ARM64 architecture, they can add this to their "tiapp.xml" file.

<ti:app xmlns:ti="http://ti.appcelerator.org">
	<android>
		<abi>armeabi-v7a,x86</abi>
	</android>
</ti:app>

What I propose we do is add a deployType attribute to the <abi> XML element so that a dev can indicate which architectures per deployment type (ie: "test", "development", and "production"). The idea being you can have multiple <abi> elements that differ by deployment type, and if the current deployment type <abi> cannot be found, then it defaults to the <abi> that does not have the deployment type defined... and if that can't be found that it supports all architectures.

For example...

<ti:app xmlns:ti="http://ti.appcelerator.org">
	<android>
		<abi>armeabi-v7a,arm64-v8a,x86</abi>
		<abi deployType="production">armeabi-v7a,arm64-v8a</abi>
	</android>
</ti:app>

Odds are a dev would only define 1 <abi> element set to "production" and have all test builds include all architectures, but in the future we might want to add a split="true" attribute that the dev can use to generate multiple APKs per architecture.

Also, you can get away with only including ARMv7 architecture for production builds because:

  • Most x86 atom devices can run ARMv7 apps via "libhoudini" emulation.
  • ARM64 devices can run ARMv7 apps.

x86 is really only useful for fast Android emulator support. ARM64 gives you a slight performance boost, but isn't required (Google Play may require this architecture in order to be "featured" on the app store in the future; we'll see).

Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thinking as a ti dev that dealt with this a few weeks ago: I‘d love to have a command or option to export each abi as a single apk, bumping the version code for each apk to avoid google play errors. I actually wanted to write a node
script for that but would rather want to see it in the SDK. If we‘d support gradle, it could be done automatically as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@hansemannn, yeah, I'm still pondering how to approach this. Especially after speaking with a community member here...
https://jira.appcelerator.org/browse/AC-5591

While inconvenient from a test/maintenance standpoint for the app developer, I can understand the desire of wanting to split APKs per architecture to reduce download size for end-users while still supporting best performance for the end-user's device architecture.

Now I'm thinking that we should allow multiple <abi> tags of type "production" and build an APK for each one found. And also support a versionCodeOffset="" attribute since the version code of a universal APK needs to be different than the per-architecture APKs. This is what Google recommends to do in gradle, although if you look at it, the version code offsetting is kind of kludge on Google's end.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That sounds like a great idea!

@build
Copy link
Contributor

build commented May 8, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@garymathews garymathews added this to the 7.3.0 milestone May 18, 2018
@lokeshchdhry
Copy link
Contributor

FR Passed.

Built KS & a default app with this fix/improvement with hyperloop & liveview & no crashes seen. Also verified the packaged apk does not have the x86 folder inside lib folder.

Studio Ver: 5.1.0.201805150850
SDK Ver: 7.3.0 build with the fix/improvement
OS Ver: 10.13.4
Xcode Ver: Xcode 9.3.1
Appc NPM: 4.2.13
Appc CLI: 7.0.3
Daemon Ver: 1.1.1
Ti CLI Ver: 5.1.0
Alloy Ver: 1.12.0
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10
Devices: ⇨ google Nexus 6P --- Android 8.1.0
⇨ google Nexus 5 --- Android 6.0.1
Emulator: ⇨ Android 4.1.2

@sgtcoolguy sgtcoolguy merged commit 59c1214 into tidev:master May 18, 2018
@sgtcoolguy
Copy link
Contributor

@garymathews This PR seems to be breaking builds with NDK r16b and r17 for me over at #9926

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants