Skip to content

Add compaction explicit prefetch stats #13520

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add compaction explicit prefetch stats #13520

wants to merge 1 commit into from

Conversation

hx235
Copy link
Contributor

@hx235 hx235 commented Apr 4, 2025 •
edited
Loading

Context/Summary:
This PR adds new stats to measure compaction readahead size for rocksdb managed prefetching (not FS prefetching). It can be used to verify compaction read-ahead is doing what's configured. This PR also excludes compaction readahead stats from user scan readahead stats measured in existing stats so there is a cleaner separating between these two.

Bonus: this PR also included some typo fixing about "io activities"

Test:
Modified existing test to verify stats

@hx235 hx235 force-pushed the tem_compaction_ra_stats branch 2 times, most recently from 3cc18c8 to 1dbafe4 Compare April 11, 2025 22:30
@hx235 hx235 changed the title [No merge] Temp compaction explicit readahead stats Compaction explicit prefetch stats Apr 11, 2025
@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 tem_compaction_ra_stats branch from 1dbafe4 to 100bd0f Compare April 11, 2025 23:32
@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 Compaction explicit prefetch stats Add compaction explicit prefetch stats Apr 11, 2025
@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 requested review from anand1976 and archang19 April 12, 2025 00:32
archang19
archang19 reviewed Apr 12, 2025
View reviewed changes
Copy link
Contributor

@archang19 archang19 left a comment

Choose a reason for hiding this comment

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

I think this change is fine but I am not sure if we need it. I am able to verify that compaction prefetching is working as expected based on the read request sizes that I record in Sally / see in our metrics.

Will follow up offline

unreleased_history/behavior_changes/ra_stats_user_only.md Outdated
@@ -0,0 +1 @@
Exclude prefetching bytes or hits for compaction read from stats `PREFETCH_BYTES_USEFUL`, `PREFETCH_HITS`, `PREFETCH_BYTES`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the current behavior, so I don't think we need this note.

Copy link
Contributor Author

@hx235 hx235 Apr 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch - I think what I was finding is about these stats also include reading non-data blocks upon file open in any occasion. Let me update my wording in the .md!

rocksdb::FilePrefetchBuffer::UpdateStats(bool, unsigned long) (/data/users/huixiao/rocksdb/file/file_prefetch_buffer.h:579)
rocksdb::FilePrefetchBuffer::TryReadFromCacheUntracked(rocksdb::IOOptions const&, rocksdb::RandomAccessFileReader*, unsigned long, unsigned long, rocksdb::Slice*, rocksdb::Status*, bool) (/data/users/huixiao/rocksdb/file/file_prefetch_buffer.cc:863)
rocksdb::FilePrefetchBuffer::TryReadFromCache(rocksdb::IOOptions const&, rocksdb::RandomAccessFileReader*, unsigned long, unsigned long, rocksdb::Slice*, rocksdb::Status*, bool) (/data/users/huixiao/rocksdb/file/file_prefetch_buffer.cc:763)
rocksdb::BlockFetcher::TryGetFromPrefetchBuffer() (/data/users/huixiao/rocksdb/table/block_fetcher.cc:79)
rocksdb::BlockFetcher::ReadBlockContents() (/data/users/huixiao/rocksdb/table/block_fetcher.cc:366)
rocksdb::(anonymous namespace)::ReadAndParseBlockFromFile<rocksdb::Block_kMetaIndex>(rocksdb::RandomAccessFileReader *, rocksdb::FilePrefetchBuffer *, const rocksdb::Footer &, const rocksdb::ReadOptions &, const rocksdb::BlockHandle &, std::unique_ptr<rocksdb::Block_kMetaIndex, std::default_delete<rocksdb::Block_kMetaIndex> > *, const rocksdb::ImmutableOptions &, rocksdb::BlockCreateContext &, bool, const rocksdb::UncompressionDict &, const rocksdb::PersistentCacheOptions &, rocksdb::MemoryAllocator *, bool, bool) (/data/users/huixiao/rocksdb/table/block_based/block_based_table_reader.cc:218)
rocksdb::BlockBasedTable::ReadMetaIndexBlock(rocksdb::ReadOptions const&, rocksdb::FilePrefetchBuffer*, std::unique_ptr<rocksdb::Block, std::default_delete<rocksdb::Block>>*, std::unique_ptr<rocksdb::InternalIteratorBase<rocksdb::Slice>, std::default_delete<rocksdb::InternalIteratorBase<rocksdb::Slice>>>*) (/data/users/huixiao/rocksdb/table/block_based/block_based_table_reader.cc:1307)
rocksdb::BlockBasedTable::Open(rocksdb::ReadOptions const&, rocksdb::ImmutableOptions const&, rocksdb::EnvOptions const&, rocksdb::BlockBasedTableOptions const&, rocksdb::InternalKeyComparator const&, std::unique_ptr<rocksdb::RandomAccessFileReader, std::default_delete<rocksdb::RandomAccessFileReader>>&&, unsigned long, unsigned char, std::unique_ptr<rocksdb::TableReader, std::default_delete<rocksdb::TableReader>>*, unsigned long, std::shared_ptr<rocksdb::CacheReservationManager>, std::shared_ptr<rocksdb::SliceTransform const> const&, bool, bool, int, bool, unsigned long, bool, rocksdb::TailPrefetchStats*, rocksdb::BlockCacheTracer*, unsigned long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char>> const&, unsigned long, std::array<unsigned long, 2ul>, bool) (/data/users/huixiao/rocksdb/table/block_based/block_based_table_reader.cc:727)
rocksdb::BlockBasedTableFactory::NewTableReader(rocksdb::ReadOptions const&, rocksdb::TableReaderOptions const&, std::unique_ptr<rocksdb::RandomAccessFileReader, std::default_delete<rocksdb::RandomAccessFileReader>>&&, unsigned long, std::unique_ptr<rocksdb::TableReader, std::default_delete<rocksdb::TableReader>>*, bool) const (/data/users/huixiao/rocksdb/table/block_based/block_based_table_factory.cc:581)

