Use all the bits (popcount) in FindFirstNonFullAfterResize and `P…

…repareInsertAfterSoo`. Motivation: the previous change (simplification of small table growth) caused quite a lot of tests to become flaky. In order to avoid that happening in the long run, we want to make order and relative order more randomized. Caveat: generally popcount has latency 3 and the change may add a bit of latency for the small table growth codepath. But there is a lot of other work to be done in parallel. Newly introduced `RelativeIterationOrderChangesByInstance` test verifies randomness in relative order for small tables. Before this change it was failing 5% of the time with 10K iterations. With new implementation it passes with 500 iterations. BM_SWISSMAP_InsertManyOrdered_Hot shows regression, but it is likely an artifact of the benchmark. Order in the benchmark is taken from the table that was filled without reserve. Now this order for small tables is more drastically different compared to order in reserved table. Macrobenchmarks are neutral. ``` name old cpu/op new cpu/op delta BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 4>/set_size:1/density:1 3.04ns ± 3% 3.04ns ± 3% ~ (p=0.598 n=34+34) BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 4>/set_size:2/density:1 3.12ns ± 3% 3.12ns ± 3% ~ (p=0.981 n=35+35) BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 4>/set_size:4/density:1 3.11ns ± 3% 3.11ns ± 3% ~ (p=0.888 n=35+35) BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 4>/set_size:8/density:1 3.15ns ± 3% 3.16ns ± 3% ~ (p=0.778 n=35+35) BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 64>/set_size:1/density:1 3.02ns ± 3% 3.01ns ± 3% ~ (p=0.322 n=34+35) BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 64>/set_size:2/density:1 3.12ns ± 3% 3.11ns ± 4% ~ (p=0.860 n=35+35) BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 64>/set_size:4/density:1 3.12ns ± 3% 3.12ns ± 3% ~ (p=0.895 n=35+34) BM_SWISSMAP_InsertMiss_Hot<::absl::flat_hash_set, 64>/set_size:8/density:1 3.17ns ± 2% 3.17ns ± 3% ~ (p=0.842 n=35+35) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 4>/set_size:1/density:1 3.34ns ± 3% 3.34ns ± 3% ~ (p=0.432 n=33+35) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 4>/set_size:2/density:1 3.30ns ± 3% 3.29ns ± 3% ~ (p=0.328 n=33+34) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 4>/set_size:4/density:1 3.30ns ± 3% 3.29ns ± 3% ~ (p=0.252 n=35+35) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 4>/set_size:8/density:1 3.30ns ± 4% 3.28ns ± 3% ~ (p=0.138 n=35+33) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 64>/set_size:1/density:1 3.40ns ± 2% 3.38ns ± 3% ~ (p=0.302 n=29+35) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 64>/set_size:2/density:1 3.46ns ± 3% 3.44ns ± 3% ~ (p=0.242 n=35+35) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 64>/set_size:4/density:1 3.46ns ± 3% 3.45ns ± 3% ~ (p=0.548 n=34+34) BM_SWISSMAP_InsertHit_Hot<::absl::flat_hash_set, 64>/set_size:8/density:1 3.45ns ± 3% 3.45ns ± 3% ~ (p=0.573 n=35+35) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 4>/set_size:1/density:1 13.6ns ± 3% 14.7ns ± 3% +8.35% (p=0.000 n=35+35) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 4>/set_size:2/density:1 12.3ns ± 3% 13.4ns ± 4% +8.92% (p=0.000 n=35+35) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 4>/set_size:4/density:1 12.3ns ± 3% 13.4ns ± 4% +8.92% (p=0.000 n=35+35) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 4>/set_size:8/density:1 11.7ns ± 4% 12.4ns ± 3% +5.47% (p=0.000 n=34+35) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 64>/set_size:1/density:1 13.4ns ± 3% 13.4ns ± 4% ~ (p=0.933 n=34+35) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 64>/set_size:2/density:1 12.2ns ± 3% 12.2ns ± 3% ~ (p=0.471 n=34+33) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 64>/set_size:4/density:1 12.2ns ± 2% 12.2ns ± 3% ~ (p=0.944 n=31+35) BM_SWISSMAP_InsertManyOrdered_Hot<::absl::flat_hash_set, 64>/set_size:8/density:1 11.6ns ± 2% 11.6ns ± 4% ~ (p=0.335 n=31+35) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 4>/set_size:1/density:1 18.8ns ± 3% 18.8ns ± 4% ~ (p=0.577 n=34+33) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 4>/set_size:2/density:1 14.6ns ± 3% 14.5ns ± 4% ~ (p=0.106 n=35+34) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 4>/set_size:4/density:1 14.5ns ± 3% 14.5ns ± 3% ~ (p=0.427 n=31+34) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 4>/set_size:8/density:1 12.8ns ± 3% 12.8ns ± 9% ~ (p=0.869 n=30+31) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 64>/set_size:1/density:1 18.8ns ± 2% 18.7ns ± 4% -0.76% (p=0.015 n=29+35) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 64>/set_size:2/density:1 14.5ns ± 3% 14.4ns ± 4% ~ (p=0.314 n=33+31) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 64>/set_size:4/density:1 14.5ns ± 3% 14.4ns ± 3% ~ (p=0.141 n=34+31) BM_SWISSMAP_InsertManyUnordered_Hot<::absl::flat_hash_set, 64>/set_size:8/density:1 12.7ns ± 2% 12.7ns ± 3% ~ (p=0.099 n=34+34) ``` PiperOrigin-RevId: 702758293 Change-Id: I0ec1cec37ddeafff70a43707385467c087fb424c
abseil · Dec 4, 2024 · 7316f56 · 7316f56
1 parent 9e2c537
commit 7316f56
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 20 deletions.
diff --git a/absl/container/internal/raw_hash_set.cc b/absl/container/internal/raw_hash_set.cc
@@ -28,6 +28,7 @@
 #include "absl/container/internal/container_memory.h"
 #include "absl/container/internal/hashtablez_sampler.h"
 #include "absl/hash/hash.h"
