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

Add statistic to track compaction prefetching memory usage #13302

Draft

archang19 wants to merge 8 commits into facebook:main from archang19:export-D68224419

Contributor

archang19 commented Jan 16, 2025 •

edited

Loading

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 FilePrefetchBuffer getting 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:

I searched for all the places with new FilePrefetchBuffer.
GetOrCreatePrefetchBuffer inside PrefetchBufferCollection is used for blob files only.
BlockBasedTableReader has a CreateFilePrefetchBuffer method which is used in PartitionedFilterBlockReader and PartitionedIndexReader.
CreateFilePrefetchBufferIfNotExists is used by BlockPrefetcher inside PrefetchIfNeeded, which is called by BlockBasedtableIterator and PartitionedIndexIterator.
I did not enable InternalStats for 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.

facebook-github-bot added the CLA Signed label

Contributor

facebook-github-bot commented Jan 16, 2025

This pull request was exported from Phabricator. Differential Revision: D68224419

facebook-github-bot added the fb-exported label

archang19 force-pushed the export-D68224419 branch from c76a690 to 998b94a Compare

January 16, 2025 00:05

Contributor

facebook-github-bot commented Jan 16, 2025

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

Contributor

facebook-github-bot commented Jan 16, 2025

This pull request was exported from Phabricator. Differential Revision: D68224419

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

314f2db

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from 998b94a to 314f2db Compare

January 16, 2025 18:04

Contributor

facebook-github-bot commented Jan 16, 2025

This pull request was exported from Phabricator. Differential Revision: D68224419

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

e63d9fc

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from 314f2db to e63d9fc Compare

January 16, 2025 18:23

Contributor

facebook-github-bot commented Jan 16, 2025

This pull request was exported from Phabricator. Differential Revision: D68224419

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

884ef9f

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from e63d9fc to 884ef9f Compare

January 16, 2025 19:42

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

c77e283

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from 884ef9f to c77e283 Compare

January 16, 2025 19:45

Contributor

facebook-github-bot commented Jan 16, 2025

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

Contributor

facebook-github-bot commented Jan 16, 2025

This pull request was exported from Phabricator. Differential Revision: D68224419

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

58ce655

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from c77e283 to 58ce655 Compare

January 16, 2025 19:51

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

aefe67b

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from 58ce655 to aefe67b Compare

January 16, 2025 19:54

Contributor

facebook-github-bot commented Jan 16, 2025

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

Contributor

facebook-github-bot commented Jan 16, 2025

This pull request was exported from Phabricator. Differential Revision: D68224419

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

d4fc198

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from aefe67b to d4fc198 Compare

January 16, 2025 21:16

Contributor

facebook-github-bot commented Jan 17, 2025

This pull request was exported from Phabricator. Differential Revision: D68224419

archang19 force-pushed the export-D68224419 branch from d4fc198 to d1ec645 Compare

January 17, 2025 00:04

archang19 added a commit to archang19/rocksdb that referenced this pull request


          Add compaction prefetching internal stats (facebook#13302)

d1ec645

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19

archang19 force-pushed the export-D68224419 branch from 4559ceb to 44b05de Compare

January 17, 2025 17:59

archang19 added 3 commits

January 17, 2025 10:12


          Add compaction prefetching internal stats (facebook#13302)

54b5eff

Summary: Pull Request resolved: facebook#13302

Differential Revision: D68224419

Pulled By: archang19


          Feed through internal stats to fileprefetchbuffer

cc8a329


          Typos and lint

2bd88d2

archang19 force-pushed the export-D68224419 branch from 8faada9 to 2bd88d2 Compare

January 17, 2025 18:12

Contributor

facebook-github-bot commented Jan 17, 2025

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

archang19 commented

View reviewed changes

db/compaction/compaction_job.cc

@@ @@ -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,

Contributor Author

archang19 Jan 17, 2025

This is the part that we care about the most IMO. We want to have internal_stats passed in from CompactionJob::Run

archang19 added 5 commits

January 17, 2025 10:20


          External file ingestion job can also pass in internal stats

71f1b60


          ImportColumnFamilyJob can pass in internal_stats

0433eea


          Repair, table_cache, version_builder, and version_set

23a6006


          Fix fuzzer, file prefetch buffer, block based table reader, sst file …

960e47c

…dumper


          Fix GetMapPropertyDbStats

402e8a5

archang19 changed the title ~~Add compaction prefetching internal stats~~ Add statistic to track compaction prefetching memory usage

This was referenced Jan 21, 2025

Track the total number of compaction sorted runs inside BackgroundCallCompaction #13320

Draft

Track the total number of compaction sorted runs from inside CompactionMergingIterator #13325

Open

archang19 marked this pull request as draft

January 24, 2025 18:15

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

Labels

CLA Signed fb-exported