Support subcmpct using reserved resources for round-robin priority #10341
Conversation
9c2e415
to
df35a2d
Compare
9b5b52d
to
1bc9245
Compare
|
@littlepig2013 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
1bc9245
to
35b130f
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
35b130f
to
34aea5a
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
34aea5a
to
2d32d5a
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
2d32d5a
to
524db97
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
There was a problem hiding this comment.
Review summary to @littlepig2013 + @ajkr:
- I focused on reviewing resource reservation part of this PR (compaction_job.cc). My biggest advice is to use self-explanatory variables and function abstraction to clarify different layers of constraint we add to number of subcompactions allowed. Unit tests are missing for resource reservation logic at this point so I will take another round of review when they are in-place.
- I haven't looked into compaction related part much but my impression based on my offline chat with @littlepig2013 is that it's so much easier to understand the code after knowing the big ideas (i.e, the 4 constraints in compaction_level_picker.cc level ). So if possible and haven’t done so, please reflect those ideas into coding (e.g, better abstraction, naming, paragraph, space ... ) to ease up the code reading. This is not just for domain expert like @ajkr but also for non-experts (such as me ... :)
- An extra ask is to update PR summary before landing with more high-level descriptions like what you told me in the meeting but without getting into the codes too much (okay to cite function name though). This will save future developers' time from diving into coding just for big ideas.
Thank you!
524db97
to
400bde5
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
55d0a70
to
1319058
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
1319058
to
c1d5832
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
c1d5832
to
06352e9
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
|
@littlepig2013 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
06352e9
to
091a50c
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
|
@littlepig2013 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.
LGTM. The Shrink and Release functions and client usages look perfect. Acquire/Increase could perhaps be cleaned up a bit further (Increase doesn't account for the initial value of extra_num_subcompaction_threads_reserved_ being positive; Acquire has some higher-level logic than subcompaction resources by accounting for subcompactions run without resource reservations). I don't have a concrete proposal and worry if I did it'd conflict with earlier review suggestions, so think we should land it as is unless you have a proposal :).
What if we merge the logic of AcquireSubcompactionResources() into GenSubcompactionBoundaries() (per Hui's comment: #10341 (comment)). Then rename IncreaseSubcompactionResources() to AcquireSubcompactionResources() since the latter naming implies we are starting with zero reserved resources (which is indeed the case). Would that improve it? |
Yeah. I agree. Let me apply this change. Thanks! |
091a50c
to
48c9315
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
|
@littlepig2013 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…vel size is below `MaxBytesForLevel(level)`, split into sub-compactions, and execute them using reserved resources
48c9315
to
30da312
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
…vel size is below `MaxBytesForLevel(level)`, split into sub-compactions, and execute them using reserved resources
30da312
to
a327849
Compare
|
@littlepig2013 has updated the pull request. You must reimport the pull request before landing. |
|
@littlepig2013 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…acebook#10341) Summary: Earlier implementation of round-robin priority can only pick one file at a time and disallows parallel compactions within the same level. In this PR, round-robin compaction policy will expand towards more input files with respecting some additional constraints, which are summarized as follows: * Constraint 1: We can only pick consecutive files - Constraint 1a: When a file is being compacted (or some input files are being compacted after expanding), we cannot choose it and have to stop choosing more files - Constraint 1b: When we reach the last file (with the largest keys), we cannot choose more files (the next file will be the first one with small keys) * Constraint 2: We should ensure the total compaction bytes (including the overlapped files from the next level) is no more than `mutable_cf_options_.max_compaction_bytes` * Constraint 3: We try our best to pick as many files as possible so that the post-compaction level size can be just less than `MaxBytesForLevel(start_level_)` * Constraint 4: If trivial move is allowed, we reuse the logic of `TryNonL0TrivialMove()` instead of expanding files with Constraint 3 More details can be found in `LevelCompactionBuilder::SetupOtherFilesWithRoundRobinExpansion()`. The above optimization accelerates the process of moving the compaction cursor, in which the write-amp can be further reduced. While a large compaction may lead to high write stall, we break this large compaction into several subcompactions **regardless of** the `max_subcompactions` limit. The number of subcompactions for round-robin compaction priority is determined through the following steps: * Step 1: Initialized against `max_output_file_limit`, the number of input files in the start level, and also the range size limit `ranges.size()` * Step 2: Call `AcquireSubcompactionResources()`when max subcompactions is not sufficient, but we may or may not obtain desired resources, additional number of resources is stored in `extra_num_subcompaction_threads_reserved_`). Subcompaction limit is changed and update `num_planned_subcompactions` with `GetSubcompactionLimit()` * Step 3: Call `ShrinkSubcompactionResources()` to ensure extra resources can be released (extra resources may exist for round-robin compaction when the number of actual number of subcompactions is less than the number of planned subcompactions) More details can be found in `CompactionJob::AcquireSubcompactionResources()`,`CompactionJob::ShrinkSubcompactionResources()`, and `CompactionJob::ReleaseSubcompactionResources()`. Pull Request resolved: facebook#10341 Test Plan: Add `CompactionPriMultipleFilesRoundRobin[1-3]` unit test in `compaction_picker_test.cc` and `RoundRobinSubcompactionsAgainstResources.SubcompactionsUsingResources/[0-4]`, `RoundRobinSubcompactionsAgainstPressureToken.PressureTokenTest/[0-1]` in `db_compaction_test.cc` Reviewed By: ajkr, hx235 Differential Revision: D37792644 Pulled By: littlepig2013 fbshipit-source-id: 7fecb7c4ffd97b34bbf6e3b760b2c35a772a0657
Summary: Update HISTORY.md for CompactionPri::kRoundRobin. Detailed implementation can be found in [PR10107](#10107), [PR10227](#10227), [PR10250](#10250), [PR10278](#10278), [PR10316](#10316), and [PR10341](#10341) Pull Request resolved: #10421 Reviewed By: ajkr Differential Revision: D38194070 Pulled By: littlepig2013 fbshipit-source-id: 4ce153dc0bf22cd865d09c5429955023dbc90f37
Summary:
Earlier implementation of round-robin priority can only pick one file at a time and disallows parallel compactions within the same level. In this PR, round-robin compaction policy will expand towards more input files with respecting some additional constraints, which are summarized as follows:
mutable_cf_options_.max_compaction_bytesMaxBytesForLevel(start_level_)TryNonL0TrivialMove()instead of expanding files with Constraint 3More details can be found in
LevelCompactionBuilder::SetupOtherFilesWithRoundRobinExpansion().The above optimization accelerates the process of moving the compaction cursor, in which the write-amp can be further reduced. While a large compaction may lead to high write stall, we break this large compaction into several subcompactions regardless of the
max_subcompactionslimit. The number of subcompactions for round-robin compaction priority is determined through the following steps:max_output_file_limit, the number of input files in the start level, and also the range size limitranges.size()AcquireSubcompactionResources()when max subcompactions is not sufficient, but we may or may not obtain desired resources, additional number of resources is stored inextra_num_subcompaction_threads_reserved_). Subcompaction limit is changed and updatenum_planned_subcompactionswithGetSubcompactionLimit()ShrinkSubcompactionResources()to ensure extra resources can be released (extra resources may exist for round-robin compaction when the number of actual number of subcompactions is less than the number of planned subcompactions)More details can be found in
CompactionJob::AcquireSubcompactionResources(),CompactionJob::ShrinkSubcompactionResources(), andCompactionJob::ReleaseSubcompactionResources().Test Plan:
Add
CompactionPriMultipleFilesRoundRobin[1-3]unit test incompaction_picker_test.ccandRoundRobinSubcompactionsAgainstResources.SubcompactionsUsingResources/[0-4],RoundRobinSubcompactionsAgainstPressureToken.PressureTokenTest/[0-1]indb_compaction_test.cc