Skip to content
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.

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

Non-initial file preloading should always prefetch index and filter #4852

Closed
wants to merge 1 commit into from

Conversation

siying
Copy link
Contributor

@siying siying commented Jan 8, 2019

Summary: #3340 introduces preloading when max_open_files != -1.
It doesn't preload index and filter in non-initial file loading case. This is a little bit too
complicated to understand. We observed in one MyRocks use case where the filter is expected to be
preloaded but is not. To simplify the use case, we simply always prefetch the index and filter.
They anyway is expected to be loaded in the file verification phase anyway.

Test Plan: Add a unit test to cover this code path. The test passes before and after the change, but
it verifies the expected behavior anyway.

Summary: facebook#3340 introduces preloading when max_open_files != -1.
It doesn't preload index and filter in non-initial file loading case. This is a little bit too
complicated to understand. We observed in one MyRocks use case where the filter is expected to be
preloaded but is not. To simplify the use case, we simply always prefetch the index and filter.
They anyway is expected to be loaded in the file verification phase anyway.

Test Plan: Add a unit test to cover this code path. The test passes before and after the change, but
it verifies the expected behavior anyway.
@siying
Copy link
Contributor Author

siying commented Jan 8, 2019

The prefetching of index and filter is supposed to happen anyway in the verify case, so there shouldn't be any behavior change in majority of the cases. Here are the call stack to preload it anyway:

#0  rocksdb::BlockBasedTable::GetFilter (this=0x7ffff6634350, prefetch_buffer=0x0, filter_blk_handle=..., is_a_filter_partition=<optimized out>, no_io=<optimized out>, get_context=0x0, prefix_extractor=0x0) at table/block_based_table_reader.cc:1620
#1  0x00000000006b1c37 in rocksdb::BlockBasedTable::GetFilter (get_context=0x0, no_io=false, prefetch_buffer=0x0, prefix_extractor=<optimized out>, this=0x7ffff6634350) at table/block_based_table_reader.cc:1586
#2  rocksdb::BlockBasedTable::PrefetchIndexAndFilterBlocks (rep=rep@entry=0x7ffff0f91a00, prefetch_buffer=0x7ffff362baa0, meta_iter=0x7ffff43ea000, new_table=new_table@entry=0x7ffff6634350, prefix_extractor=prefix_extractor@entry=0x0, prefetch_all=true, table_options=..., level=1, prefetch_index_and_filter_in_cache=true) at table/block_based_table_reader.cc:1161
#3  0x00000000006b8858 in rocksdb::BlockBasedTable::Open (ioptions=..., env_options=..., table_options=..., internal_comparator=..., file=..., file_size=859, table_reader=0x7ffff59aa8d0, prefix_extractor=0x0, prefetch_index_and_filter_in_cache=true, skip_filters=false, level=1, immortal_table=false, largest_seqno=4, tail_prefetch_stats=0x7ffff663e6a8) at table/block_based_table_reader.cc:864
#4  0x00000000006a6ef1 in rocksdb::BlockBasedTableFactory::NewTableReader (this=<optimized out>, table_reader_options=..., file=..., file_size=<optimized out>, table_reader=<optimized out>, prefetch_index_and_filter_in_cache=true) at table/block_based_table_factory.cc:206
#5  0x00000000005e1030 in rocksdb::TableCache::GetTableReader (this=this@entry=0x7ffff669d540, env_options=..., internal_comparator=..., fd=..., sequential_mode=sequential_mode@entry=false, readahead=0, record_read_stats=true, file_read_hist=0x7ffff6707fc8, table_reader=0x7ffff59aa8d0, prefix_extractor=0x0, skip_filters=false, level=1, prefetch_index_and_filter_in_cache=true, for_compaction=false) at db/table_cache.cc:124
#6  0x00000000005e1997 in rocksdb::TableCache::FindTable (this=this@entry=0x7ffff669d540, env_options=..., internal_comparator=..., fd=..., handle=handle@entry=0x7ffff59aa9c0, prefix_extractor=0x0, no_io=false, record_read_stats=true, file_read_hist=0x7ffff6707fc8, skip_filters=false, level=1, prefetch_index_and_filter_in_cache=true) at db/table_cache.cc:163
#7  0x00000000005e3215 in rocksdb::TableCache::NewIterator (this=0x7ffff669d540, options=..., env_options=..., icomparator=..., file_meta=..., range_del_agg=range_del_agg@entry=0x0, prefix_extractor=0x0, table_reader_ptr=0x0, file_read_hist=0x7ffff6707fc8, for_compaction=false, arena=0x0, skip_filters=false, level=1, smallest_compaction_key=0x0, largest_compaction_key=0x0) at db/table_cache.cc:234
#8  0x00000000004e65bc in rocksdb::CompactionJob::<lambda(rocksdb::Status&)>::operator()(rocksdb::Status &) const (__closure=__closure@entry=0x7ffff59aad10, output_status=...) at db/compaction_job.cc:655
#9  0x00000000004ef73a in rocksdb::CompactionJob::Run (this=this@entry=0x7ffff59ab2e0) at db/compaction_job.cc:675
#10 0x00000000005370e0 in rocksdb::DBImpl::BackgroundCompaction (this=this@entry=0x7ffff66c0000, made_progress=made_progress@entry=0x7ffff59ab72f, job_context=job_context@entry=0x7ffff59ab7d0, log_buffer=log_buffer@entry=0x7ffff59ab980, prepicked_compaction=prepicked_compaction@entry=0x7ffff0f30a00) at db/db_impl_compaction_flush.cc:2603
#11 0x000000000053ae34 in rocksdb::DBImpl::BackgroundCallCompaction (this=this@entry=0x7ffff66c0000, prepicked_compaction=prepicked_compaction@entry=0x7ffff0f30a00, bg_thread_pri=bg_thread_pri@entry=rocksdb::Env::LOW) at db/db_impl_compaction_flush.cc:2170
#12 0x000000000053b53e in rocksdb::DBImpl::BGWorkCompaction (arg=<optimized out>) at db/db_impl_compaction_flush.cc:1957
......

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@sagar0 sagar0 left a comment

Choose a reason for hiding this comment

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

lgtm. Thanks!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@siying is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

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

Successfully merging this pull request may close these issues.

None yet

3 participants