dolysis
/
rocksdb
mirror of https://github.com/facebook/rocksdb.git
3
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Suppress TSAN reports on AutoHyperClockTable::Lookup (#11806) 

Summary:
This function uses racing reads for heuristic performance improvement. My change in https://github.com/facebook/rocksdb/issues/11792 only worked for clang, not gcc, and gcc does not accurately handle TSAN suppressions. I would have to mark much more code as suppressed than I want to.

So I've taken a different approach: TSAN build does not use the racing reads but substitutes random results, as an extra test that a "correct" value is not needed for correct overall behavior.

Pull Request resolved: https://github.com/facebook/rocksdb/pull/11806

Test Plan: manual TSAN builds & tests with cache_bench

Reviewed By: ajkr

Differential Revision: D49100115

Pulled By: pdillinger

fbshipit-source-id: d6d0dfb796d710b953212dd3fc171b6e88fadea1
This commit is contained in:
Peter Dillinger 2023-09-08 10:50:47 -07:00 committed by Facebook GitHub Bot
parent 05183bedcc
commit e67ee46642
2 changed files with 9 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
cache/clock_cache.cc vendored
View File

@ -3089,13 +3089,15 @@ AutoHyperClockTable::HandleImpl* AutoHyperClockTable::Lookup(
HandleImpl* h = &arr[GetNextFromNextWithShift(next_with_shift)]; HandleImpl* h = &arr[GetNextFromNextWithShift(next_with_shift)];
// Attempt cheap key match without acquiring a read ref. This could give a // Attempt cheap key match without acquiring a read ref. This could give a
// false positive, which is re-checked after acquiring read ref, or false // false positive, which is re-checked after acquiring read ref, or false
// negative, which is re-checked in the full Lookup. // negative, which is re-checked in the full Lookup. Also, this is a
// technical UB data race according to TSAN, but we don't need to read
// ReadAllowRace suppresses TSAN report on these reads. Also, using // a "correct" value here for correct overall behavior.
// & rather than && to give more flexibility to the compiler and CPU, #ifdef __SANITIZE_THREAD__
// as it is safe to access [1] even if [0] doesn't match. bool probably_equal = Random::GetTLSInstance()->OneIn(2);
if ((int{ReadAllowRace(h->hashed_key[0]) == hashed_key[0]} & #else
int{ReadAllowRace(h->hashed_key[1]) == hashed_key[1]})) { bool probably_equal = h->hashed_key == hashed_key;
#endif
if (probably_equal) {
// Increment acquire counter for definitive check // Increment acquire counter for definitive check
uint64_t old_meta = h->meta.fetch_add(ClockHandle::kAcquireIncrement, uint64_t old_meta = h->meta.fetch_add(ClockHandle::kAcquireIncrement,
std::memory_order_acquire); std::memory_order_acquire);

7
port/lang.h
View File

@ -69,13 +69,6 @@ constexpr bool kMustFreeHeapAllocations = false;
#define TSAN_SUPPRESSION #define TSAN_SUPPRESSION
#endif // TSAN_SUPPRESSION #endif // TSAN_SUPPRESSION
// Read memory while allowing data races. Only use where it is OK to read
// the wrong value, e.g. where reading the latest value improves performance.
template <typename T>
TSAN_SUPPRESSION inline T ReadAllowRace(const T& v) {
return v;
}
// Compile-time CPU feature testing compatibility // Compile-time CPU feature testing compatibility
// //
// A way to be extra sure these defines have been included. // A way to be extra sure these defines have been included.