Improve map tests:

 - Make sure the hash flooding test actually does hash flooding. The technique used to choose the inputs stopped working when random seeding was added. Now we assert trees are used in the test so it won't silently break again.
 - Improve test coverage for tree buckets.
 - Update the hash flood test to use absl::Time/Duration.
 - Improve the map tests to use long strings. This way we can test that the values are being properly destroyed. Small strings could be leaked without side effects because of SSO.

PiperOrigin-RevId: 530917835

Author: protobuf-github-bot

src/google/protobuf/BUILD.bazel

@@ -849,6 +849,7 @@ cc_library(
         "//src/google/protobuf/testing",
         "@com_google_absl//absl/container:flat_hash_map",
         "@com_google_absl//absl/log:absl_check",
+        "@com_google_absl//absl/time",
         "@com_google_googletest//:gtest",
     ],
 )
@@ -980,9 +981,9 @@ cc_library(
     visibility = ["//pkg:__pkg__"],
     deps = [
         ":protobuf",
-        "@com_google_googletest//:gtest",
         "@com_google_absl//absl/log:absl_check",
         "@com_google_absl//absl/memory",
+        "@com_google_googletest//:gtest",
     ],
 )
 

src/google/protobuf/map_test.inc

@@ -56,6 +56,7 @@
 #include "absl/log/absl_check.h"
 #include "absl/log/absl_log.h"
 #include "absl/strings/substitute.h"
+#include "absl/time/time.h"
 #include "google/protobuf/arena_test_util.h"
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/descriptor_database.h"
@@ -73,7 +74,6 @@
 #include "google/protobuf/test_util2.h"
 #include "google/protobuf/text_format.h"
 #include "google/protobuf/util/message_differencer.h"
-#include "google/protobuf/util/time_util.h"
 #include "google/protobuf/wire_format.h"
 
 
@@ -204,6 +204,24 @@ struct MapTestPeer {
     }
     return true;
   }
