Skip to content

Commit

Permalink
Address New Architecture performance regressions by properly setting …
Browse files Browse the repository at this point in the history
…NDEBUG (#36172)

Summary:
It looks like we're not properly setting `NDEBUG` for "non debug" builds with CMake (the name is terrible but that's what Buck uses originally).

This configures `NDEBUG` correctly so that is set only for release variants, so that also `REACT_NATIVE_DEBUG` is set correctly (and we don't fire asserts on release builds).
This should address several performance regression we saw for New Architecture on some release scenarios (credits to sammy-SC for spotting it).

## Changelog

[ANDROID] [FIXED] - Address New Architecture performance regressions by properly setting NDEBUG

Pull Request resolved: #36172

Test Plan:
I've tested this by checking the ninja output for the debug/release builds for the `YogaLayoutableShadowNode.cpp` file

### Debug (does not contain `-DNDEBUG`)

```
build ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir/YogaLayoutableShadowNode.cpp.o: CXX_COMPILER__rrc_view_Debug /Users/ncor/git/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp || cmake_object_order_depends_target_rrc_view
  DEFINES = -Drrc_view_EXPORTS
  DEP_FILE = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir/YogaLayoutableShadowNode.cpp.o.d
  FLAGS = -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -fexceptions -frtti -stdlib=libc++ -g  -fno-limit-debug-info -fPIC -Wall -Werror -std=c++17 -fexceptions -frtti -Wpedantic -Wno-gnu-zero-variadic-macro-arguments -DLOG_TAG=\"Fabric\" -DFOLLY_NO_CONFIG=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_MOBILE=1 -DFOLLY_HAVE_RECVMMSG=1 -DFOLLY_HAVE_PTHREAD=1 -DFOLLY_HAVE_XSI_STRERROR_R=1
  INCLUDES = -I/Users/ncor/git/react-native/ReactCommon -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/folly/. -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/glog/exported -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/double-conversion/. -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/boost/boost_1_76_0 -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/fmt/include -I/Users/ncor/git/react-native/ReactAndroid/src/main/jni/first-party/fbgloginit/. -I/Users/ncor/git/react-native/ReactCommon/jsi -I/Users/ncor/git/react-native/ReactCommon/logger/. -I/Users/ncor/git/react-native/ReactCommon/react/renderer/graphics/platform/android -I/Users/ncor/git/react-native/ReactAndroid/src/main/jni/first-party/fb/include -I/Users/ncor/git/react-native/ReactAndroid/src/main/jni/first-party/yogajni/jni -I/Users/ncor/git/react-native/ReactCommon/yoga/. -isystem /Users/ncor/.gradle/caches/transforms-3/ebdfaf25aad9044f80de924d25488688/transformed/fbjni-0.3.0/prefab/modules/fbjni/include
  OBJECT_DIR = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir
  OBJECT_FILE_DIR = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir
  TARGET_COMPILE_PDB = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir/
  TARGET_PDB = /Users/ncor/git/react-native/ReactAndroid/build/intermediates/cxx/Debug/193k1y15/obj/arm64-v8a/librrc_view.pdb
```

### Release (does contain `-DNDEBUG`)

```
build ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir/YogaLayoutableShadowNode.cpp.o: CXX_COMPILER__rrc_view_RelWithDebInfo /Users/ncor/git/react-native/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp || cmake_object_order_depends_target_rrc_view
  DEFINES = -Drrc_view_EXPORTS
  DEP_FILE = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir/YogaLayoutableShadowNode.cpp.o.d
  FLAGS = -DANDROID -fdata-sections -ffunction-sections -funwind-tables -fstack-protector-strong -no-canonical-prefixes -D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -fexceptions -frtti -stdlib=libc++ -O2 -g -DNDEBUG -fPIC -Wall -Werror -std=c++17 -fexceptions -frtti -Wpedantic -Wno-gnu-zero-variadic-macro-arguments -DLOG_TAG=\"Fabric\" -DFOLLY_NO_CONFIG=1 -DFOLLY_HAVE_CLOCK_GETTIME=1 -DFOLLY_USE_LIBCPP=1 -DFOLLY_MOBILE=1 -DFOLLY_HAVE_RECVMMSG=1 -DFOLLY_HAVE_PTHREAD=1 -DFOLLY_HAVE_XSI_STRERROR_R=1
  INCLUDES = -I/Users/ncor/git/react-native/ReactCommon -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/folly/. -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/glog/exported -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/double-conversion/. -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/boost/boost_1_76_0 -I/Users/ncor/git/react-native/ReactAndroid/build/third-party-ndk/fmt/include -I/Users/ncor/git/react-native/ReactAndroid/src/main/jni/first-party/fbgloginit/. -I/Users/ncor/git/react-native/ReactCommon/jsi -I/Users/ncor/git/react-native/ReactCommon/logger/. -I/Users/ncor/git/react-native/ReactCommon/react/renderer/graphics/platform/android -I/Users/ncor/git/react-native/ReactAndroid/src/main/jni/first-party/fb/include -I/Users/ncor/git/react-native/ReactAndroid/src/main/jni/first-party/yogajni/jni -I/Users/ncor/git/react-native/ReactCommon/yoga/. -isystem /Users/ncor/.gradle/caches/transforms-3/ebdfaf25aad9044f80de924d25488688/transformed/fbjni-0.3.0/prefab/modules/fbjni/include
  OBJECT_DIR = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir
  OBJECT_FILE_DIR = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir
  TARGET_COMPILE_PDB = ReactCommon/react/renderer/components/view/CMakeFiles/rrc_view.dir/
  TARGET_PDB = /Users/ncor/git/react-native/ReactAndroid/build/intermediates/cxx/RelWithDebInfo/53pv2v65/obj/arm64-v8a/librrc_view.pdb
```

Reviewed By: sammy-SC, cipolleschi

Differential Revision: D43344120

Pulled By: cortinico

fbshipit-source-id: e0567aec2742c5dab2d008cdcf198f34d5626b65
  • Loading branch information
cortinico authored and facebook-github-bot committed Feb 16, 2023
1 parent 40c687c commit 8486e19
Show file tree
Hide file tree
Showing 5 changed files with 20 additions and 1 deletion.
3 changes: 3 additions & 0 deletions ReactAndroid/src/main/jni/first-party/fb/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ add_compile_options(
-Wno-error=unused-but-set-variable
-DHAVE_POSIX_CLOCKS
)
if(${CMAKE_BUILD_TYPE} MATCHES Release)
add_compile_options(-DNDEBUG)
endif()

# Yogacore needs to link towards android and log from the NDK libs
target_link_libraries(fb dl android log)
Expand Down
6 changes: 6 additions & 0 deletions ReactCommon/hermes/executor/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,10 @@ if(${CMAKE_BUILD_TYPE} MATCHES Debug)
PRIVATE
-DHERMES_ENABLE_DEBUGGER=1
)
else()
target_compile_options(
hermes_executor_common
PRIVATE
-DNDEBUG
)
endif()
4 changes: 4 additions & 0 deletions ReactCommon/jsc/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,7 @@ target_link_libraries(jscruntime
# TODO: Remove this flag when ready.
# Android has this enabled by default, but the flag is still needed for iOS.
target_compile_options(jscruntime PRIVATE -DRN_FABRIC_ENABLED)

if(${CMAKE_BUILD_TYPE} MATCHES Release)
target_compile_options(jscruntime PRIVATE -DNDEBUG)
endif()
4 changes: 4 additions & 0 deletions ReactCommon/react/debug/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,7 @@ add_library(react_debug SHARED ${react_debug_SRC})
target_include_directories(react_debug PUBLIC ${REACT_COMMON_DIR})

target_link_libraries(react_debug log folly_runtime)

if(${CMAKE_BUILD_TYPE} MATCHES Release)
target_compile_options(react_debug PUBLIC -DNDEBUG)
endif()
4 changes: 3 additions & 1 deletion ReactCommon/react/debug/flags.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@
//
// Enable REACT_NATIVE_DEBUG if NDEBUG is not defined.
// Due to BUCK defaults in open-source, NDEBUG is always defined for all android
// builds (if you build without BUCK, this isn't an issue). Thus we introduce
// builds.
// If you build in OSS with CMake, you will have -DNDEBUG set only for release
// builds, therefore REACT_NATIVE_DEBUG will not be set. Here we introduce
// REACT_NATIVE_DEBUG that we use internally instead of NDEBUG that we can
// control and use as a more reliable xplat flag. For any build that doesn't
// have NDEBUG defined, we enable REACT_NATIVE_DEBUG for convenience.
Expand Down

0 comments on commit 8486e19

Please sign in to comment.