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 new stat rocksdb.table.open.prefetch.tail.read.bytes, rocksdb.table.open.prefetch.tail.{miss|hit} #11265

Closed
wants to merge 3 commits into from

Conversation

hx235
Copy link
Contributor

@hx235 hx235 commented Mar 3, 2023

Context/Summary:
We are adding new stats to measure behavior of prefetched tail size and look up into this buffer

The stat collection is done in FilePrefetchBuffer but only for prefetched tail buffer during table open for now using FilePrefetchBuffer enum. It's cleaner than the alternative of implementing in upper-level call places of FilePrefetchBuffer for table open. It also has the benefit of extensible to other types of FilePrefetchBuffer if needed. See db bench for perf regression concern.

Test:
- Piggyback on existing test
- rocksdb.table.open.prefetch.tail.miss is harder to UT so I manually set prefetch tail read bytes to be small and run db bench.

./db_bench -db=/tmp/testdb -statistics=true -benchmarks="fillseq" -key_size=32 -value_size=512 -num=5000 -write_buffer_size=655 -target_file_size_base=655 -disable_auto_compactions=false -compression_type=none -bloom_bits=3  -use_direct_reads=true
rocksdb.table.open.prefetch.tail.read.bytes P50 : 4096.000000 P95 : 4096.000000 P99 : 4096.000000 P100 : 4096.000000 COUNT : 225 SUM : 921600
rocksdb.table.open.prefetch.tail.miss COUNT : 91
rocksdb.table.open.prefetch.tail.hit COUNT : 1034

- No perf regression observed in db_bench

SETUP command: create same db with ~900 files for pre-change/post-change.

./db_bench -db=/tmp/testdb -benchmarks="fillseq" -key_size=32 -value_size=512 -num=500000 -write_buffer_size=655360  -disable_auto_compactions=true -target_file_size_base=16777216 -compression_type=none

TEST command 60 runs or til convergence: as suggested by @anand1976 and @akankshamahajan15, vary seek_nexts and async_io in testing.

./db_bench -use_existing_db=true -db=/tmp/testdb -statistics=false -cache_size=0 -cache_index_and_filter_blocks=false -benchmarks=seekrandom[-X60] -num=50000 -seek_nexts={10, 500, 1000} -async_io={0|1} -use_direct_reads=true

async io = 0, direct io read = true

  seek_nexts = 10, 30 runs seek_nexts = 500, 12 runs seek_nexts = 1000, 6 runs
pre-post change 4776 (± 28) ops/sec; 24.8 (± 0.1) MB/sec 288 (± 1) ops/sec; 74.8 (± 0.4) MB/sec 145 (± 4) ops/sec; 75.6 (± 2.2) MB/sec
post-change 4790 (± 32) ops/sec; 24.9 (± 0.2) MB/sec 288 (± 3) ops/sec; 74.7 (± 0.8) MB/sec 143 (± 3) ops/sec; 74.5 (± 1.6) MB/sec

async io = 1, direct io read = true

  seek_nexts = 10, 54 runs seek_nexts = 500, 6 runs seek_nexts = 1000, 4 runs
pre-post change 3350 (± 36) ops/sec; 17.4 (± 0.2) MB/sec 264 (± 0) ops/sec; 68.7 (± 0.2) MB/sec 138 (± 1) ops/sec; 71.8 (± 1.0) MB/sec
post-change 3358 (± 27) ops/sec; 17.4 (± 0.1) MB/sec 263 (± 2) ops/sec; 68.3 (± 0.8) MB/sec 139 (± 1) ops/sec; 72.6 (± 0.6) MB/sec

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from 4115cf6 to d847d87 Compare March 3, 2023 20:12
@hx235 hx235 changed the title [draft] New stat rocksdb.table.prefetch.tail.read.bytes Add new stat rocksdb.table.prefetch.tail.read.bytes Mar 3, 2023
@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

ajkr
ajkr previously approved these changes Mar 3, 2023
Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from d847d87 to dbb4b3d Compare March 3, 2023 21:20
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from dbb4b3d to dc04cbb Compare March 4, 2023 00:02
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from dc04cbb to 4f882d4 Compare March 4, 2023 00:03
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235 hx235 requested a review from ajkr March 10, 2023 08:20
@hx235 hx235 changed the title Add new stat rocksdb.table.prefetch.tail.read.bytes Add new stat rocksdb.table.open.prefetch.tail.read.bytes, rocksdb.table.open.prefetch.tail.{miss|hit} Mar 10, 2023
@hx235
Copy link
Contributor Author

hx235 commented Mar 10, 2023

@ajkr - not official review request but just want to let you know that after adding rocksdb.table.open.prefetch.tail.{miss|hit}, I realized there is a cleaner solution to rocksdb.table.open.prefetch.tail.read.bytes and a bug I made in the original PR. I decided to fix them in the same PR but will request again later since the original PR was accepted.

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from 7cd4630 to 26a5b2e Compare March 10, 2023 08:43
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235 hx235 changed the title Add new stat rocksdb.table.open.prefetch.tail.read.bytes, rocksdb.table.open.prefetch.tail.{miss|hit} [Draft]Add new stat rocksdb.table.open.prefetch.tail.read.bytes, rocksdb.table.open.prefetch.tail.{miss|hit} Mar 10, 2023
@hx235 hx235 added the WIP Work in progress label Mar 10, 2023
@hx235 hx235 changed the title [Draft]Add new stat rocksdb.table.open.prefetch.tail.read.bytes, rocksdb.table.open.prefetch.tail.{miss|hit} Add new stat rocksdb.table.open.prefetch.tail.read.bytes, rocksdb.table.open.prefetch.tail.{miss|hit} Mar 11, 2023
@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from 26a5b2e to 311455f Compare March 11, 2023 00:42
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235 hx235 dismissed ajkr’s stale review March 11, 2023 02:04

Making new changes to the PR that worth another review

@hx235
Copy link
Contributor Author

hx235 commented Mar 11, 2023

@ajkr - Hi it's ready for review! Sorry I decided to make changes to previous approved PR, including adding the new stats as they are too related. For that I have to click "dismiss" your previous approval and request a new review on the PR. Let me know if there is any concern/questions.

Main change: a cleaner solution to collect rocksdb.table.open.prefetch.tail.*.stats in FilePrefetchBuffer level by distinguishing different usage of FilePrefetchBuffer (extensible for future too if we want to know stat about certain FilePrefetchBuffer); fix a bug I made in the original PR by including system prefetch in the buffer byte; variable renaming and better tests

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from 311455f to fda63c9 Compare March 11, 2023 06:52
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@akankshamahajan15
Copy link
Contributor

FilePrefetchBuffer side changes LGTM.

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from fda63c9 to 4d7b9e7 Compare March 13, 2023 18:19
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@hx235
Copy link
Contributor Author

hx235 commented Mar 13, 2023

rebase and re-import (cuz I forgot to do that previously :P)

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from 4d7b9e7 to 706da85 Compare March 15, 2023 00:34
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from 706da85 to 6f66e4d Compare March 15, 2023 00:39
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

…for non-FilePrefetchBufferUsage::kTableOpenPrefetchTail
@hx235 hx235 force-pushed the prefetch_tail_read_bytes_stat branch from 6f66e4d to 4eac8b3 Compare March 15, 2023 00:44
@facebook-github-bot
Copy link
Contributor

@hx235 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@hx235 merged this pull request in bab5f9a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants