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

Upgrade Android toolchain #615

Closed
wants to merge 2 commits into from
Closed

Upgrade Android toolchain #615

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented May 23, 2023

Should buy android devs some time. Still want to investigate upgrading to Gradle 8.

Some notable upgrades:

  • latest version of react
  • latest version of react-native
  • latest version of react-native-v8 with -nointl
  • latest version of Kotlin
  • Gradle on the latest 7.x release

Only tested on Pixel 7 up to seeing the order book, so needs more testing.

The diff of these files is the best place to see what was upgraded:

  • package.json
  • android/gradle/wrapper/gradle-wrapper.properties
  • android/gradle.properties
  • android/build.gradle
  • android/app/build.gradle

Generally things seem to work though :)

TODO:

  • This was all done on an M1 mac, need to test on x86 ubuntu
  • Needs more testing of APKs especially areas of the app that use native plugins

Checklist before merging

  • Install pre-commit and initialize it: pip install pre-commit, then pre-commit install. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.

Seems to work :)
@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 23, 2023

Hey @shyfire131 , awesome that you are tackling this one. I tried a couple of times in the past and this task ended up giving me nightmares :D

EDIT: disregard this one, the dev environment was not well set up.
When running npx react-native run-android locally I get:

... some errors...

When running ./gradlew assembleRelease, we get this other error. It happens both, locally and on our release workflow (here full GH Action logs https://github.com/RoboSats/robosats/actions/runs/5056095489/jobs/9073141503)

> Task :react-native-v8:buildCMakeRelWithDebInfo[arm64-v8a][v8executor]
C/C++: ninja: Entering directory `/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/android/.cxx/RelWithDebInfo/5iv263h6/arm64-v8a'
C/C++: clang++: error: linker command failed with exit code 1 (use -v to see invocation)

> Task :react-native-v8:buildCMakeRelWithDebInfo[arm64-v8a][v8executor] FAILED
91 actionable tasks: 91 executed

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':react-native-v8:buildCMakeRelWithDebInfo[arm64-v8a][v8executor]'.
> com.android.ide.common.process.ProcessException: ninja: Entering directory `/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/android/.cxx/RelWithDebInfo/5iv263h6/arm64-v8a'
  [1/14] Building CXX object CMakeFiles/reactnative_internal_static.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native/ReactCommon/reactperflogger/reactperflogger/BridgeNativeModulePerfLogger.cpp.o
  [2/14] Building CXX object CMakeFiles/reactnative_internal_static.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native/ReactCommon/cxxreact/JSExecutor.cpp.o
  [3/14] Building CXX object CMakeFiles/reactnative_internal_static.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native/ReactCommon/jsiexecutor/jsireact/JSINativeModules.cpp.o
  [4/14] Building CXX object CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/JSIV8ValueConverter.cpp.o
  [5/14] Building CXX object CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/HostProxy.cpp.o
  [6/14] Building CXX object CMakeFiles/reactnative_internal_static.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native/ReactCommon/jsiexecutor/jsireact/JSIExecutor.cpp.o
  [7/14] Linking CXX static library libreactnative_internal_static.a
  [8/14] Building CXX object CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8PointerValue.cpp.o
  [9/14] Building CXX object CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8RuntimeFactory.cpp.o
  [10/14] Building CXX object CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp.o
  [11/14] Building CXX object CMakeFiles/v8executor.dir/src/main/cpp/V8ExecutorFactory.cpp.o
  [12/14] Building CXX object CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8Inspector.cpp.o
  [13/14] Building CXX object CMakeFiles/v8executor.dir/src/main/cpp/OnLoad.cpp.o
  [14/14] Linking CXX shared library ../../../../build/intermediates/cxx/RelWithDebInfo/5iv263h6/obj/arm64-v8a/libv8executor.so
  FAILED: ../../../../build/intermediates/cxx/RelWithDebInfo/5iv263h6/obj/arm64-v8a/libv8executor.so 
  : && /usr/local/lib/android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ --target=aarch64-none-linux-android26 --gcc-toolchain=/usr/local/lib/android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/linux-x86_64 --sysroot=/usr/local/lib/android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/linux-x86_64/sysroot -fPIC -g -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security   -DREACT_NATIVE_TARGET_VERSION=71 -fexceptions -fno-omit-frame-pointer -frtti -Wno-sign-compare -fstack-protector-all -DV8_COMPRESS_POINTERS -O2 -g -DNDEBUG  -Wl,--exclude-libs,libgcc.a -Wl,--exclude-libs,libgcc_real.a -Wl,--exclude-libs,libatomic.a -Wl,--build-id -Wl,--fatal-warnings -Wl,--no-undefined -Qunused-arguments -shared -Wl,-soname,libv8executor.so -o ../../../../build/intermediates/cxx/RelWithDebInfo/5iv263h6/obj/arm64-v8a/libv8executor.so CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/HostProxy.cpp.o CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/JSIV8ValueConverter.cpp.o CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8Inspector.cpp.o CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8PointerValue.cpp.o CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp.o CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8RuntimeFactory.cpp.o CMakeFiles/v8executor.dir/src/main/cpp/V8ExecutorFactory.cpp.o CMakeFiles/v8executor.dir/src/main/cpp/OnLoad.cpp.o  /usr/local/lib/android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/lib/aarch64-linux-android/26/liblog.so  -landroid  /home/runner/.gradle/caches/transforms-3/5d07f50a3418bed82ae08cf2148d9eae/transformed/jetified-fbjni-0.3.0/prefab/modules/fbjni/libs/android.arm64-v8a/libfbjni.so  /home/runner/.gradle/caches/transforms-3/b03bb67f791cdea275f2788aac865b2e/transformed/jetified-react-android-0.71.8-release/prefab/modules/jsinspector/libs/android.arm64-v8a/libjsinspector.so  /home/runner/.gradle/caches/transforms-3/b03bb67f791cdea275f2788aac865b2e/transformed/jetified-react-android-0.71.8-release/prefab/modules/runtimeexecutor/libs/android.arm64-v8a/libruntimeexecutor.so  libreactnative_internal_static.a  ../../../../build/jniLibs/v8/jni/arm64-v8a/libv8android.so  /home/runner/.gradle/caches/transforms-3/b03bb67f791cdea275f2788aac865b2e/transformed/jetified-react-android-0.71.8-release/prefab/modules/folly_runtime/libs/android.arm64-v8a/libfolly_runtime.so  /home/runner/.gradle/caches/transforms-3/b03bb67f791cdea275f2788aac865b2e/transformed/jetified-react-android-0.71.8-release/prefab/modules/glog/libs/android.arm64-v8a/libglog.so  /home/runner/.gradle/caches/transforms-3/b03bb67f791cdea275f2788aac865b2e/transformed/jetified-react-android-0.71.8-release/prefab/modules/jsi/libs/android.arm64-v8a/libjsi.so  /home/runner/.gradle/caches/transforms-3/b03bb67f791cdea275f2788aac865b2e/transformed/jetified-react-android-0.71.8-release/prefab/modules/reactnativejni/libs/android.arm64-v8a/libreactnativejni.so  -latomic -lm && :
  CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp.o: In function `std::__ndk1::__fs::filesystem::path::filename() const':
  /usr/local/lib/android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/c++/v1/filesystem:1067: undefined reference to `std::__ndk1::__fs::filesystem::path::__filename() const'
  CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp.o: In function `std::__ndk1::__fs::filesystem::path::has_root_directory() const':
  /usr/local/lib/android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/c++/v1/filesystem:1084: undefined reference to `std::__ndk1::__fs::filesystem::path::__root_directory() const'
  CMakeFiles/v8executor.dir/home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/src/v8runtime/V8Runtime.cpp.o: In function `std::__ndk1::__fs::filesystem::path::has_filename() const':
  /usr/local/lib/android/sdk/ndk/21.4.7075529/toolchains/llvm/prebuilt/linux-x86_64/sysroot/usr/include/c++/v1/filesystem:1096: undefined reference to `std::__ndk1::__fs::filesystem::path::__filename() const'
  clang++: error: linker command failed with exit code 1 (use -v to see invocation)
  ninja: build stopped: subcommand failed.
  
  C++ build system [build] failed while executing:
      /usr/local/lib/android/sdk/cmake/3.18.1/bin/ninja \
        -C \
        /home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/android/.cxx/RelWithDebInfo/5iv263h6/arm64-v8a \
        v8executor
    from /home/runner/work/robosats/robosats/mobile/node_modules/react-native-v8/android

I cleared the cache of our android release workflow and retried, but it got the same error (cleaned everything too locally).

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 23, 2023

Okay. I do not often quote Elon Musk but this is an exceptional case: "the best part, is no part". I removed react-native-v8 and v8-android-nointl . As far as I can tell, the app is fully functional without the dependencies. See branch main...update-android-toolchain

Which leads to question why these dependencies were needed in the first place 😄 . I believe react-native-tor could not work on react-native v0.69 without them, but this is just a blurry memory of a conversation with @KoalaSat (only I could find this as context #247)

Find build artifacts on pre-release: https://github.com/RoboSats/robosats/releases/tag/android-66eb18c

@ghost
Copy link
Author

ghost commented May 23, 2023

I checked out the main...update-android-toolchain branch locally and can confirm that removing v8 works on my side too. Although I think there is still a stale maven repository reference for it but I'll omit that when I update this branch.

Going to gulp try bumping Gradle to Gradle 8 now.

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 24, 2023

To be on the clear about the effect of removing V8 in performance, I wrote a small component that renders our webapp <Main /> component 100 times and prints the average render time (https://github.com/RoboSats/robosats/tree/add-render-benchmark). Then I ran the benchmark 20 times on both: react-native v0.69 with V8 and react-native v0.71.8 without V8.

Results

Here the summary stats of the 20 runs on a Pixel 6

RN-v0.69 w/ V8 RN-v0.71 w/o V8
mean 12.23 ms 11.966 ms
stdev 0.2844 ms 0.2903 ms
sem 0.636 0.0649
n 20 20

The two-tailed P value = 0.0061

And 10 runs on a Pixel 3XL

RN-v0.69 w/ V8 RN-v0.71 w/o V8
mean 38.955 ms 38.185 ms
stdev 0.4961 ms 0.4966
sem 0.1569 0.1570
n 10 10

The two-tailed P value = 0.0027

So...

Looking at the mean rendering times, there is really no performance difference in absolute terms, times are very similar. We can safely drop V8 dependencies. However, the performance is indeed statisticallt different (positive test). Just not the way we expected, worse without V8, but the other way around: react-native 0.71 without V8 is a tiny faster (but who knows why, maybe other factors can explain, like phone temperature, background load, etc. But it is consistent across devices).

Indeed, it seems the main factor for JS performance is the Webview engine of the Android system, which in GrapheneOS is Vanadium (i.e., Chromium, so it has V8).

@ghost
Copy link
Author

ghost commented May 24, 2023

Very nice! Just to be clear, the null hypothesis for your p-values is "Removing V8 would slow down performance" ?

@Reckless-Satoshi
Copy link
Collaborator

Reckless-Satoshi commented May 24, 2023

the null hypothesis for your p-values is "Removing V8 would slow down performance"

Given that is a two-sided t-test, the null hypothesis is "Removing V8 has no performance impact". A small p-value, like we got, means that, indeed, removing V8 has a performance impact. However, the significant impact seems to be for increased performance with no V8! Certainly unexpected. Most likely V8 has no impact at all and the increased performance must be due to some optimizations of react-native v0.71.8. But that's enough of nerding around! :D

@Reckless-Satoshi
Copy link
Collaborator

Merged into main via another branch without the dependaBot merge conflicts.

Thank you @shyfire131 taking a look and getting this to work on the newer react-native. Please accept a 200K Sats donation from the devfund. Send an invoice from a proxy wallets with a >24h expiration time :D

@ghost
Copy link
Author

ghost commented May 30, 2023

lnbc2m1pj8vt9app5g77zcsphuzd8v0dlvqnmydaevgjflzfsz8agsgfjvu4ye2dtl2zqdqu2askcmr9wssx7e3q2dshgmmndp5scqzzsxqyz5vqsp5p76lkcy38564kypyk3gs47wxd8vvsjvugv2r5g0uvvzdqa7n9mes9qyyssqqpgtfyjtaqqs477r40dyvj2rj0ywlr9zx0hmljvkhatuahlyk4vjkv5hy0e0pad7txp89vnatdrnxl8z9gvehvvyfq8xpatdrcxu9cgpwwt9v6

@ghost ghost deleted the chore/update-android-toolchain branch May 30, 2023 17:08
@Reckless-Satoshi
Copy link
Collaborator

lnbc2m1pj8vt9app5g77zcsphuzd8v0dlvqnmydaevgjflzfsz8agsgfjvu4ye2dtl2zqdqu2askcmr9wssx7e3q2dshgmmndp5scqzzsxqyz5vqsp5p76lkcy38564kypyk3gs47wxd8vvsjvugv2r5g0uvvzdqa7n9mes9qyyssqqpgtfyjtaqqs477r40dyvj2rj0ywlr9zx0hmljvkhatuahlyk4vjkv5hy0e0pad7txp89vnatdrnxl8z9gvehvvyfq8xpatdrcxu9cgpwwt9v6

f495d79433c47c6748cf1f13b81d4b4880a4b2bdded2e5038ec50cf73e667a1f

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.

1 participant