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

Enabled optimizations for Distribution configuration on Android #747

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

mihe
Copy link
Contributor

@mihe mihe commented Nov 8, 2023

This enables optimizations when building the Distribution configuration for Android, by simply copying the compile flags from CMAKE_CXX_FLAGS_RELEASE into CMAKE_CXX_FLAGS_DISTRIBUTION, which at the moment consists of -O3 and -DNDEBUG.

I'm not very familiar with Android Studio and Gradle, so I'm not sure what the deal has been until now, but I get the feeling that the buildTypes defined in the Gradle files are limited to debug and release, which presumably translate to something like CMAKE_BUILD_TYPE=Debug and CMAKE_BUILD_TYPE=Release respectively, meaning projects like PerformanceTest has only ever been running Release on Android?

In either case, when building from command-line using the NDK it seems to be like any other LLVM toolchain and you can use whatever configurations you want, and the Distribution configuration doesn't currently define any flags on Android, thereby omitting optimizations entirely.

I wasn't sure whether to copy the flags from CMAKE_CXX_FLAGS_RELEASE or whether to just define them explicitly, but seeing as how none of the other configurations have defined them explicitly I opted to simply copy them.

@mihe
Copy link
Contributor Author

mihe commented Nov 8, 2023

I guess there's also some argument for just using the same if-statement as Linux/macOS/iOS/MinGW/Emscripten as well, seeing as how you're otherwise missing out on things like the explicit -ffp-model. The problem with that is that you'd be overwriting a bunch of the arguments that the NDK adds on top through CMAKE_CXX_FLAGS, which currently consists of:

-DANDROID
-fdata-sections
-ffunction-sections
-funwind-tables
-fstack-protector-strong
-no-canonical-prefixes
-D_FORTIFY_SOURCE=2
-Wformat
-Werror=format-security
-fexceptions
-frtti

... with the latter two being configurable through CMAKE_ANDROID_EXCEPTIONS and CMAKE_ANDROID_RTTI starting with CMake 3.20.

Whether all that is desirable or not is a different story though.

@jrouwe jrouwe merged commit d8e0fec into jrouwe:master Nov 8, 2023
61 checks passed
@jrouwe
Copy link
Owner

jrouwe commented Nov 8, 2023

Thanks!

I think you're compiling a bit differently than I have been because I'm not using that CMakeLists.txt at all on Android. Instead I'm using:

https://github.com/jrouwe/JoltPhysics/blob/master/Build/Android/UnitTests/src/main/cpp/CMakeLists.txt

Which takes the compiler options from:

https://github.com/jrouwe/JoltPhysics/blob/master/Build/Android/UnitTests/build.gradle

One thing I'm wondering about the change you made is that you're will also be missing -I., so I'm a bit surprised that it works in its current form.

@mihe mihe deleted the android-optimizations branch November 8, 2023 19:44
@mihe
Copy link
Contributor Author

mihe commented Nov 8, 2023

I think you're compiling a bit differently than I have been because I'm not using that CMakeLists.txt at all on Android.

Ah, that would explain it I guess. I'm honestly not sure what the most canonical way is to compile a native shared library for Android, but just using the NDK as any other toolchain seemed like a nice and simple option, and seems to be fairly well-supported by CMake as well.

One thing I'm wondering about the change you made is that you're will also be missing -I., so I'm a bit surprised that it works in its current form.

What exactly is -I. meant to do? Wouldn't that add the working directory as an include path? I would imagine that would change depending on from where you execute the build command.

The root folder is otherwise added as an include path here:

target_include_directories(Jolt PUBLIC ${PHYSICS_REPO_ROOT})

@jrouwe
Copy link
Owner

jrouwe commented Nov 9, 2023

Ah yes, -I. is from the times that the build was done from the root folder and not from the Builds folder. I removed it and it seems to work fine without.

What do you think of #751 for supporting Android? This way you can stop Jolt from overriding the CXX flags and you still get the floating point model params.

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.

2 participants