From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:49
From: Thomas De Schampheleire <redacted>
Hello,
This patch series started with as main goal to fix the performance degradation
found when testing gRPC in combination with BR2_ENABLE_DEBUG.
Its implementation strives to settle a discussion that has happened several
times, regarding the CMAKE_BUILD_TYPE that Buildroot should set, by not forcing
anything upon the user but allowing the choice.
Additionally, it cleans up some related changes done in specific packages.
After this series, there are still packages that set CMAKE_BUILD_TYPE
explicitly, but the associated comments indicate that they are needed to avoid
build failures, so they are not touched.
Best regards,
Thomas
Thomas De Schampheleire (7):
core: introduce BR2_ENABLE_RUNTIME_DEBUG
core: enable 'NDEBUG' unless BR2_ENABLE_RUNTIME_DEBUG is set
package/pkg-cmake.mk: determine CMAKE_BUILD_TYPE depending on
BR2_ENABLE_RUNTIME_DEBUG
package/libjson: drop explicit '-DNDEBUG'
package/flare-engine: remove explicit setting of CMAKE_BUILD_TYPE
package/supertux: remove explicit setting of CMAKE_BUILD_TYPE
package/sysrepo: remove explicit setting of CMAKE_BUILD_TYPE
Config.in | 13 +++++++++++++
docs/manual/adding-packages-cmake.txt | 2 +-
package/Makefile.in | 3 +++
package/flare-engine/flare-engine.mk | 5 -----
package/libjson/libjson.mk | 2 +-
package/pkg-cmake.mk | 2 +-
package/supertux/supertux.mk | 2 --
package/sysrepo/sysrepo.mk | 2 --
8 files changed, 19 insertions(+), 12 deletions(-)
--
2.26.2
From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:50
From: Thomas De Schampheleire <redacted>
Some packages have optional runtime assertions, extra traces, or other
elements that can help in debugging problems. However, such runtime elements
can negatively influence performance.
In a test program performing 100K gRPC calls from a client to a local server
and receiving the returned response, we see following execution time:
- runtime debug enabled: 1065 seconds
- runtime debug disabled: 48 seconds
This is more than a factor 20 (!) difference. Analysis shows that the
problem mostly stems from libabseil-cpp (a dependency of gRPC) which enables
mutex deadlock analysis when the preprocessor flag 'NDEBUG' is not set,
which adds a 'backtrace()' call on every lock/unlock. Potentially worse,
when libunwind is enabled and linked with the test program, 'backtrace()' is
not provided by glibc but by libunwind itself.
For production systems, users expect good performance out-of-the-box. In the
example above, the difference is huge and unless explicitly tested and
analyzed, users may not realize that the performance could be much better.
Address this problem by introducing a new option BR2_ENABLE_RUNTIME_DEBUG,
which can be used by packages or package infrastructures to set the
necessary flags.
Note that BR2_ENABLE_RUNTIME_DEBUG is orthogonal to BR2_ENABLE_DEBUG: the
former changes runtime behavior, while the latter is only expected to add
debug symbols to the build. Today, the cmake build system does introduce a
runtime impact when BR2_ENABLE_DEBUG is set, but that will be rectified in a
subsequent commit.
Signed-off-by: Thomas De Schampheleire <redacted>
---
Config.in | 13 +++++++++++++
1 file changed, 13 insertions(+)
From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:51
From: Thomas De Schampheleire <redacted>
A common way to disable runtime assertions is by honoring the 'NDEBUG'
preprocessor flag. Set it when BR2_ENABLE_RUNTIME_DEBUG is disabled (the
default case).
Signed-off-by: Thomas De Schampheleire <redacted>
---
package/Makefile.in | 3 +++
1 file changed, 3 insertions(+)
From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:52
From: Thomas De Schampheleire <redacted>
The CMAKE_BUILD_TYPE is currently set as 'Debug' in case BR2_ENABLE_DEBUG is
set, and as 'Release' in other cases. However, while the description of
BR2_ENABLE_DEBUG is to enable debug symbols (no runtime impact), the 'Debug'
build type in CMake can actually have runtime impact. For one, because it
does not set -DNDEBUG like is done for 'Release', but also because packages
may do custom things based on it.
The question of which CMAKE_BUILD_TYPE Buildroot should set, be it 'Debug',
'Release', 'RelWithDebInfo' or others, has come up several times in the
past. See some references below:
- July 2016: switch from Debug to RelWithDebInfo:
https://git.buildroot.org/buildroot/commit/?id=4b0120183404913f7f7788ef4f0f6b51498ef363
- October 2016: switch from RelWithDebInfo back to Debug:
https://git.buildroot.org/buildroot/commit/?id=104bb29e0490bfb487e2e665448dd3ca07fcc2b5
and changes to make sure Buildroot's flags are respected:
https://git.buildroot.org/buildroot/commit/?id=12494ef48f893684d0800e7f6fe39a2ceaed0451
- August 2017: bug #10246 - "BR2_ENABLE_DEBUG does not have the expected
effect for cmake packages"
https://bugs.busybox.net/show_bug.cgi?id=10246
- August 2017: mail thread following bug #10246:
http://lists.busybox.net/pipermail/buildroot/2017-August/200778.html
In the last mail thread, Samuel Martin confirmed that the 'Release' build
type could be used in all cases, because Buildroot is actually making sure
that the optimization flags are those determined by Buildroot, not the
defaults of cmake, thanks to commit 12494ef48f.
But Arnout Vandecappelle objected to using always 'Release', stating that
users may actually want the extra assertions.
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, Buildroot can now cater
for all cases:
- use CMAKE_BUILD_TYPE=Release by default. This makes sure that there is no
unexpected performance degradation triggered by enabling BR2_ENABLE_DEBUG.
- users can optionally enable BR2_ENABLE_RUNTIME_DEBUG if they want runtime
debug info like assertions, at the risk of introducing performance
degradation. In this case, we switch to CMAKE_BUILD_TYPE=Debug.
- orthogonally to the above, BR2_ENABLE_DEBUG still determines passing the
'-g' flag to enable debug symbols, and BR2_OPTIMIZE_X still determines the
used optimization flags.
Signed-off-by: Thomas De Schampheleire <redacted>
---
docs/manual/adding-packages-cmake.txt | 2 +-
package/pkg-cmake.mk | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
@@ -100,7 +100,7 @@ typical packages will therefore only use a few of them. necessary to set them in the package's +*.mk+ file unless you want to override them:-** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_DEBUG+;+** +CMAKE_BUILD_TYPE+ is driven by +BR2_ENABLE_RUNTIME_DEBUG+; ** +CMAKE_INSTALL_PREFIX+; ** +BUILD_SHARED_LIBS+ is driven by +BR2_STATIC_LIBS+; ** +BUILD_DOC+, +BUILD_DOCS+ are disabled;
From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:53
From: Thomas De Schampheleire <redacted>
The passing of 'NDEBUG' is now steered by BR2_ENABLE_RUNTIME_DEBUG and
commonly set from package/Makefile.in.
Signed-off-by: Thomas De Schampheleire <redacted>
---
package/libjson/libjson.mk | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:54
From: Thomas De Schampheleire <redacted>
flare-engine set CMAKE_BUILD_TYPE=RelWithDebInfo to avoid '-pg' for
profiling.
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, this change should no
longer be necessary. Users that do not wish to have profiling information,
just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value), and those that
enable BR2_ENABLE_RUNTIME_DEBUG will get profiling.
Signed-off-by: Thomas De Schampheleire <redacted>
---
package/flare-engine/flare-engine.mk | 5 -----
1 file changed, 5 deletions(-)
@@ -14,11 +14,6 @@ FLARE_ENGINE_DEPENDENCIES += sdl2 sdl2_image sdl2_mixer sdl2_ttf# Don't use /usr/games and /usr/share/gamesFLARE_ENGINE_CONF_OPTS+=-DBINDIR=bin-DDATADIR=share/flare-# Don't use the default Debug type as it adds -pg (gprof)-ifeq ($(BR2_ENABLE_DEBUG),y)-FLARE_ENGINE_CONF_OPTS+=-DCMAKE_BUILD_TYPE=RelWithDebInfo-endif-ifeq ($(BR2_TOOLCHAIN_HAS_GCC_BUG_85180),y)# CMakeLists.txt sets CMAKE_CXX_FLAGS_<BUILD_TYPE> depending on# BUILD_TYPE, and this comes after the generic CMAKE_CXX_FLAGS.
From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:55
From: Thomas De Schampheleire <redacted>
supertux explicitly set CMAKE_BUILD_TYPE=Release, ignoring any possible
value of BR2_ENABLE_DEBUG (previously) or BR2_ENABLE_RUNTIME_DEBUG (now).
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, this change should no
longer be necessary. Users that do not wish to have profiling information,
just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value), and those that
enable BR2_ENABLE_RUNTIME_DEBUG will get profiling.
Signed-off-by: Thomas De Schampheleire <redacted>
---
package/supertux/supertux.mk | 2 --
1 file changed, 2 deletions(-)
From: Thomas De Schampheleire <hidden> Date: 2021-02-10 14:12:56
From: Thomas De Schampheleire <redacted>
sysrepo explicitly set CMAKE_BUILD_TYPE=Release, ignoring any possible
value of BR2_ENABLE_DEBUG (previously) or BR2_ENABLE_RUNTIME_DEBUG (now).
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, this change should no
longer be necessary. Users that do not wish to have additional runtime
debugging just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value).
Signed-off-by: Thomas De Schampheleire <redacted>
---
package/sysrepo/sysrepo.mk | 2 --
1 file changed, 2 deletions(-)
From: Jan Kundrát <hidden> Date: 2021-02-11 14:16:31
On st?eda 10. ?nora 2021 15:12:56 CET, Thomas De Schampheleire wrote:
quoted hunk
From: Thomas De Schampheleire <redacted>
sysrepo explicitly set CMAKE_BUILD_TYPE=Release, ignoring any possible
value of BR2_ENABLE_DEBUG (previously) or BR2_ENABLE_RUNTIME_DEBUG (now).
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, this change should no
longer be necessary. Users that do not wish to have additional runtime
debugging just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value).
Signed-off-by: Thomas De Schampheleire <redacted>
---
package/sysrepo/sysrepo.mk | 2 --
1 file changed, 2 deletions(-)
You'll need to pass -DREPO_PATH=/etc/sysrepo here so that sysrepo on the
target does not accidentally try to use a path that only exists on the
build host. See
https://github.com/sysrepo/sysrepo/blob/v1.4.104/CMakeLists.txt#L138-L144
for details.
After that, feel free to add my Reviewed-by.
Jan
From: Thomas De Schampheleire <hidden> Date: 2021-02-12 10:13:27
El jue, 11 feb 2021 a las 15:16, Jan Kundr?t ([off-list ref]) escribi?:
On st?eda 10. ?nora 2021 15:12:56 CET, Thomas De Schampheleire wrote:
quoted
From: Thomas De Schampheleire <redacted>
sysrepo explicitly set CMAKE_BUILD_TYPE=Release, ignoring any possible
value of BR2_ENABLE_DEBUG (previously) or BR2_ENABLE_RUNTIME_DEBUG (now).
With the introduction of BR2_ENABLE_RUNTIME_DEBUG, this change should no
longer be necessary. Users that do not wish to have additional runtime
debugging just keep BR2_ENABLE_RUNTIME_DEBUG disabled (default value).
Signed-off-by: Thomas De Schampheleire <redacted>
---
package/sysrepo/sysrepo.mk | 2 --
1 file changed, 2 deletions(-)