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

Control release mode debug logging output by ENABLE_DEBUG_LOGGING option #5628

Conversation

wangyoucao577
Copy link
Contributor

Issue

What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.
#3427

Please read our documentation on release and version management.
If your PR is still work in progress please attach the relevant label.

Tasklist

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@wangyoucao577
Copy link
Contributor Author

As I write in #3427 (comment), the performance of debug log is acceptable.

@akashihi
Copy link
Contributor

Can it be a compile time option? So one will be able to choose, whether debug output should not be compiled at all in release mode, or compiled in but not visible by default?

@wangyoucao577
Copy link
Contributor Author

Can it be a compile time option? So one will be able to choose, whether debug output should not be compiled at all in release mode, or compiled in but not visible by default?

Technically it's ok to be a compile time option, actually the NDEBUG is already the one. However, is it really valuable to control it by compiling? Intutively people will expect to have the debug log simply by -l DEBUG at runtime, that what I was thought before really deep dive the code. It really made bothered me for a long time. So that I stronglly suggest to remove the compile time control.

@danpat
Copy link
Member

danpat commented Jun 30, 2020

I think whether performance is acceptable or not depents where the debug log output is being generated. If someone places a debug message in one of the important loops during data processing, even if logging is suppressed, it could increase processing time.

My preference would be to leave the current behaviour by default - compile-time-disable debug logging, but may be add a CMake rule that enables debug logging for release builds if it's desired.

@wangyoucao577 wangyoucao577 changed the title Remove compile time debug log control Control release mode debug logging output by ENABLE_DEBUG_LOGGING option Jul 29, 2020
@wangyoucao577
Copy link
Contributor Author

I think whether performance is acceptable or not depents where the debug log output is being generated. If someone places a debug message in one of the important loops during data processing, even if logging is suppressed, it could increase processing time.

My preference would be to leave the current behaviour by default - compile-time-disable debug logging, but may be add a CMake rule that enables debug logging for release builds if it's desired.

@danpat I have added cmake option ENABLE_DEBUG_LOGGING as you suggested. Its behavior is very similar with ENABLE_ASSERTIONS that defaully ON in debug mode and OFF in release mode. With -DENABLE_DEBUG_LOGGING=ON it can be enabled in release mode also. Please help to review it. Thanks!

@akashihi
Copy link
Contributor

akashihi commented Aug 3, 2020

Looks good for me!

@danpat @gardster Any objections to merge this?

@akashihi akashihi merged commit b9ebe0c into Project-OSRM:master Aug 11, 2020
@wangyoucao577 wangyoucao577 deleted the feature/disable-debug-log-compile-time-control branch August 11, 2020 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants