Change is_range_del_table_empty_ flag to atomic #4801
Conversation
Summary: To avoid a race on the flag, make it an atomic_bool. This doesn't seem to significantly affect benchmarks. Test Plan: - run readwhilewriting benchmark with db_bench on a DB with range tombstones - run crashtests with tsan enabled
| @@ -409,7 +409,7 @@ class MemTable { | |||
| ConcurrentArena arena_; | |||
| std::unique_ptr<MemTableRep> table_; | |||
| std::unique_ptr<MemTableRep> range_del_table_; | |||
| bool is_range_del_table_empty_; | |||
| std::atomic_bool is_range_del_table_empty_; | |||
There was a problem hiding this comment.
looks like we use std::atomic much more often in the codebase, any idea if there is any difference?
There was a problem hiding this comment.
It's typedef'd to std::atomic<bool> (see "Type Aliases" section on https://en.cppreference.com/w/cpp/atomic/atomic). I can change it for consistency if you want though.
|
Note: this fix is mainly to fix CI annoyances. In practice, this can only happen between a concurrent reader and writer, where the writer issues a DeleteRange that is about to complete as the read is processed. We know the write hasn't completed yet because |
|
Thanks for writing down the details. |
|
Before landing I'm going to change the memory ordering to |
There was a problem hiding this comment.
@abhimadan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Summary: To avoid a race on the flag, make it an atomic_bool. This doesn't seem to significantly affect benchmarks. Pull Request resolved: facebook#4801 Differential Revision: D13523845 Pulled By: abhimadan fbshipit-source-id: 3bc29f53c50a4e06cd9f8c6232a4bb221868e055
Summary: To avoid a race on the flag, make it an atomic_bool. This
doesn't seem to significantly affect benchmarks.
Test Plan:
range tombstones