+#include "absl/numeric/bits.h"
 
 namespace absl {
 ABSL_NAMESPACE_BEGIN
@@ -96,6 +97,16 @@ bool ShouldRehashForBugDetection(const ctrl_t* ctrl, size_t capacity) {
          RehashProbabilityConstant();
 }
 
+// Find a non-deterministic hash for single group table.
+// Last two bits are used to find a position for a newly inserted element after
+// resize.
+// This function is mixing all bits of hash and control pointer to maximize
+// entropy.
+size_t SingleGroupTableH1(size_t hash, ctrl_t* control) {
+  return static_cast<size_t>(absl::popcount(
+      hash ^ static_cast<size_t>(reinterpret_cast<uintptr_t>(control))));
+}
+
 }  // namespace
 
 GenerationType* EmptyGeneration() {
@@ -135,7 +146,7 @@ size_t PrepareInsertAfterSoo(size_t hash, size_t slot_size,
   // index 1 occupied, so we need to insert either at index 0 or index 2.
   assert(HashSetResizeHelper::SooSlotIndex() == 1);
   PrepareInsertCommon(common);
-  const size_t offset = H1(hash, common.control()) & 2;
+  const size_t offset = SingleGroupTableH1(hash, common.control()) & 2;
   common.growth_info().OverwriteEmptyAsFull();
   SetCtrlInSingleGroupTable(common, offset, H2(hash), slot_size);
   common.infoz().RecordInsert(hash, /*distance_from_desired=*/0);
@@ -583,6 +594,23 @@ const void* GetHashRefForEmptyHasher(const CommonFields& common) {
   return &common;
 }
 
+FindInfo HashSetResizeHelper::FindFirstNonFullAfterResize(const CommonFields& c,
+                                                          size_t old_capacity,
+                                                          size_t hash) {
+  size_t new_capacity = c.capacity();
+  if (!IsGrowingIntoSingleGroupApplicable(old_capacity, new_capacity)) {
+    return find_first_non_full(c, hash);
+  }
+
+  // We put the new element either at the beginning or at the end of the table
+  // with approximately equal probability.
+  size_t offset =
+      SingleGroupTableH1(hash, c.control()) & 1 ? 0 : new_capacity - 1;
+
+  assert(IsEmpty(c.control()[offset]));
+  return FindInfo{offset, 0};
+}
+
 size_t PrepareInsertNonSoo(CommonFields& common, size_t hash, FindInfo target,
                            const PolicyFunctions& policy) {
   // When there are no deleted slots in the table

diff --git a/absl/container/internal/raw_hash_set.h b/absl/container/internal/raw_hash_set.h
@@ -2007,25 +2007,7 @@ class HashSetResizeHelper {
   // `GrowSizeIntoSingleGroup*` in case `IsGrowingIntoSingleGroupApplicable`.
   // Falls back to `find_first_non_full` in case of big groups.
   static FindInfo FindFirstNonFullAfterResize(const CommonFields& c,
-                                              size_t old_capacity,
-                                              size_t hash) {
-    if (!IsGrowingIntoSingleGroupApplicable(old_capacity, c.capacity())) {
-      return find_first_non_full(c, hash);
-    }
-    // Find a location for the new element non-deterministically.
-    // Note that any position is correct.
-    // It will be located at `0` or one of the other
-    // empty slots with approximately 50% probability each.
-    size_t offset = probe(c, hash).offset();
-
-    // Note that we intentionally use unsigned int underflow.
-    if (offset - (old_capacity + 1) >= old_capacity) {
-      // Offset fall on kSentinel or into the mostly occupied first half.
-      offset = 0;
-    }
-    ABSL_SWISSTABLE_ASSERT(IsEmpty(c.control()[offset]));
-    return FindInfo{offset, 0};
-  }
+                                              size_t old_capacity, size_t hash);
 
   HeapOrSoo& old_heap_or_soo() { return old_heap_or_soo_; }
   void* old_soo_data() { return old_heap_or_soo_.get_soo_data(); }

diff --git a/absl/container/internal/raw_hash_set_test.cc b/absl/container/internal/raw_hash_set_test.cc
@@ -30,6 +30,7 @@
 #include <numeric>
 #include <ostream>
 #include <random>
+#include <set>
 #include <string>
 #include <tuple>
 #include <type_traits>
@@ -2472,6 +2473,50 @@ TYPED_TEST(SooTest, IterationOrderChangesByInstance) {
   }
 }
 
+TYPED_TEST(SooTest, RelativeIterationOrderChangesByInstance) {
+  for (size_t size : {size_t{3}, size_t{4}, size_t{5}}) {
+    std::set<std::pair<int64_t, int64_t>> relative_orders;
+    auto str = [&] {
+      std::string out;
+      for (auto p : relative_orders) {
+        absl::StrAppend(&out, "{", p.first, ",", p.second, "}", "|");
+      }
+      return out;
+    };
+    const size_t expected_num_orders = size * (size - 1);
+
+    std::vector<std::vector<TypeParam>> tables;
+    for (int i = 0; relative_orders.size() < expected_num_orders; ++i) {
+      static constexpr int kBatchSize = 100;
+      ASSERT_LE(i * kBatchSize, 500)
+          << "Relative iteration order remained the same across "
+             "many attempts with size "
+          << size << ". Found " << relative_orders.size() << " out of expected "
+          << expected_num_orders << " orders found " << str();
+
+      std::vector<TypeParam> batch(kBatchSize);
+      // Insert into the tables one by one in order to avoid reusing the same
+      // memory that was freed by the previous table resize.
+      for (size_t value = 0; value < size; ++value) {
+        for (auto& table : batch) {
+          table.insert(value);
+        }
+      }
+
+      for (const auto& table : batch) {
+        auto order = OrderOfIteration(table);
+        for (auto it = order.begin(); it != order.end(); ++it) {
+          for (auto it2 = std::next(it); it2 != order.end(); ++it2) {
+            relative_orders.emplace(*it, *it2);
+          }
+        }
+      }
+
+      tables.push_back(std::move(batch));
+    }
+  }
+}
+
 TYPED_TEST(SooTest, IterationOrderChangesOnRehash) {
   // We test different sizes with many small numbers, because small table
   // resize has a different codepath.