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

Add logging to the library #359

Merged
merged 7 commits into from
Apr 2, 2024
Merged

Add logging to the library #359

merged 7 commits into from
Apr 2, 2024

Conversation

nsavoire
Copy link
Collaborator

@nsavoire nsavoire commented Jan 11, 2024

  • Setup logger in library
  • Use DD_PROFILING_NATIVE_LIBRARY_LOG_MODE with fallback to DD_PROFILING_NATIVE_LOG_MODE to determine library log mode
  • Use DD_PROFILING_NATIVE_LIBRARY_LOG_LEVEL with fallback to DD_PROFILING_NATIVE_LOG_LEVEL to determine library log level
  • Add callback in logger to determine if log is allowed, this callback is used to disable logging when allocations are forbidden

@nsavoire nsavoire force-pushed the nsavoire/logging_in_library branch 2 times, most recently from fee1626 to b55c53b Compare January 11, 2024 22:27
@pr-commenter
Copy link

pr-commenter bot commented Jan 11, 2024

Benchmark results for collatz

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.17.0+5f192d11.31107662 ddprof 0.17.0+9790fe1d.31247570

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 Jan 11, 2024

Benchmark results for BadBoggleSolver_run

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.17.0+5f192d11.31107662 ddprof 0.17.0+9790fe1d.31247570

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

r1viollet
r1viollet previously approved these changes Jan 12, 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
Should we delay the change to 0.16, or do you feel the change is harmless ?

@nsavoire nsavoire force-pushed the nsavoire/avoid_allocations_in_mmap_hooks branch from 86873df to 91adb83 Compare January 12, 2024 14:05
Base automatically changed from nsavoire/avoid_allocations_in_mmap_hooks to main January 12, 2024 15:35
@nsavoire nsavoire dismissed r1viollet’s stale review January 12, 2024 15:35

The base branch was changed.

if (_state.pid == 0) {
_state.pid = getpid();
}
if (tl_state.tid == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe leaving a cassert is nice

r1viollet
r1viollet previously approved these changes Jan 19, 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

@nsavoire nsavoire force-pushed the nsavoire/logging_in_library branch 6 times, most recently from a7eadd5 to fbd0b92 Compare February 2, 2024 12:42
@nsavoire nsavoire requested a review from r1viollet February 2, 2024 12:43
#else
// DDPROF_CHECK_FATAL must not be used inside profiling library
# define DDPROF_CHECK_FATAL(condition, ...) \
static_assert( \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good thinking

r1viollet
r1viollet previously approved these changes Feb 13, 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

@nsavoire nsavoire force-pushed the nsavoire/logging_in_library branch from fbd0b92 to 26c39e0 Compare March 29, 2024 14:05
@nsavoire nsavoire requested a review from r1viollet March 29, 2024 14:06
@nsavoire nsavoire force-pushed the nsavoire/logging_in_library branch from 26c39e0 to 6ec12ac Compare March 29, 2024 14:20
* Setup logger in library
* Use DD_PROFILING_NATIVE_LIBRARY_LOG_MODE with fallback to
  DD_PROFILING_NATIVE_LOG_MODE to determine library log mode
* Use DD_PROFILING_NATIVE_LIBRARY_LOG_LEVEL with fallback to
  DD_PROFILING_NATIVE_LOG_LEVEL to determine library log level
* Add callback in logger to determine if log is allowed, this callback
  is used to disable logging when allocations are forbidden
@nsavoire nsavoire force-pushed the nsavoire/logging_in_library branch 2 times, most recently from a0e82a7 to f8c1438 Compare April 2, 2024 10:58
nsavoire added 2 commits April 2, 2024 11:10
Using arrays made the code incorrect when embedded data was not present,
since their addresses and the resulting computed size would be non-NULL.
@nsavoire nsavoire force-pushed the nsavoire/logging_in_library branch from f8c1438 to 9790fe1 Compare April 2, 2024 11:10
@nsavoire
Copy link
Collaborator Author

nsavoire commented Apr 2, 2024

@r1viollet
I fixed some annoying leak sanitizer false positive. PR should be ready now.

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 f44f806 into main Apr 2, 2024
2 checks passed
@nsavoire nsavoire deleted the nsavoire/logging_in_library branch April 2, 2024 14:27
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