-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
log decimal TID on linux #2973
log decimal TID on linux #2973
Conversation
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.
@ajkr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
the travis failure is in |
Do we still want this? I wrote it because someone internal requested it but didn't really see the point. It needs to be benchmarked if we're going to check it in. |
@ajkr it does seem to be a good diff in general. What benchmark do you mean? |
I don't quite remember. Maybe any workload that writes a lot of log messages. We can make sure the syscall we're using doesn't consume too much CPU compared to the library call we used before. Let me know if you want this PR and I can rebase. |
It sounds like a good one. I'm more concerned about how to close it:) Either closing with merging or not. I don't have any concern about merging it. |
Today I finally saw the point after trying to figure out what thread pool was running excessive parallel compactions. Will resurrect this. |
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.
Summary: 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 #2973. Pull Request resolved: #9164 Test Plan: - ran some benchmarks and correlated logged thread IDs with those shown by `ps -L`. - verified no noticeable regression in throughput for log heavy (more than 700k log lines and over 5k / second) scenario. Benchmark command: ``` $ TEST_TMPDIR=/dev/shm ./db_bench -benchmarks=filluniquerandom -compression_type=none -max_bytes_for_level_multiplier=2 -write_buffer_size=262144 -num_levels=7 -max_bytes_for_level_base=2097152 -target_file_size_base=524288 -level_compaction_dynamic_level_bytes=true -max_background_jobs=12 -num=20000000 ``` Results before: 15.9MB/s, 15.8MB/s, 16.0MB/s Results after: 16.3MB/s, 16.3MB/s, 15.8MB/s - Rely on CI to test the fallback behavior Reviewed By: riversand963 Differential Revision: D32399660 Pulled By: ajkr fbshipit-source-id: c24d44fdf7782faa616ef0a0964eaca3539d9c24
identify the thread using the output of
gettid()
syscall on Linux, which is a system-wide unique ID, unlikepthread_self()
. Also changed from hex to decimal to be compatible with tools liketop
.Test Plan:
top
entries with log entries