+
+  template <typename T>
+  static size_t BucketNumber(T& map, typename T::key_type key) {
+    return map.BucketNumber(key);
+  }
+
+  template <typename T>
+  static void Resize(T& map, size_t num_buckets) {
+    map.Resize(num_buckets);
+  }
+
+  template <typename T>
+  static bool HasTreeBuckets(T& map) {
+    for (size_t i = 0; i < map.num_buckets_; ++i) {
+      if (TableEntryIsTree(map.table_[i])) return true;
+    }
+    return false;
+  }
 };
 
 namespace {
@@ -463,57 +481,87 @@ TEST_F(MapImplTest, IteratorBasic) {
   EXPECT_TRUE(it == cit);
 }
 
-template <typename Iterator>
-static int64_t median(Iterator i0, Iterator i1) {
-  std::vector<int64_t> v(i0, i1);
+static absl::Duration median(absl::Span<absl::Duration> v) {
   std::nth_element(v.begin(), v.begin() + v.size() / 2, v.end());
   return v[v.size() / 2];
 }
 
-static int64_t Now() {
-  return util::TimeUtil::TimestampToNanoseconds(
-      util::TimeUtil::GetCurrentTime());
-}
-
 // Arbitrary odd integers for creating test data.
 static int k0 = 812398771;
 static int k1 = 1312938717;
 static int k2 = 1321555333;
 
-// Try to create kTestSize keys that will land in just a few buckets, and
-// time the insertions, to get a rough estimate of whether an O(n^2) worst case
-// was triggered.  This test is a hacky, but probably better than nothing.
-TEST_F(MapImplTest, HashFlood) {
-  const int kTestSize = 1024;  // must be a power of 2
-  absl::btree_set<int> s;
-  for (int i = 0; s.size() < kTestSize; i++) {
-    if ((map_.hash_function()(i) & (kTestSize - 1)) < 3) {
-      s.insert(i);
+// Finds inputs that will fall in the first few buckets for this particular map
+// (with the random seed it has) and this particular size.
+static std::vector<int> FindBadInputs(Map<int, int>& map, int num_inputs) {
+  // Make sure the seed and the size is set so that BucketNumber works.
+  while (map.size() < num_inputs) map[map.size()];
+  map.clear();
+
+  std::vector<int> out;
+
+  for (int i = 0; out.size() < num_inputs; ++i) {
+    if (MapTestPeer::BucketNumber(map, i) < 3) {
+      out.push_back(i);
     }
   }
-  // Create hash table with kTestSize entries that hash flood a table with
-  // 1024 (or 512 or 2048 or ...) entries.  This assumes that map_ uses powers
-  // of 2 for table sizes, and that it's sufficient to "flood" with respect to
-  // the low bits of the output of map_.hash_function().
-  std::vector<int64_t> times;
-  auto it = s.begin();
+
+  // Reset the table to get it to grow from scratch again.
+  // The capacity will be lost, but we will get it again on insertion.
+  // It will also keep the seed.
+  map.clear();
+  MapTestPeer::Resize(map, 8);
+
+  return out;
+}
+
+TEST_F(MapImplTest, TreePathWorksAsExpected) {
+  const std::vector<int> s = FindBadInputs(map_, 1000);
+
+  for (int i : s) {
+    map_[i] = 0;
+  }
+  // Make sure we are testing what we think we are testing.
+  ASSERT_TRUE(MapTestPeer::HasTreeBuckets(map_));
+  for (int i : s) {
+    ASSERT_NE(map_.find(i), map_.end()) << i;
+  }
+  for (int i : s) {
+    ASSERT_EQ(1, map_.erase(i)) << i;
+  }
+  EXPECT_FALSE(MapTestPeer::HasTreeBuckets(map_));
+  EXPECT_TRUE(map_.empty());
+}
+
+// Create kTestSize keys that will land in just a few buckets, and time the
+// insertions, to get a rough estimate of whether an O(n^2) worst case was
+// triggered.  This test is a hacky, but probably better than nothing.
+TEST_F(MapImplTest, HashFlood) {
+  const std::vector<int> s = FindBadInputs(map_, 1000);
+
+  // Create hash table with 1000 entries that hash flood a table. The entries
+  // were chosen so that they all fall in a few buckets.
+  std::vector<absl::Duration> times;
   int count = 0;
-  do {
-    const int64_t start = Now();
-    map_[*it] = 0;
-    const int64_t end = Now();
+  for (int i : s) {
+    const auto start = absl::Now();
+    map_[i] = 0;
+    const auto end = absl::Now();
     if (end > start) {
       times.push_back(end - start);
     }
     ++count;
-    ++it;
-  } while (it != s.end());
+  }
   if (times.size() < .99 * count) return;
-  int64_t x0 = median(times.begin(), times.begin() + 9);
-  int64_t x1 = median(times.begin() + times.size() - 9, times.end());
-  // x1 will greatly exceed x0 if the code we just executed took O(n^2) time.
-  // But we want to allow O(n log n).  A factor of 20 should be generous enough.
-  EXPECT_LE(x1, x0 * 20);
+  const auto x0 = median(absl::MakeSpan(times).subspan(0, 9));
+  const auto x1 = median(absl::MakeSpan(times).subspan(times.size() - 9));
+  ABSL_LOG(INFO) << "number of samples=" << times.size() << " x0=" << x0
+                 << " x1=" << x1;
+  // x1 will greatly exceed x0 if inserting the elements was O(n) time.
+  // We know it will not be O(1) because of all the collisions, but we want to
+  // allow O(log n).
+  // lg2(1000) is ~10, times a constant for the cost of the tree.
+  EXPECT_LE(x1, x0 * 50);
 }
 
 TEST_F(MapImplTest, CopyIteratorStressTest) {

src/google/protobuf/map_test_util_impl.h

@@ -31,6 +31,8 @@
 #ifndef GOOGLE_PROTOBUF_MAP_TEST_UTIL_IMPL_H__
 #define GOOGLE_PROTOBUF_MAP_TEST_UTIL_IMPL_H__
 
+#include <string>
+
 #include <gtest/gtest.h>
 
 
@@ -100,6 +102,13 @@ class MapTestUtilImpl {
   // // Get pointers of map entries from release.
   // static std::vector<const Message*> GetMapEntriesFromRelease(
   //     MapMessage* message);
+
+  static std::string long_string() {
+    return "This is a very long string that goes in the heap";
+  }
+  static std::string long_string_2() {
+    return "This is another very long string that goes in the heap";
+  }
 };
 
 template <typename EnumType, EnumType enum_value0, EnumType enum_value1,
@@ -119,8 +128,8 @@ void MapTestUtilImpl::SetMapFields(MapMessage* message) {
   (*message->mutable_map_int32_float())[0] = 0.0;
   (*message->mutable_map_int32_double())[0] = 0.0;
   (*message->mutable_map_bool_bool())[0] = false;
-  (*message->mutable_map_string_string())["0"] = "0";
-  (*message->mutable_map_int32_bytes())[0] = "0";
+  (*message->mutable_map_string_string())[long_string()] = long_string();
+  (*message->mutable_map_int32_bytes())[0] = long_string();
   (*message->mutable_map_int32_enum())[0] = enum_value0;
   (*message->mutable_map_int32_foreign_message())[0].set_c(0);
 
@@ -138,8 +147,8 @@ void MapTestUtilImpl::SetMapFields(MapMessage* message) {
   (*message->mutable_map_int32_float())[1] = 1.0;
   (*message->mutable_map_int32_double())[1] = 1.0;
   (*message->mutable_map_bool_bool())[1] = true;
-  (*message->mutable_map_string_string())["1"] = "1";
-  (*message->mutable_map_int32_bytes())[1] = "1";
+  (*message->mutable_map_string_string())[long_string_2()] = long_string_2();
+  (*message->mutable_map_int32_bytes())[1] = long_string_2();
   (*message->mutable_map_int32_enum())[1] = enum_value1;
   (*message->mutable_map_int32_foreign_message())[1].set_c(1);
 }
@@ -161,8 +170,8 @@ void MapTestUtilImpl::SetArenaMapFields(MapMessage* message) {
   (*message->mutable_map_int32_float())[0] = 0.0;
   (*message->mutable_map_int32_double())[0] = 0.0;
   (*message->mutable_map_bool_bool())[0] = false;
-  (*message->mutable_map_string_string())["0"] = "0";
-  (*message->mutable_map_int32_bytes())[0] = "0";
+  (*message->mutable_map_string_string())[long_string()] = long_string();
+  (*message->mutable_map_int32_bytes())[0] = long_string();
   (*message->mutable_map_int32_enum())[0] = enum_value0;
   (*message->mutable_map_int32_foreign_message())[0].set_c(0);
 
@@ -180,8 +189,8 @@ void MapTestUtilImpl::SetArenaMapFields(MapMessage* message) {
   (*message->mutable_map_int32_float())[1] = 1.0;
   (*message->mutable_map_int32_double())[1] = 1.0;
   (*message->mutable_map_bool_bool())[1] = true;
-  (*message->mutable_map_string_string())["1"] = "1";
-  (*message->mutable_map_int32_bytes())[1] = "1";
+  (*message->mutable_map_string_string())[long_string_2()] = long_string_2();
+  (*message->mutable_map_int32_bytes())[1] = long_string_2();
   (*message->mutable_map_int32_enum())[1] = enum_value1;
   (*message->mutable_map_int32_foreign_message())[1].set_c(1);
 }
@@ -203,7 +212,7 @@ void MapTestUtilImpl::SetMapFieldsInitialized(MapMessage* message) {
   (*message->mutable_map_int32_float())[0];
   (*message->mutable_map_int32_double())[0];
   (*message->mutable_map_bool_bool())[0];
-  (*message->mutable_map_string_string())["0"];
+  (*message->mutable_map_string_string())[long_string()];
   (*message->mutable_map_int32_bytes())[0];
   (*message->mutable_map_int32_enum())[0];
   (*message->mutable_map_int32_foreign_message())[0];
@@ -224,7 +233,7 @@ void MapTestUtilImpl::ModifyMapFields(MapMessage* message) {
   (*message->mutable_map_int32_float())[1] = 2.0;
   (*message->mutable_map_int32_double())[1] = 2.0;
   (*message->mutable_map_bool_bool())[1] = false;
-  (*message->mutable_map_string_string())["1"] = "2";
+  (*message->mutable_map_string_string())[long_string_2()] = "2";
   (*message->mutable_map_int32_bytes())[1] = "2";
   (*message->mutable_map_int32_enum())[1] = enum_value;
   (*message->mutable_map_int32_foreign_message())[1].set_c(2);
@@ -285,8 +294,8 @@ void MapTestUtilImpl::ExpectMapFieldsSet(const MapMessage& message) {
   EXPECT_EQ(0, message.map_int32_float().at(0));
   EXPECT_EQ(0, message.map_int32_double().at(0));
   EXPECT_EQ(false, message.map_bool_bool().at(0));
-  EXPECT_EQ("0", message.map_string_string().at("0"));
-  EXPECT_EQ("0", message.map_int32_bytes().at(0));
+  EXPECT_EQ(long_string(), message.map_string_string().at(long_string()));
+  EXPECT_EQ(long_string(), message.map_int32_bytes().at(0));
   EXPECT_EQ(enum_value0, message.map_int32_enum().at(0));
   EXPECT_EQ(0, message.map_int32_foreign_message().at(0).c());
 
@@ -303,8 +312,8 @@ void MapTestUtilImpl::ExpectMapFieldsSet(const MapMessage& message) {
   EXPECT_EQ(1, message.map_int32_float().at(1));
   EXPECT_EQ(1, message.map_int32_double().at(1));
   EXPECT_EQ(true, message.map_bool_bool().at(1));
-  EXPECT_EQ("1", message.map_string_string().at("1"));
-  EXPECT_EQ("1", message.map_int32_bytes().at(1));
+  EXPECT_EQ(long_string_2(), message.map_string_string().at(long_string_2()));
+  EXPECT_EQ(long_string_2(), message.map_int32_bytes().at(1));
   EXPECT_EQ(enum_value1, message.map_int32_enum().at(1));
   EXPECT_EQ(1, message.map_int32_foreign_message().at(1).c());
 }
@@ -343,8 +352,8 @@ void MapTestUtilImpl::ExpectArenaMapFieldsSet(const MapMessage& message) {
   EXPECT_EQ(0, message.map_int32_float().at(0));
   EXPECT_EQ(0, message.map_int32_double().at(0));
   EXPECT_EQ(false, message.map_bool_bool().at(0));
-  EXPECT_EQ("0", message.map_string_string().at("0"));
-  EXPECT_EQ("0", message.map_int32_bytes().at(0));
+  EXPECT_EQ(long_string(), message.map_string_string().at(long_string()));
+  EXPECT_EQ(long_string(), message.map_int32_bytes().at(0));
   EXPECT_EQ(enum_value0, message.map_int32_enum().at(0));
   EXPECT_EQ(0, message.map_int32_foreign_message().at(0).c());
 
@@ -361,8 +370,8 @@ void MapTestUtilImpl::ExpectArenaMapFieldsSet(const MapMessage& message) {
   EXPECT_EQ(1, message.map_int32_float().at(1));
   EXPECT_EQ(1, message.map_int32_double().at(1));
   EXPECT_EQ(true, message.map_bool_bool().at(1));
-  EXPECT_EQ("1", message.map_string_string().at("1"));
-  EXPECT_EQ("1", message.map_int32_bytes().at(1));
+  EXPECT_EQ(long_string_2(), message.map_string_string().at(long_string_2()));
+  EXPECT_EQ(long_string_2(), message.map_int32_bytes().at(1));
   EXPECT_EQ(enum_value1, message.map_int32_enum().at(1));
   EXPECT_EQ(1, message.map_int32_foreign_message().at(1).c());
 }
@@ -400,7 +409,7 @@ void MapTestUtilImpl::ExpectMapFieldsSetInitialized(const MapMessage& message) {
   EXPECT_EQ(0, message.map_int32_float().at(0));
   EXPECT_EQ(0, message.map_int32_double().at(0));
   EXPECT_EQ(false, message.map_bool_bool().at(0));
-  EXPECT_EQ("", message.map_string_string().at("0"));
+  EXPECT_EQ("", message.map_string_string().at(long_string()));
   EXPECT_EQ("", message.map_int32_bytes().at(0));
   EXPECT_EQ(enum_value, message.map_int32_enum().at(0));
   EXPECT_EQ(0, message.map_int32_foreign_message().at(0).ByteSizeLong());
@@ -443,8 +452,8 @@ void MapTestUtilImpl::ExpectMapFieldsModified(const MapMessage& message) {
   EXPECT_EQ(0, message.map_int32_float().at(0));
   EXPECT_EQ(0, message.map_int32_double().at(0));
   EXPECT_EQ(false, message.map_bool_bool().at(0));
-  EXPECT_EQ("0", message.map_string_string().at("0"));
-  EXPECT_EQ("0", message.map_int32_bytes().at(0));
+  EXPECT_EQ(long_string(), message.map_string_string().at(long_string()));
+  EXPECT_EQ(long_string(), message.map_int32_bytes().at(0));
   EXPECT_EQ(enum_value0, message.map_int32_enum().at(0));
   EXPECT_EQ(0, message.map_int32_foreign_message().at(0).c());
 
@@ -462,7 +471,7 @@ void MapTestUtilImpl::ExpectMapFieldsModified(const MapMessage& message) {
   EXPECT_EQ(2, message.map_int32_float().at(1));
   EXPECT_EQ(2, message.map_int32_double().at(1));
   EXPECT_EQ(false, message.map_bool_bool().at(1));
-  EXPECT_EQ("2", message.map_string_string().at("1"));
+  EXPECT_EQ("2", message.map_string_string().at(long_string_2()));
   EXPECT_EQ("2", message.map_int32_bytes().at(1));
   EXPECT_EQ(enum_value1, message.map_int32_enum().at(1));
   EXPECT_EQ(2, message.map_int32_foreign_message().at(1).c());