-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Changes from 3 commits
9aa93ec
b330df4
c9e4f7b
e0d8417
7e37281
5c432a2
ab2477a
fdc5c3d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,6 +33,32 @@ CLEAN_OBJ := if exist $(OBJ_DIR) (rd /s /q $(subst /,\\,$(OBJ_DIR)) && mkdir $(s | |
endif | ||
|
||
LDLIBS := -L$(SYSROOT)/usr/lib -ldl -llog -L$(TARGET_OUT) | ||
|
||
# optimize output size | ||
CFLAGS += -Os -flto -ffunction-sections -fdata-sections | ||
LDLIBS += -Wl,--gc-sections,--strip-all | ||
|
||
# exclude v8 libraries | ||
LDLIBS += -Wl,--exclude-libs=libv8_libbase | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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? |
||
LDLIBS += -Wl,--exclude-libs=libv8_libplatform | ||
# LDLIBS += -Wl,--exclude-libs=libv8_base | ||
LDLIBS += -Wl,--exclude-libs=libv8_nosnapshot | ||
LDLIBS += -Wl,--exclude-libs=libv8_builtins_generators | ||
LDLIBS += -Wl,--exclude-libs=libv8_builtins_setup | ||
|
||
# tune for common architectures | ||
ifeq ($(TARGET_ARCH_ABI),arm64-v8a) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Most processors should benefit from this, as they derive from these common architectures. |
||
CFLAGS += -mtune=cortex-a53 | ||
endif | ||
ifeq ($(TARGET_ARCH_ABI),x86) | ||
CFLAGS += -mtune=atom | ||
LDLIBS += -Wl,--icf=all | ||
endif | ||
ifeq ($(TARGET_ARCH_ABI),armeabi-v7a) | ||
CFLAGS += -mtune=cortex-a7 | ||
LDLIBS += -Wl,--icf=all | ||
endif | ||
|
||
ABS_SRC_FILES := \ | ||
$(wildcard $(LOCAL_PATH)/*.cpp) \ | ||
$(wildcard $(LOCAL_PATH)/modules/*.cpp) | ||
|
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.
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?
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.
Users can specify the abis they want using the
<abi>
tag (which overrides the removal of x86 from production builds):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)
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.
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.
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...
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 asplit="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:
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?
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.
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.
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.
@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 aversionCodeOffset=""
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.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.
That sounds like a great idea!