mapbox / mapbox/hpp-skel

make Release builds RelWithDebug

Open
#51 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
CMake
Stars
116
Forks
18
PR merge metrics
No merged PRs in 30d

Description

@GretaCB - Now that benchmarks are landing in #48, the problem arises of how to best profile those benchmarks. As we did last week, we will be profiling the benchmarks to get calltrees of what is taking time in order to understand where we might optimize. The challenge is that those calltrees are often truncated due to inlining. But we should be careful not to make decisions on debug builds, which change performance of the binaries.

In an ideal world we could have release binaries (that run as fast as possible) but that also have beautiful calltrees as if the binaries are not optimized by inlining.

While the world is not ideal, we can approximate this by using `RelWithDebug` which will add `-g`. Then we can also add `-fno-inline-functions`. This latter flag with disable inlining only (and all other optimizations will still be applied). This is dangerous for production binaries we might deploy (since it could slow the code down), but safe for benchmarks (as long as the slower code is not too unrepresentative of reality). See https://github.com/mapbox/cpp/blob/master/glossary.md#problem-of-debugging-release-crashes for more details.

To recap, I think we should test doing:

```diff
diff --git a/CMakeLists.txt b/CMakeLists.txt
index 17405d7..f64844b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -13,7 +13,8 @@ if (CMAKE_BUILD_TYPE STREQUAL "Debug")
set(OPTIMIZATION_FLAGS "-O0 -DDEBUG")
message("-- Configuring debug build")
else()
- set(OPTIMIZATION_FLAGS "-O3 -DNDEBUG")
+ # https://github.com/mapbox/cpp/blob/master/glossary.md#profiling-build
+ set(OPTIMIZATION_FLAGS "-O3 -DNDEBUG -fno-inline-functions")
message("-- Configuring release build")
endif()

diff --git a/Makefile b/Makefile
index ce7a832..98b8867 100644
--- a/Makefile
+++ b/Makefile
@@ -5,7 +5,8 @@ export WERROR ?= true
default: release

release:
- mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=Release -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .
+ # https://github.com/mapbox/cpp/blob/master/glossary.md#debuggable-release-build
+ mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=RelWithDebInfo -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .

debug:
mkdir -p build && cd build && cmake ../ -DCMAKE_BUILD_TYPE=Debug -DWERROR=$(WERROR) && VERBOSE=1 cmake --build .
```

If this does not slow down the benchmark results more than 5% and also results in better calltrees (no major missing data for in Activity Monitor like we saw in release mode), then I would feel comfortable with this being committed.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with CMakeLists.txt and Makefile, then inspect the release and debug build targets and the benchmark setup from issue #48. Build the proposed release configuration, profile the benchmarks, and compare the results against the stated 5% slowdown limit and the quality of the resulting calltrees.

Written by the indexing model from the issue text.

Assessment

Tech stack
cmake, cpp
Domain
build-system, performance
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.