Add statistic to track compaction prefetching memory usage #13302
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This pull request was exported from Phabricator. Differential Revision: D68224419 |
archang19
force-pushed
the
export-D68224419
branch
from
c76a690 to
998b94a
Compare
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was exported from Phabricator. Differential Revision: D68224419 |
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 16, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
998b94a to
314f2db
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68224419 |
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 16, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
314f2db to
e63d9fc
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68224419 |
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 16, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
e63d9fc to
884ef9f
Compare
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 16, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
884ef9f to
c77e283
Compare
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was exported from Phabricator. Differential Revision: D68224419 |
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 16, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
c77e283 to
58ce655
Compare
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 16, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
58ce655 to
aefe67b
Compare
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
This pull request was exported from Phabricator. Differential Revision: D68224419 |
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 16, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
aefe67b to
d4fc198
Compare
|
This pull request was exported from Phabricator. Differential Revision: D68224419 |
archang19
force-pushed
the
export-D68224419
branch
from
d4fc198 to
d1ec645
Compare
archang19
added a commit
to archang19/rocksdb
that referenced
this pull request
Jan 17, 2025
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
4559ceb to
44b05de
Compare
Summary: Pull Request resolved: facebook#13302 Differential Revision: D68224419 Pulled By: archang19
archang19
force-pushed
the
export-D68224419
branch
from
8faada9 to
2bd88d2
Compare
|
@archang19 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
archang19
commented
Jan 17, 2025
| @@ -781,7 +781,7 @@ Status CompactionJob::Run() { | |||
| cfd->internal_comparator(), files_output[file_idx]->meta, | |||
| /*range_del_agg=*/nullptr, | |||
| compact_->compaction->mutable_cf_options(), | |||
| /*table_reader_ptr=*/nullptr, | |||
There was a problem hiding this comment.
This is the part that we care about the most IMO. We want to have internal_stats passed in from CompactionJob::Run
archang19
changed the title
This was referenced Jan 21, 2025
facebook-github-bot
pushed a commit
that referenced
this pull request
Feb 6, 2025
…onMergingIterator (#13325) Summary: **This PR adds a new statistic to track the total number of sorted runs for running compactions.** Context: I am currently working on a separate project, where I am trying to tune the read request sizes made by `FilePrefetchBuffer` to the storage backend. In this particular case, `FilePrefetchBuffer` will issue larger reads and have to buffer larger read responses. This means we expect to see higher memory utilization. At least for the initial rollout, we only want to enable this optimization for compaction reads. **I want some way to get a sense of what the memory usage _impact_ will be if the prefetch read request size is increased from (for instance) 8MB to 64MB.** **If I know the number of files that compactions are actively reading from (i.e. the number of sorted runs / "input iterators"), I can determine how much the memory usage will increase if I bump up the readahead size inside `FilePrefetchBuffer`.** For instance, if there are 16 sorted runs at any given point in time and I bump up the readahead size by 64MB, I can project an increase of 16 * 64 MB. In most cases, the number of sorted runs processed per compaction is the number of L0 files plus the number of non-L0 levels. However, we need to be aware of exceptions like trivial compactions, deletion compactions, and subcompactions. This is a major reason why this PR chooses to implement the stats counting inside `CompactionMergingIterator`, since by the time we get down to that part of the stack, we know the "true" values for the number of input iterators / sorted runs. Alternatives considered: - #13299 gives you a histogram for the number of sorted runs ("input iterators") for a _single compaction_. While this statistic is interested and in the direction of what we want, we are going to be assessing the memory impact across _all_ compactions that are currently running. Thus, this statistic does not give us all the information we need. - #13302 gives you the total prefetch buffer memory usage, but it doesn't tell you what happens when the readahead size is increased. Furthermore, the code change is error prone and very "invasive" -- look at how many places in the code had to be updated. This would be useful in the future for general memory accounting purposes, but it does not serve our immediate needs. - #13320 aimed to track the same metric, but did this inside `DbImpl:: BackgroundCallCompaction`. It turns out that this does not handle the case where a compaction is divided into multiple subcompactions (in which case, there would be _more_ sorted runs being processed at the same time than you would otherwise predict.) The current PR handles subcompactions automatically, and I think it is cleaner overall. Note: When I attempted to put this statistic as part of the `cf_stats_value_` array, even after updating the array to use `std::atomic<uint64_t>`, I still was able to get assertions to _fail_ inside the crash tests. These assertions checked that the unsigned integer would not underflow below zero during compaction. I experimented for many hours but could not figure out a solution, even though it would seem like things "should" work with `fetch_add` and `fetch_sub`. One possibility is that the values in `cf_stats_value_` are being cleared to 0, but I added a `fprintf` to that portion of the code and didn't see it getting printed out before my assertions failed. Regardless, I think that this statistic is different enough from the CF-specific and the other DB-wide stats that the best solution is to just have it defined as a separate `std::atomic<uint64_t>`. I also do not want to spend more hours trying to debug why the crash test assertions break, when the solution in the current version of the PR can get the assertions to consistently pass. Pull Request resolved: #13325 Test Plan: - I updated one unit test to confirm that `num_running_compaction_sorted_runs` starts and ends at 0. This checks that all the additions and subtractions cancel out. I also made sure the statistic got incremented at least once. - When I added `fprintf` manually, I confirmed that my statistics updating code was being exercised numerous times inside `db_compaction_test`. I printed out the results before and after the increments/decrements, and the numbers looked good. - We will monitor the generated statistics after this PR is merged. - There are assertion checks after each increment and before each decrement. If there are bugs, the crash test will almost certainly find them, since they quickly found issues with my initial implementation for this PR which tried using the `cf_stats_value_` array (modified to use `std::atomic`). Reviewed By: anand1976, hx235 Differential Revision: D68527895 Pulled By: archang19 fbshipit-source-id: 135cf210e0ff1550ea28ae4384d429ae620b1784
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For the purpose of determining the impact of readahead size increases on compaction prefetch buffer memory usage, I am going with #13325 instead. However, making this PR was a useful exercise in figuring out the entire chain that leads to
FilePrefetchBuffergetting used, and it will be useful in the future if we do want to incorporate prefetching buffers into overall memory usage accounting.Some notes for future reference:
new FilePrefetchBuffer.GetOrCreatePrefetchBufferinsidePrefetchBufferCollectionis used for blob files only.BlockBasedTableReaderhas aCreateFilePrefetchBuffermethod which is used inPartitionedFilterBlockReaderandPartitionedIndexReader.CreateFilePrefetchBufferIfNotExistsis used byBlockPrefetcherinsidePrefetchIfNeeded, which is called byBlockBasedtableIteratorandPartitionedIndexIterator.InternalStatsfor all use cases because there are many, many places in the codebase, and for what we are interested in, I don't think all the usages are relevant. Right now, we care about the compaction prefetching buffer memory usage.