Disable omitting frame pointers for simpler unwind#103811
Conversation
Although it is not a problem in ClickHouse, since we use libunwind that handles this well, it is a problem for perf, or if we will want to use eBPF to do some performance analysis (to avoid overhead in the clickhouse-server process), though there are also other options [1]. [1]: parca-dev/parca-agent#948 Refs: https://www.brendangregg.com/blog/2024-03-17/the-return-of-the-frame-pointers.html
clickhouse-gh
Bot
added
the
pr-performance
![clickhouse-gh[bot]](https://avatars.githubusercontent.com/in/928874?s=60&v=4){kind=small}
| set (CMAKE_C_FLAGS_ADD "${CMAKE_C_FLAGS_ADD} -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer") | ||
| set (CMAKE_ASM_FLAGS_ADD "${CMAKE_ASM_FLAGS_ADD} -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer") | ||
| endif() | ||
| set (CMAKE_CXX_FLAGS_ADD "${CMAKE_CXX_FLAGS_ADD} -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer") |
There was a problem hiding this comment.
Making frame-pointer preservation unconditional for every build removes the previous opt-in behavior and will impose a permanent runtime overhead for production users (historically this cost is measurable in CPU-heavy workloads). Please keep a CMake switch so packagers/operators can choose this trade-off, for example defaulting to ON in CI/profiling builds but allowing explicit opt-out in throughput-sensitive deployments.
|
Enabling frame pointers is a nice move for easy debuggability with
|
Yes, this is the reason, though there are other options to overcome this:
It was optional already
This is just an experiment, if the performance cost will be visible it will not be applied
Yes, this is not that new format AFAIR, and does it support is a good idea right now is an open question. |
| <uncompressed_cache_size>1000000000</uncompressed_cache_size> | ||
| <asynchronous_metrics_update_period_s>10</asynchronous_metrics_update_period_s> | ||
| <remap_executable replace="replace">true</remap_executable> | ||
| <!-- FIXME: Broken w/o omitting frame pointers --> |
There was a problem hiding this comment.
This comment is a bit confusing as written: w/o omitting frame pointers reads ambiguously and can be interpreted in the opposite way. Please reword it explicitly (for example, Broken with frame pointers enabled) so future maintainers can immediately understand the condition.
There was a problem hiding this comment.
This FIXME means the new always-on frame-pointer policy is known to break remap_executable, but the PR only turns the setting off in the performance harness. Server.cpp still accepts remap_executable=true and calls remapExecutable, and ServerSettings.cpp still documents the setting, so users can still enable a combination this PR already knows is broken at startup. Please either make remapExecutable work with frame-pointer builds or reject remap_executable=true explicitly and document that restriction.
2 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):
Disable omitting frame pointers for simpler unwind
Although it is not a problem in ClickHouse, since we use libunwind that handles this well, it is a problem for perf, or if we will want to use eBPF to do some performance analysis (to avoid overhead in the clickhouse-server process), though there are also other options 1.
Refs: https://www.brendangregg.com/blog/2024-03-17/the-return-of-the-frame-pointers.html