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

Improve C++ SDK perf 5x by respecting CMAKE_BUILD_TYPE and enabling mimalloc #4094

Merged
merged 12 commits into from
Oct 31, 2023
35 changes: 28 additions & 7 deletions rerun_cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,14 @@

# -----------------------------------------------------------------------------
# Arrow:
if (WIN32)
if(WIN32)
# This makes the setup a lot easier on Windows where we otherwise need to put Arrow.dll either in path or copy it with the executable.
set(RERUN_ARROW_LINK_SHARED_DEFAULT OFF)
else()
# On non-windows platforms ld search path settings alleviate this concern and this makes linking faster!
set(RERUN_ARROW_LINK_SHARED_DEFAULT ON)
endif()

option(RERUN_ARROW_LINK_SHARED "Link to the Arrow shared library" ${RERUN_ARROW_LINK_SHARED_DEFAULT})
option(RERUN_DOWNLOAD_AND_BUILD_ARROW "If enabled, arrow will be added as an external project and built with the minimal set required by the Rerun C++ SDK" ON)

Expand Down Expand Up @@ -159,10 +160,18 @@
set(DARROW_CXXFLAGS "")
endif()

if(NOT CMAKE_BUILD_TYPE)
set(ARROW_CMAKE_BUILD_TYPE Release)
# Workaround for https://stackoverflow.com/questions/35519538/visual-studio-code-tab-key-does-not-insert-a-tab
Wumpf marked this conversation as resolved.
Show resolved Hide resolved
# This works around linking issues on Windows we got after enabling mimalloc.
if(MSVC)
file(MAKE_DIRECTORY ${ARROW_DOWNLOAD_PATH}/src/arrow_cpp-build/debug/)
file(MAKE_DIRECTORY ${ARROW_DOWNLOAD_PATH}/src/arrow_cpp-build/relwithdebinfo/)
file(MAKE_DIRECTORY ${ARROW_DOWNLOAD_PATH}/src/arrow_cpp-build/release/)
endif()

if(CMAKE_BUILD_TYPE STREQUAL "Debug")
set(ARROW_CMAKE_PRESET ninja-debug-minimal)
else()
set(ARROW_CMAKE_BUILD_TYPE ${CMAKE_BUILD_TYPE})
set(ARROW_CMAKE_PRESET ninja-release-minimal)
endif()

ExternalProject_Add(
Expand All @@ -173,19 +182,18 @@
GIT_SHALLOW true
GIT_PROGRESS true
CMAKE_ARGS
--preset ninja-debug-minimal
--preset ${ARROW_CMAKE_PRESET}
-DARROW_BOOST_USE_SHARED=OFF
-DARROW_BUILD_SHARED=${ARROW_BUILD_SHARED}
-DARROW_BUILD_STATIC=${ARROW_BUILD_STATIC}
-DARROW_CXXFLAGS=${DARROW_CXXFLAGS}
-DARROW_IPC=ON
-DARROW_JEMALLOC=OFF
-DARROW_JEMALLOC=OFF # We encountered some build issues with jemalloc, use mimalloc instead.
-DARROW_MIMALLOC=ON
emilk marked this conversation as resolved.
Show resolved Hide resolved
-DARROW_USE_ASAN=OFF
-DARROW_USE_TSAN=OFF
-DARROW_USE_UBSAN=OFF
-DBOOST_SOURCE=BUNDLED
-DCMAKE_BUILD_TYPE=${ARROW_CMAKE_BUILD_TYPE}
-DCMAKE_INSTALL_PREFIX=${ARROW_DOWNLOAD_PATH}
-Dxsimd_SOURCE=BUNDLED
SOURCE_SUBDIR cpp
Expand All @@ -202,7 +210,20 @@
endif()
else()
add_library(RerunArrowTarget STATIC IMPORTED)

# Need to set the ARROW_STATIC define, otherwise arrow functions are dllimport decorated on Windows.
target_compile_definitions(RerunArrowTarget INTERFACE ARROW_STATIC)

# Unclear why this is only an issue on Windows, but we have to explicitely opt in the arrow bundled dependencies,

Check warning on line 217 in rerun_cpp/CMakeLists.txt

View workflow job for this annotation

GitHub Actions / Checks / Spell Check

"explicitely" should be "explicitly".
# otherwise're missing the symbols for mimalloc.
if(WIN32)
add_library(RerunArrowTargetBundledDeps STATIC IMPORTED)
add_dependencies(RerunArrowTargetBundledDeps arrow_cpp)
set_target_properties(RerunArrowTargetBundledDeps PROPERTIES
IMPORTED_LOCATION ${ARROW_DOWNLOAD_PATH}/lib/arrow_bundled_dependencies.lib
)
target_link_libraries(RerunArrowTarget INTERFACE RerunArrowTargetBundledDeps)
endif()
endif()

add_dependencies(RerunArrowTarget arrow_cpp)
Expand Down
2 changes: 1 addition & 1 deletion rerun_cpp/src/rerun/demo_utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ namespace rerun {
inline std::vector<T> linspace(T start, T end, size_t num) {
std::vector<T> linspaced(num);
std::generate(linspaced.begin(), linspaced.end(), [&, i = 0]() mutable {
return start + static_cast<T>(i++) * (end - start) / static_cast<T>(num - 1);
return static_cast<T>(start + static_cast<T>(i++) * (end - start) / static_cast<T>(num - 1));
Copy link
Member

Choose a reason for hiding this comment

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

this makes no sense to me - isn't this casting T to T? Are you sure this is needed?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's this weird quirk of C++ where char + char == int

Copy link
Member

Choose a reason for hiding this comment

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

(you know that weird quirk. Not like the others)
Bites me every time!

https://godbolt.org/z/4W8xYnqKq

actually only works with unsigned char, which is also the case I got a warning in (uint8_t)

Copy link
Member

Choose a reason for hiding this comment

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

God damn you C++

});
return linspaced;
}
Expand Down
Loading