Skip to content

Paired-Bloom-Filter: Balancing rounding to batches between the bottom-most level and other levels#371

Merged
Yuval-Ariel merged 1 commit into
mainfrom
paired_bloom_filter_second_improvement
Feb 13, 2023
Merged

Paired-Bloom-Filter: Balancing rounding to batches between the bottom-most level and other levels#371
Yuval-Ariel merged 1 commit into
mainfrom
paired_bloom_filter_second_improvement

Conversation

@noamhaham

Copy link
Copy Markdown
Contributor

No description provided.

@udi-speedb

Copy link
Copy Markdown
Contributor

@noamhaham - Please run make format before pushing (and please do so before pushing the next commit with the updates)

// round down to multiple of a Batch
rv = std::max<size_t>((rv / kBatchSizeInBytes) * kBatchSizeInBytes,
kBatchSizeInBytes);
// rv = std::max<size_t>((rv / kBatchSizeInBytes) * kBatchSizeInBytes,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please cleanup (remove the comment lines with "dead" code)

if (is_bottomost_)
{num_batches_ = (num_blocks_ / speedb_filter::kPairedBloomBatchSizeInBlocks);
} else{
num_batches_ = std::ceil((num_blocks_+0.0) / speedb_filter::kPairedBloomBatchSizeInBlocks);

@udi-speedb udi-speedb Jan 26, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Assuming you intend num_blocks_ to be a double in the calculation, please use
static_cast<double>(num_blocks_)

What I mean is to replace the "+0.0" with a static_cast

rv = std::max<size_t>((rv / kBatchSizeInBytes) * kBatchSizeInBytes,kBatchSizeInBytes);
}else{

rv=std::ceil<size_t>(((rv+0.0) / kBatchSizeInBytes)) * kBatchSizeInBytes;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here
you are using std::ceil as if it were a template which it isn't (std::ceil<size_t>)

Also, the function is called RoundDownUsableSpace and you may set rv to a value > available_size which I think is problematic (and definitely misleading).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is still the issue I have pointed out in the previous cycle:

Also, the function is called RoundDownUsableSpace and you may set rv to a value > available_size which I think is problematic (and definitely misleading).

@@ -571,8 +576,19 @@ size_t SpdbPairedBloomBitsBuilder::RoundDownUsableSpace(size_t available_size) {
}

// round down to multiple of a Batch

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove this comment if not applicable

@noamhaham noamhaham requested a review from udi-speedb February 5, 2023 13:25
{num_batches_ = (num_blocks_ / speedb_filter::kPairedBloomBatchSizeInBlocks);
} else{
num_batches_ = std::ceil(static_cast<double>(num_blocks_) / speedb_filter::kPairedBloomBatchSizeInBlocks);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indentation.
Did you run make format before committting?


}


Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indentation

rv = std::max<size_t>((rv / kBatchSizeInBytes) * kBatchSizeInBytes,kBatchSizeInBytes);
}else{

rv=std::ceil<size_t>(((rv+0.0) / kBatchSizeInBytes)) * kBatchSizeInBytes;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is still the issue I have pointed out in the previous cycle:

Also, the function is called RoundDownUsableSpace and you may set rv to a value > available_size which I think is problematic (and definitely misleading).

@noamhaham noamhaham requested a review from udi-speedb February 5, 2023 14:44
@udi-speedb udi-speedb changed the title Second improvement of paired bloom filter Paired-Bloom-Filter: Balancing rounding to batches between the bottom-most level and other levels Feb 6, 2023
@udi-speedb udi-speedb force-pushed the paired_bloom_filter_second_improvement branch from 73ddc73 to 42110dc Compare February 6, 2023 14:21
@udi-speedb

Copy link
Copy Markdown
Contributor

@noamhaham

  • Did the branch pass 'make check' (all of the unit tests)?
  • Did you run filter bench with the paired bloom filter and various bpk-s, including small ones?
  • I think you should test it using db_bench with compaction running and multiple levels (and our filter obviously) to verify it runs without any issues.
  • Before testing performance, QA (@Yuval-Ariel ) should probably db-stress test it, using our filter and when there are multiple levels (as the change / improvement deals with behaving differently in the bottomest level relative to higher levels)

@udi-speedb udi-speedb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@noamhaham - I am approving the code of the change itself. However, please see my comments / suggestions / questions. Thanks

@noamhaham

noamhaham commented Feb 6, 2023

Copy link
Copy Markdown
Contributor Author

@udi-speedb, @erez-speedb, @Yuval-Ariel

  1. Make check was completed successfully.
  2. I ran filter bench with 10 bits per key and with 20 bits per key
  3. We could add this kind of test to the performance tests (read while write)
  4. I agree about the db-stress
  5. @erez-speedb for performance tests on HDD please use the branch: paired_bloom_filter_balanced_rounding_for_tests

@udi-speedb udi-speedb force-pushed the paired_bloom_filter_second_improvement branch from 42110dc to bef6f1e Compare February 6, 2023 15:46
@Guyme Guyme requested a review from udi-speedb February 7, 2023 13:56
@udi-speedb udi-speedb force-pushed the paired_bloom_filter_second_improvement branch from bef6f1e to 93763e2 Compare February 11, 2023 09:30
udi-speedb
udi-speedb previously approved these changes Feb 11, 2023
@Yuval-Ariel

Copy link
Copy Markdown
Contributor

@udi-speedb , hey we'd like to get this into the upcoming version. can you fix the build?

@udi-speedb

Copy link
Copy Markdown
Contributor

@udi-speedb , hey we'd like to get this into the upcoming version. can you fix the build?

@Yuval-Ariel The commit has no changes in the file that seems to cacuse the issue (util/build_version.cc.in)?

@udi-speedb udi-speedb force-pushed the paired_bloom_filter_second_improvement branch from 93763e2 to e53149e Compare February 12, 2023 08:37
@udi-speedb

Copy link
Copy Markdown
Contributor

I have now cherry-picked the branch's commit on the latest origin/main and I hope that would solve it.

@udi-speedb

Copy link
Copy Markdown
Contributor

@Yuval-Ariel Did the trick. Please merge.

@udi-speedb

Copy link
Copy Markdown
Contributor

@Yuval-Ariel
I see that the black box test failed on the following:

***note: appending --bloom-bits (7.000000) to --filter-uri***
[2023-02-12 11:46:58.582634] Running db_stress with pid=14160: ./db_stress --acquire_snapshot_one_in=10000 --adaptive_readahead=0 --allow_concurrent_memtable_write=0 --allow_data_in_errors=True --async_io=0 --avoid_flush_during_recovery=0 --avoid_unnecessary_blocking_io=0 --backup_max_size=104857600 --backup_one_in=100000 --batch_protection_bytes_per_key=0 --block_size=4096 --bloom_bits=13 --bottommost_compression_type=zlib --bytes_per_sync=262144 --cache_index_and_filter_blocks=0 --cache_size=8388608 --cache_type=lru_cache --charge_compression_dictionary_building_buffer=0 --charge_file_metadata=0 --charge_filter_construction=0 --charge_table_reader=0 --checkpoint_one_in=1000000 --checksum_type=kXXH3 --clear_column_family_one_in=0 --compact_files_one_in=1000000 --compact_range_one_in=1000000 --compaction_pri=4 --compaction_ttl=1000 --compare_full_db_state_snapshot=1 --compression_max_dict_buffer_bytes=0 --compression_max_dict_bytes=0 --compression_parallel_threads=1 --compression_type=snappy --compression_use_zstd_dict_trainer=0 --compression_zstd_max_train_bytes=0 --continuous_verification_interval=0 --customopspercent=0 --data_block_hash_table_util_ratio=0.51 --data_block_index_type=1 --db=/tmp/rocksdb_crashtest_blackboxoogywfea --db_write_buffer_size=0 --delpercent=18 --delrangepercent=3 --destroy_db_initially=0 --detect_filter_construct_corruption=0 --disable_wal=0 --enable_compaction_filter=0 --enable_pipelined_write=0 --expected_values_dir=/tmp/rocksdb_crashtest_expected_gb2qn1ct --fail_if_options_file_error=1 --file_checksum_impl=crc32c --filter_uri= --flush_one_in=1000000 --format_version=5 --get_current_wal_file_one_in=0 --get_live_files_one_in=100000 --get_property_one_in=1000000 --get_sorted_wal_files_one_in=0 --index_block_restart_interval=8 --index_type=2 --ingest_external_file_one_in=0 --initial_auto_readahead_size=16384 --initiate_wbm_flushes=0 --iterpercent=23 --key_len_percent_dist=3,5,7,28,6,11,23,13,4 --level_compaction_dynamic_level_bytes=True --long_running_snapshots=1 --mark_for_compaction_one_file_in=10 --max_auto_readahead_size=0 --max_background_compactions=20 --max_bytes_for_level_base=10485760 --max_key=102400 --max_key_len=9 --max_manifest_file_size=1073741824 --max_write_batch_group_size_bytes=16777216 --max_write_buffer_number=3 --max_write_buffer_size_to_maintain=8388608 --memtable_prefix_bloom_size_ratio=0 --memtable_protection_bytes_per_key=1 --memtable_whole_key_filtering=0 --memtablerep=skip_list --mmap_read=0 --mock_direct_io=False --nooverwritepercent=40 --num_file_reads_for_auto_readahead=0 --num_iterations=51 --open_files=-1 --open_metadata_write_fault_one_in=0 --open_read_fault_one_in=0 --open_write_fault_one_in=16 --ops_per_thread=100000000 --optimize_filters_for_memory=1 --paranoid_file_checks=1 --partition_filters=1 --partition_pinning=3 --pause_background_one_in=1000000 --periodic_compaction_seconds=2 --prefix_size=-1 --prefixpercent=0 --prepopulate_block_cache=1 --progress_reports=0 --read_fault_one_in=0 --readahead_size=0 --readpercent=28 --recycle_log_file_num=1 --reopen=0 --ribbon_starting_level=999 --secondary_cache_fault_one_in=32 --secondary_cache_uri=compressed_secondary_cache://capacity=8388608;enable_custom_split_merge=true --seed=25961358 --set_options_one_in=10000 --snapshot_hold_ops=100000 --sst_file_manager_bytes_per_sec=104857600 --sst_file_manager_bytes_per_truncate=1048576 --stats_dump_period_sec=0 --subcompactions=3 --sync=0 --sync_fault_injection=1 --sync_wal_one_in=100000 --target_file_size_base=2097152 --target_file_size_multiplier=2 --test_batches_snapshots=0 --top_level_index_pinning=3 --unpartitioned_pinning=0 --use_direct_io_for_flush_and_compaction=1 --use_direct_reads=0 --use_dynamic_delay=0 --use_full_merge_v1=False --use_merge=0 --use_multiget=0 --user_timestamp_size=0 --value_size_mult=32 --verify_before_write=False --verify_checksum=1 --verify_checksum_one_in=1000000 --verify_db_one_in=100000 --verify_sst_unique_id_in_manifest=1 --wal_bytes_per_sync=524288 --wal_compression=zstd --write_buffer_size=134217728 --write_dbid_to_manifest=1 --writepercent=28


[2023-02-12 11:47:05.979145] WARNING: db_stress (pid=14160) ended before kill: exitcode=1

Exit Before Killing
stdout:

stderr:
WARNING: use_zstd_dict_trainer is false but zstd finalizeDictionary cannot be used because ZSTD 1.4.5+ is not linked with the binary. zstd dictionary trainer will be used.
WARNING: prefix_size is non-zero but memtablerep != prefix_hash
ASAN:DEADLYSIGNAL
=================================================================
==14160==ERROR: AddressSanitizer: FPE on unknown address 0x5[559](https://github.com/speedb-io/speedb/actions/runs/4155651486/jobs/7189154145#step:6:560)0387ce56 (pc 0x55590387ce56 bp 0x7ffd69e95220 sp 0x7ffd69e95210 T0)
    #0 0x55590387ce55 in rocksdb::FSFileState::DropRandomUnsyncedData(rocksdb::Random*) utilities/fault_injection_fs.cc:88
    #1 0x55590387d3bc in rocksdb::FaultInjectionTestFS::DropRandomUnsyncedFileData(rocksdb::Random*) utilities/fault_injection_fs.cc:880
    #2 0x5559027f7720 in rocksdb::StressTest::Open(rocksdb::SharedState*) db_stress_tool/db_stress_test_base.cc:2659
    #3 0x5559027fcabe in rocksdb::StressTest::InitDb(rocksdb::SharedState*) db_stress_tool/db_stress_test_base.cc:308
    #4 0x555902788b84 in rocksdb::RunStressTest(rocksdb::StressTest*) db_stress_tool/db_stress_driver.cc:76
    #5 0x5559026c4905 in rocksdb::db_stress_tool(int, char**) db_stress_tool/db_stress_tool.cc:366
    #6 0x7f964aeb5c86 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21c86)
    #7 0x5559026bfca9 in _start (/__w/speedb/speedb/db_stress+0x2bfca9)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: FPE utilities/fault_injection_fs.cc:88 in rocksdb::FSFileState::DropRandomUnsyncedData(rocksdb::Random*)
==14160==ABORTING

make[1]: *** [blackbox_crash_test] Error 2
make: *** [blackbox_asan_crash_test] Error 2
crash_test.mk:65: recipe for target 'blackbox_crash_test' failed
make[1]: Leaving directory '/__w/speedb/speedb'
Makefile:1045: recipe for target 'blackbox_asan_crash_test' failed
Error: Process completed with exit code 2.

However, the failure occurred when it wasn't using the paired bloom filter IMO. Are you familiar with this failure?

@Yuval-Ariel

Copy link
Copy Markdown
Contributor

no im not, i'll run it on main to see maybe its because of recent additions.

@Yuval-Ariel Yuval-Ariel merged commit fa371b1 into main Feb 13, 2023
@Yuval-Ariel Yuval-Ariel deleted the paired_bloom_filter_second_improvement branch May 11, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Paired-Bloom-Filter: Balancing rounding to batches between the bottom-most level and other levels

3 participants