155 proactive wbm flushes

[udi-speedb]

@isaac-io - This is a preliminary version not yet ready.
However, due to the time pressure, I would really appreciate your review.
Please focus on the production code parts and leave the unit testing part for later.
Thanks

[ayulas]

i left some comments. part of isaac i also thinks but i. didnt repeat

[udi-speedb]

i left some comments. part of isaac i also thinks but i. didnt repeat

@ayulas - I do not see any comment you have made.

Do you agree with all of Isaac's comments?

[udi-speedb]

@hilikspdb - I have added you as a reviewer as well as you are the original designer of the feature and your inputs would be helpful

[ayulas]

ayulas started a review
Pending
memtable/write_buffer_manager.cc
memory_used_.fetch_add(mem, std::memory_order_relaxed);
new_memory_used = ReserveMemWithCache(mem);
} else if (is_enabled) {
auto old_memory_used =
Member
@ayulas ayulas Pending
why we need ReserveMemWithCache to return a value. the ReserveMemWithCache inside set the memory_used_... so why not just in the ShouldInitiateAnotherFlushMemOnly scope not just load the current
memory_used_?

@ayulas Reply...
memtable/write_buffer_manager.cc
#ifndef ROCKSDB_LITE
assert(cache_res_mgr_ != nullptr);
// Use a mutex to protect various data structures. Can be optimized to a
// lock-free solution if it ends up with a performance bottleneck.
std::lock_guardstd::mutex lock(cache_res_mgr_mu_);

size_t new_mem_used = memory_used_.load(std::memory_order_relaxed) + mem;
size_t old_mem_used = memory_used_.load(std::memory_order_relaxed);
Member
@ayulas ayulas Pending
again why you change the current code?

@ayulas Reply...
memtable/write_buffer_manager.cc
} else if (is_enabled) {
[[maybe_unused]] const auto curr_memory_used =
auto old_memory_used =
Member
@ayulas ayulas Pending
as i mention up... why FreeMemWithCache return a value.... you can use load memory_used_ in
ShouldInitiateAnotherFlushMemOnly scope

@ayulas Reply...
memtable/write_buffer_manager.cc
std::unique_lockstd::mutex lock(flushes_initiators_mu_);
assert(IsInitiatorIdxValid(FindInitiator(initiator)) == false);
flush_initiators_.push_back({initiator, request});
num_initiators_ = flush_initiators_.size();
Member
@ayulas ayulas Pending
why you need num_initiators_ at all and not use the flush_initiators_.size() directly?

@ayulas Reply...
memtable/write_buffer_manager.cc
assert(IsInitiatorIdxValid(initiator_idx));

flush_initiators_.erase(flush_initiators_.begin() + initiator_idx);
num_initiators_ = flush_initiators_.size();

Member
@ayulas ayulas Pending
as previous comment

@ayulas Reply...
memtable/write_buffer_manager.cc
InstrumentedMutexUnlock flushes_mu_unlocker(flushes_mu_.get());

      std::unique_lock<std::mutex> initiators_lock(flushes_initiators_mu_);
      assert(next_candidate_initiator_idx_ < flush_initiators_.size());

Member
@ayulas ayulas Pending
the next_candidate_initiator_idx_ is calculate in the CalcNextCandidateInitiatorIdx function that do a module on
flush_initiators_ size so of course if you deregister a DB initiator from the flush_initiators_ container you might have an idx that is equal to the flush_initiators_ size.....
for example you have 3 DBs
the current flush one is idx 1 next is 2 now you deregister idx 1 , next_candidate_initiator_idx_ is 2 (eventhough its not really cause you erase idx 1.. so you will get this assert... i think that the use vector - push and erase depends on insertion vector idx ( next_candidate_initiator_idx_)is a bug

@ayulas Reply...
memtable/write_buffer_manager.cc
// increment & decrement cancel each other with respect to the recalc
++num_running_flushes_;
--num_flushes_to_initiate_;
was_flush_initiated = initiator.cb(kMinFlushSizes[iter]);
Member
@ayulas ayulas Pending
what happened if we have DB that has 2 CFs 1 CFs did small writes and it doesnt do any more. the second CF writes a lot . so it is the massive memory to free. so in that case the second CF memtable will be switch and new LOG (wal file) will be created but the old wal couldn never be released since the CF1 memtable is not going to be collected to be flushed....

[udi-speedb]

@ayulas - Thanks a lot for the review.

[ayulas]

actually you should be protected with the
if (total_size_to_flush < min_size_to_flush) {
return false;
}
cause in case no cfds in DB the total_size_to_flush should be 0 so it menas you passed min_size_to_flush 0 and this is a bug... (not related to this )
second i would have cgecked this after taking the InstrumentedMutexLock

[udi-speedb]

@Yuval-Ariel - Please run full QA on this branch. Thanks

[ayulas]

why not check if cfds is empty here and return? why move on?
also as i mention in previous review comment - eventhough you the cfds is not empty if all cfds are with no data you should have exit in total_size_to_flush < min_size_to_flush. so if you needed protect this you have a bug that min_size_to_flush is 0

[ayulas]

again exit immediately if cfds is empty...

[ayulas]

why you created 2 loops?? you can do it in 1

[udi-speedb]

@ayulas - I have pushed the changes we have discussed over zoom.
Please take a look and approve the PR so @Yuval-Ariel may start QA.
@yuval - Before you start the QA, please let me know and I will both rebase on latest origin/main and squash all of the commits. Thanks

[ayulas]

i approve. @Yuval-Ariel go ahead