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

Allow configuring ndk build architectures #31232

Closed
wants to merge 2 commits into from

Conversation

janicduplessis
Copy link
Contributor

@janicduplessis janicduplessis commented Mar 25, 2021

Summary

Building from source in debug takes a very long time because native builds need to run for all supported architectures. It is possible to check which architecture the devices for which we are about to launch the app on are and build only for those. For most cases we can reduce the number of architectures we build for to 1 instead of 4, resulting in a large speedup of the build.

This is inspired by iOS which has a "Build for active architecture only" option. Since android doesn't really support this natively we can implement it here and also in react-native by reading the build properties that we pass and alter the abi we build for.

With fabric / codegen coming up I suspect that we might want to default to building c++ soon. This should ease the transition as builds won't be orders of magnitude slower.

See react-native-community/cli#1388 for more context and how we use this new config to automatically detect running emulator architectures.

Changelog

[Android] [Added] - Allow configuring ndk build architectures

Test Plan

Tested by setting reactNativeDebugArchitectures with different values in gradle.properties. Checked the build logs to see which architectures are being built. Also made sure release builds are not affected by this value.

Clean build

reactNativeDebugArchitectures not set
824.41s

reactNativeDebugArchitectures=x86
299.77s

@facebook-github-bot facebook-github-bot added Contributor A React Native contributor. CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Mar 25, 2021
@analysis-bot
Copy link

analysis-bot commented Mar 25, 2021

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: ebe939b

@janicduplessis janicduplessis marked this pull request as ready for review March 25, 2021 18:16
@@ -254,7 +264,6 @@ if (enableCodegen) {
defaultConfig {
externalNativeBuild {
ndkBuild {
abiFilters "armeabi-v7a", "x86", "x86_64", "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.

This is the default already according to https://developer.android.com/ndk/guides/abis. This will use the project wide config added a few lines up.

@analysis-bot
Copy link

analysis-bot commented Jul 6, 2021

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,120,527 +391
android hermes armeabi-v7a 8,646,426 +259
android hermes x86 9,559,730 +258
android hermes x86_64 9,525,889 +243
android jsc arm64-v8a 10,762,975 +132
android jsc armeabi-v7a 9,680,129 -7
android jsc x86 10,797,576 -6
android jsc x86_64 11,405,238 -32

Base commit: bc1e602

Copy link
Contributor

@ShikaSD ShikaSD left a comment

Choose a reason for hiding this comment

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

Awesome!
I was manually hardcoding splits before for testing, but that is definitely better 🙂
I added a couple of comments/questions about debug mode handling.

ReactAndroid/build.gradle Show resolved Hide resolved
it.args.any { it.endsWith("Debug") }
}
def value = project.getProperties().get("reactNativeDebugArchitectures")
return value != null && isDebug ? value : "all"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "all" value valid for the abiFilters block? From the documentation, I only see it used for ndk-build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This one is only used with ndk-build so the all value is fine.

@@ -355,6 +363,7 @@ def buildReactNdkLib = tasks.register("buildReactNdkLib", Exec) {
inputs.dir("src/main/java/com/facebook/react/modules/blob")
outputs.dir("$buildDir/react-ndk/all")
commandLine(getNdkBuildFullPath(),
"APP_ABI=${reactNativeArchitectures()}",
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe safeguard the debug version here as well? 🙂

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 don't think this is needed since reactNativeArchitectures does the debug check already and is only used here. It will always return 'all' for non-debug builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, my intention is to avoid hacking the debug check based on the task name and use flags from the build task itself, that's why I am asking about it :)

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 agree, I tried finding a better way to detect wether it was a debug or release build but could not find it. The task is the same for both build types so at that point from what I could see it is not possible to know without the gradle invocation args.

One more involved solution that I could see is create a different task per variant for buildReactNdkLib, but not sure if that would be worth changing for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see it now, didn't realize we always compile the build with NDK_DEBUG set to 0. I guess it doesn't matter much for this module now, as we build everything on the CI anyways and distribute binaries only.

@janicduplessis
Copy link
Contributor Author

We currently have 2 different type of c++ builds, the main one invokes ndk-build directly and the codegen one uses gradle externalNativeBuild. abiFilters will only work for the codegen build so this is why we have 2 different handling of the achitectures value (reactNativeArchitectures method in ReactAndroid.gradle vs using abiFilters in rn-tester / app template).

@ShikaSD
Copy link
Contributor

ShikaSD commented Jul 7, 2021

We currently have 2 different type of c++ builds, the main one invokes ndk-build directly and the codegen one uses gradle externalNativeBuild.

I think ndk-build also has the debug parameter passed somewhere, so maybe we can ensure correct abi there?

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Jul 7, 2021

AFAIK there is NDK_DEBUG, but it is not based on gradle build type, we set it here https://github.com/facebook/react-native/blob/master/ReactAndroid/build.gradle#L358 based on https://github.com/facebook/react-native/blob/master/ReactAndroid/build.gradle#L43. From what I understand NDK_DEBUG will always be false unless NATIVE_BUILD_TYPE env is set.

@facebook-github-bot
Copy link
Contributor

@ShikaSD has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ShikaSD merged this pull request in d6ed1ff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged. Needs: React Native Team Attention
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants