updates to logging to automatically include file name, function name … #9
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…and execution line. this will be useful for debugging. formating improvements will allow us to use GUI tools for log viewing
|
@travisdesell the idea was to improve logging by determining where a log function was invoked. Still need to compile and test this on Ubuntu and cluster. Working on my Mac without modifying CMake or environment variables. Some more work to be done to get log messages to be well formatted in output. Delimiting log message sections with ':' so we can use some GUI log viewers for search and filtering. |
|
travisdesell
requested changes
Oct 20, 2020
There was a problem hiding this comment.
I'm a little worried a bout performance effects of this change. The reason I had the va_list/va_start buried a bit further into the functions was to only use them when needed -- many times (depending on log level) they'll never be needed if the message is ignored.
|
Just a thought -- is it possible to put the guards in the |
|
My thought is that then the compiler might even be able to optimize out those statements. Which would be ideal especially if a block of code had a number of trace/debug statements that could really slow things down. |
|
Right that makes sense, i.e. ideally avoid prep work prior to log calls. I removed Logs that used So to get performance, shut logs off by potentially disabling them at compile time. Review existing use of log to make sure additional work to prepare a log message is minimized. |
|
@travisdesell so won't get optimized away because |
|
Yeah I think it's worth doing because branch prediction should be pretty good at catching them and it will save us unneeded function calls. I imagine eventually we could set things up where we also had a compile flag to specify logging levels if we really wanted to squeeze out some more performance. |
… provided argument parameter functions
|
@travisdesell So two observations implementing this.
|
jefhai
commented
Oct 27, 2020
| @@ -239,3 +235,8 @@ void Log::log(const char *file, size_t filelen, const char *func, size_t funclen | |||
| bool Log::at_level(log_level_t level) { | |||
| return level >= std_message_level || level >= file_message_level; | |||
| } | |||
|
|
|||
| bool Log::should_log(log_level_t level) { | |||
| return !(std_message_level < level && file_message_level < level) | |||
There was a problem hiding this comment.
@travisdesell I'll simplify this
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…and execution line. this will be useful for debugging. formating improvements will allow us to use GUI tools for log viewing