Skip to content

Fix extremely slow stack traces in debug build#44569

Merged
alexey-milovidov merged 3 commits intomasterfrom
stack-traces-debug-build
Dec 27, 2022
Merged

Fix extremely slow stack traces in debug build#44569
alexey-milovidov merged 3 commits intomasterfrom
stack-traces-debug-build

Conversation

@alexey-milovidov
Copy link
Copy Markdown
Member

@alexey-milovidov alexey-milovidov commented Dec 24, 2022 •
edited
Loading

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

The issue sometimes appear in tests:

320545:2022.12.23 17:35:24.027438 [ 14029 ] {3f533464-96a6-4c21-8542-7dda8af4546b} <Debug> executeQuery: (from [::1]:60720) (comment: 01045_array_zip.sql) SELECT arrayZip(); (stage: Complete)
329175:2022.12.23 17:37:47.517614 [ 14029 ] {3f533464-96a6-4c21-8542-7dda8af4546b} <Error> executeQuery: Code: 42. DB::Exception: Function arrayZip needs at least one argument; passed 0.: While processing arrayZip(). (NUMBER_OF_ARGUMENTS_DOESNT_MATCH) (version 22.13.1.1) (from [::1]:60720) (comment: 01045_array_zip.sql) (in query: SELECT arrayZip();), Stack trace (when copying this message, always include the lines below):
360665:2022.12.23 17:47:13.727075 [ 14029 ] {3f533464-96a6-4c21-8542-7dda8af4546b} <Error> TCPHandler: Code: 42. DB::Exception: Function arrayZip needs at least one argument; passed 0.: While processing arrayZip(). (NUMBER_OF_ARGUMENTS_DOESNT_MATCH), Stack trace (when copying this message, always include the lines below):

It's unclear why it took more than 10 minutes (most likely - many parallel queries),
but even on my machine, it was more than 1 second.
Now 0.1 seconds.

Note: maybe we also need to mlock the debug info in the binary or put the binary into tmpfs.

@robot-ch-test-poll3 robot-ch-test-poll3 added pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR. labels Dec 24, 2022
alexey-milovidov
alexey-milovidov commented Dec 25, 2022
View reviewed changes
CMakeLists.txt
set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${COMPILER_FLAGS}")
set (CMAKE_CXX_FLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} -O3 ${DEBUG_INFO_FLAGS} ${CMAKE_CXX_FLAGS_ADD}")
set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 ${DEBUG_INFO_FLAGS} -fno-inline ${CMAKE_CXX_FLAGS_ADD}")
set (CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -O0 ${DEBUG_INFO_FLAGS} ${CMAKE_CXX_FLAGS_ADD}")
Copy link
Copy Markdown
Member Author

@alexey-milovidov alexey-milovidov Dec 25, 2022

Choose a reason for hiding this comment

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

Please note that -fno-inline is the default for -O0:

-fno-inline
           Do not expand any functions inline apart from those marked with the "always_inline" attribute.  This is the default when not optimizing.

@alexey-milovidov
Copy link
Copy Markdown
Member Author

alexey-milovidov commented Dec 25, 2022

#42509

azat
azat reviewed Dec 26, 2022
View reviewed changes
src/Common/CMakeLists.txt Outdated
endif ()

# Otherwise stack traces symbolization is painfully slow in debug build.
set_source_files_properties("Dwarf.cpp" PROPERTIES COMPILE_FLAGS "-O3")
Copy link
Copy Markdown
Member

@azat azat Dec 26, 2022

Choose a reason for hiding this comment

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

I guess -O2 is enough? (-O3 make it harder to debug)

Copy link
Copy Markdown
Member Author

@alexey-milovidov alexey-milovidov Dec 26, 2022

Choose a reason for hiding this comment

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

Yes, most likely it's enough.

Copy link
Copy Markdown
Member

@azat azat Dec 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

And there is already similar lines -

ClickHouse/src/CMakeLists.txt

Lines 342 to 347 in d8924f0

# Otherwise it will slow down stack traces printing too much.
set_source_files_properties(
Common/Elf.cpp
Common/Dwarf.cpp
Common/SymbolIndex.cpp
PROPERTIES COMPILE_FLAGS "-O3 ${WITHOUT_COVERAGE}")

@alexey-milovidov
Copy link
Copy Markdown
Member Author

alexey-milovidov commented Dec 26, 2022

#44615

@alexey-milovidov alexey-milovidov mentioned this pull request Dec 27, 2022
Merged
azat
azat reviewed Dec 27, 2022
View reviewed changes
contrib/libunwind-cmake/CMakeLists.txt

# We should enable optimizations (otherwise it will be too slow in debug)
# and disable sanitizers (otherwise infinite loop may happen)
target_compile_options(unwind PRIVATE -O3 -fno-exceptions -funwind-tables -fno-sanitize=all $<$<COMPILE_LANGUAGE:CXX>:-nostdinc++ -fno-rtti>)
Copy link
Copy Markdown
Member

@azat azat Dec 27, 2022

Choose a reason for hiding this comment

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

Maybe you can also switch to -O2 instead here and here

ClickHouse/src/CMakeLists.txt

Lines 342 to 347 in d8924f0

# Otherwise it will slow down stack traces printing too much.
set_source_files_properties(
Common/Elf.cpp
Common/Dwarf.cpp
Common/SymbolIndex.cpp
PROPERTIES COMPILE_FLAGS "-O3 ${WITHOUT_COVERAGE}")

This will allow to debug unwind issues in debug builds w/o rebuilding.

Copy link
Copy Markdown
Member Author

@alexey-milovidov alexey-milovidov Dec 27, 2022

Choose a reason for hiding this comment

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

Ok.

@alexey-milovidov
Copy link
Copy Markdown
Member Author

alexey-milovidov commented Dec 27, 2022

#44606

@KochetovNicolai KochetovNicolai self-assigned this Dec 27, 2022
@alexey-milovidov alexey-milovidov merged commit 3342b84 into master Dec 27, 2022
@alexey-milovidov alexey-milovidov deleted the stack-traces-debug-build branch December 27, 2022 11:31
This was referenced Dec 27, 2022
Merged
Merged
Merged
@azat
Copy link
Copy Markdown
Member

azat commented Dec 27, 2022

@alexey-milovidov BTW maybe it worth to add a test to ensure that this will not became worse? (since it can be pretty tricky to dig into this again)

This was referenced Dec 27, 2022
Merged
Merged
@alexey-milovidov
Copy link
Copy Markdown
Member Author

alexey-milovidov commented Dec 27, 2022

@azat I was thinking about it, but it's difficult to imagine this test being not fragile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@azat azat azat left review comments

@KochetovNicolai KochetovNicolai KochetovNicolai approved these changes

Assignees

@KochetovNicolai KochetovNicolai

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog submodule changed At least one submodule changed in this PR.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alexey-milovidov @azat @KochetovNicolai @robot-ch-test-poll3