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

Ensure that logger context name is used instead of MYNAME #420

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

nsavoire
Copy link
Collaborator

@nsavoire nsavoire commented Jul 26, 2024

What does this PR do?

Ensure that LOG_setname effectively changes name on log lines.

Motivation

Before this change, setting name with LOG_setname had no effect because all log macros were passing MYNAME as name to log function, that would override name set with LOG_setname.

@pr-commenter
Copy link

pr-commenter bot commented Jul 26, 2024

Benchmark results for collatz

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.18.0+5849ebfa.39514389 ddprof 0.18.0+d0dd23e1.40430912

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-collatz --preset cpu_only collatz_runner.sh same

@pr-commenter
Copy link

pr-commenter bot commented Jul 26, 2024

Benchmark results for BadBoggleSolver_run

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.18.0+5849ebfa.39514389 ddprof 0.18.0+d0dd23e1.40430912

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-bad-boggle-solver BadBoggleSolver_run work 1000 same

@@ -118,7 +118,7 @@ void LOG_set_logs_allowed_function(LogsAllowedCallback logs_allowed_function);
#define LG_IF_LVL_OK(level, ...) \
do { \
if (unlikely(LOG_is_logging_enabled_for_level(level))) { \
olprintfln(ABS(level), -1, MYNAME, __VA_ARGS__); \
olprintfln(ABS(level), -1, nullptr, __VA_ARGS__); \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor:
It is slightly strange to put a nullptr here.
I understand that we default to the context name

r1viollet
r1viollet previously approved these changes Jul 30, 2024
Copy link
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM
Please update description before merging 🙇

@nsavoire nsavoire force-pushed the nsavoire/fix_log_context_for_library branch from 5467ef9 to d0dd23e Compare July 30, 2024 09:43
@nsavoire nsavoire requested a review from r1viollet July 30, 2024 12:47
Copy link
Collaborator

@r1viollet r1viollet left a comment

Choose a reason for hiding this comment

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

LGTM

@nsavoire nsavoire merged commit 8852113 into main Jul 31, 2024
2 checks passed
@nsavoire nsavoire deleted the nsavoire/fix_log_context_for_library branch July 31, 2024 12:33
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.

2 participants