Skip to content

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

Merged
nsavoire merged 1 commit into
mainfrom
nsavoire/fix_log_context_for_library
Jul 31, 2024
Merged

Ensure that logger context name is used instead of MYNAME#420
nsavoire merged 1 commit into
mainfrom
nsavoire/fix_log_context_for_library

Conversation

@nsavoire

@nsavoire nsavoire commented Jul 26, 2024

Copy link
Copy Markdown
Collaborator

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

pr-commenter Bot commented Jul 26, 2024

Copy link
Copy Markdown

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

pr-commenter Bot commented Jul 26, 2024

Copy link
Copy Markdown

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

Comment thread include/logger.hpp Outdated
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
Copy Markdown
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

@r1viollet r1viollet left a comment

Copy link
Copy Markdown
Collaborator

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

@r1viollet r1viollet left a comment

Copy link
Copy Markdown
Collaborator

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
@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