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
Use system-wide thread ID in info log lines #9164
Conversation
This makes it easier to debug with tools like `ps`. The change only applies to builds with glibc 2.30+ and _GNU_SOURCE extensions enabled. We could adopt it in more cases by using the syscall but this is enough for our build. Replaces facebook#2973. Test Plan: will run some benchmarks and correlate with `ps`. Will also test with ROCKSDB_NO_FBCODE to verify the fallback behavior.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@ajkr has updated the pull request. You must reimport the pull request before landing. |
@ajkr has updated the pull request. You must reimport the pull request before landing. |
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
env/env_posix.cc
Outdated
static uint64_t gettid(pthread_t tid) { | ||
uint64_t thread_id = 0; | ||
uint64_t GetThreadID() const override { | ||
uint64_t thread_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I think it's still a good idea to initialize to 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@ajkr has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review!
env/env_posix.cc
Outdated
static uint64_t gettid(pthread_t tid) { | ||
uint64_t thread_id = 0; | ||
uint64_t GetThreadID() const override { | ||
uint64_t thread_id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
This makes it easier to debug with tools like
ps
. The change onlyapplies to builds with glibc 2.30+ and _GNU_SOURCE extensions enabled.
We could adopt it in more cases by using the syscall but this is enough
for our build.
Replaces #2973.
Test Plan:
ps -L
.Benchmark command:
Results before: 15.9MB/s, 15.8MB/s, 16.0MB/s
Results after: 16.3MB/s, 16.3MB/s, 15.8MB/s