Change compaction_readahead_size default value to 2MB #11762
Conversation
db/db_impl/db_impl_open.cc
Outdated
| if (db_options.compaction_readahead_size == 0) { | ||
| return Status::InvalidArgument( |
There was a problem hiding this comment.
Won't this cause any existing options files to fail validation? Should the "Sanitize" code also be changed to say that if the value is 0, set it to the default?
There was a problem hiding this comment.
good point - let me think more about it
hx235
force-pushed
the
compaction_readahead_size_option
branch
2 times, most recently
from
7f83aaf
to
268c2d1
Compare
hx235
changed the title
hx235
changed the title
hx235
changed the title
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
268c2d1
to
8ae0ab8
Compare
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
8ae0ab8
to
c683317
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
There was a problem hiding this comment.
Have you considered not doing sanitization during DB open and instead applying the default when it is used? CompressionOptions::level (see compression functions like ZSTD_Compress()) and ColumnFamilyOptions::max_background_{jobs,flushes,compactions} (see GetBGJobLimits()) do this
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
c683317
to
d12892f
Compare
hx235
changed the title
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
d12892f
to
80fe865
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
80fe865
to
51a5438
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
51a5438
to
39f15a5
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| @@ -0,0 +1 @@ | |||
| As `Options::access_hint_on_compaction_start` is made to have no effect since #11658, under non-direct IO, setting `Options::compaction_readahead_size` to 0 may regress compaction read performance, because it will not have the readahead issued by Kernel, which is the behavior prior to #11658, nor the intended replacement of Kernel-issued readahead by RocksDB-issued readahead introduced in #11631 now requiring `Options::compaction_readahead_size > 0` | |||
There was a problem hiding this comment.
How about "compaction read performance will regress when Options::compaction_readahead_size is explicitly set to 0"?
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
39f15a5
to
59555e6
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
59555e6
to
653eeaf
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
hx235
force-pushed
the
compaction_readahead_size_option
branch
from
653eeaf
to
eb69853
Compare
|
@hx235 has updated the pull request. You must reimport the pull request before landing. |
|
@hx235 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: **Context/Summary:** After #11631, we rely on `compaction_readahead_size` for how much to read ahead for compaction read under non-direct IO case. #11658 therefore also sanitized 0 `compaction_readahead_size` to 2MB under non-direct IO, which is consistent with the existing sanitization with direct IO. However, this makes disabling compaction readahead impossible as well as add one more scenario to the inconsistent effects between `Options.compaction_readahead_size=0` during DB open and `SetDBOptions("compaction_readahead_size", "0")` . - `SetDBOptions("compaction_readahead_size", "0")` will disable compaction readahead as its logic never goes through sanitization above while `Options.compaction_readahead_size=0` will go through sanitization. Therefore we decided to do this PR. Pull Request resolved: #11762 Test Plan: Modified existing UTs to cover this PR Reviewed By: ajkr Differential Revision: D48759560 Pulled By: hx235 fbshipit-source-id: b3f85e58bda362a6fa1dc26bd8a87aa0e171af79
Recently in facebook#11762 the default of `compaction_readahead_size` changed from 0 to 2 MB. Closes: facebook#12088
Context/Summary:
After #11631, we rely on
compaction_readahead_sizefor how much to read ahead for compaction read under non-direct IO case. #11658 therefore also sanitized 0compaction_readahead_sizeto 2MB under non-direct IO, which is consistent with the existing sanitization with direct IO.However, this makes disabling compaction readahead impossible as well as add one more scenario to the inconsistent effects between
Options.compaction_readahead_size=0during DB open andSetDBOptions("compaction_readahead_size", "0").SetDBOptions("compaction_readahead_size", "0")will disable compaction readahead as its logic never goes through sanitization above whileOptions.compaction_readahead_size=0will go through sanitization.Therefore we decided to do this PR.
Test:
Modified existing UTs to cover this PR