From e67ee46642cbb29ec39d9365225cd4dec15ccff7 Mon Sep 17 00:00:00 2001
From: Peter Dillinger <peterd@meta.com>
Date: Fri, 8 Sep 2023 10:50:47 -0700
Subject: [PATCH] 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
---
 cache/clock_cache.cc | 16 +++++++++-------
 port/lang.h          |  7 -------
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/cache/clock_cache.cc b/cache/clock_cache.cc
index caa7db2f46..c7a8cf3936 100644
--- a/cache/clock_cache.cc
+++ b/cache/clock_cache.cc
@@ -3089,13 +3089,15 @@ AutoHyperClockTable::HandleImpl* AutoHyperClockTable::Lookup(
     HandleImpl* h = &arr[GetNextFromNextWithShift(next_with_shift)];
     // 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
-    // negative, which is re-checked in the full Lookup.
-
-    // ReadAllowRace suppresses TSAN report on these reads. Also, using
-    // & rather than && to give more flexibility to the compiler and CPU,
-    // as it is safe to access [1] even if [0] doesn't match.
-    if ((int{ReadAllowRace(h->hashed_key[0]) == hashed_key[0]} &
-         int{ReadAllowRace(h->hashed_key[1]) == hashed_key[1]})) {
+    // 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
+    // a "correct" value here for correct overall behavior.
+#ifdef __SANITIZE_THREAD__
+    bool probably_equal = Random::GetTLSInstance()->OneIn(2);
+#else
+    bool probably_equal = h->hashed_key == hashed_key;
+#endif
+    if (probably_equal) {
       // Increment acquire counter for definitive check
       uint64_t old_meta = h->meta.fetch_add(ClockHandle::kAcquireIncrement,
                                             std::memory_order_acquire);
diff --git a/port/lang.h b/port/lang.h
index e0cb8da5d2..a4201ca3b2 100644
--- a/port/lang.h
+++ b/port/lang.h
@@ -69,13 +69,6 @@ constexpr bool kMustFreeHeapAllocations = false;
 #define 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
 //
 // A way to be extra sure these defines have been included.