utilities/fault_injection_fs.h Outdated
@@ -424,9 +424,10 @@ class FaultInjectionTestFS : public FileSystemWrapper {
allow_link_open_file_ = allow_link_open_file;
}

bool ShouldIOActivtiesExcludedFromFaultInjection(Env::IOActivity io_activty) {
bool ShouldIOActivtiesExcludedFromFaultInjection(
Copy link
Contributor

Choose a reason for hiding this comment

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

did you mean to include this in this pr? if we are going to clean up spelling, there is also io_activties_excluded_from_fault_injection and ShouldIOActivtiesExcludedFromFaultInjection

Copy link
Contributor Author

@hx235 hx235 Apr 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch - let me include those as well and updated my PR summary for that!

@hx235
Copy link
Contributor Author

hx235 commented Apr 12, 2025

I am able to verify that compaction prefetching is working as expected based on the read request sizes that I record in Sally / see in our metrics

Yeah - I believe most of the IO stats can be done by extra work under the RocksDB through file system with the passed-down IOActivity with the help of offset and count. This PR is really here to save that work. This particular one was previously requested by internal user not using Sally and recently another user on Sally as well. I will loop you in that conversation!

By "our metrics", are you referring to RocksDB metrics? By the way, it felt slightly not symmetric while we have similar metrics for user scan but not compaction read-ahead. But that could just be my personal feeling.

@hx235 hx235 force-pushed the tem_compaction_ra_stats branch from 100bd0f to 5432189 Compare April 12, 2025 22:05
@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.

@archang19
Copy link
Contributor

By "our metrics", are you referring to RocksDB metrics?
I was able to confirm the compaction readahead sizes through Scuba logs and Sally metrics.

By the way, it felt slightly not symmetric while we have similar metrics for user scan but not compaction read-ahead. But that could just be my personal feeling.
I agree. In fact, I think I even made a PR in the past to try to combine the stats into one, but we decided we want to maintain a separate metric just for user prefetches.

@facebook-github-bot
Copy link
Contributor

@hx235 merged this pull request in 29c6610.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archang19 archang19 archang19 approved these changes

@anand1976 anand1976 Awaiting requested review from anand1976

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hx235 @facebook-github-bot @archang19