diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel index 9af95d63e4..b86a24c2ef 100644 --- a/src/google/protobuf/BUILD.bazel +++ b/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", ], ) diff --git a/src/google/protobuf/map_test.inc b/src/google/protobuf/map_test.inc index 209802472a..e8f171ab42 100644 --- a/src/google/protobuf/map_test.inc +++ b/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 + static size_t BucketNumber(T& map, typename T::key_type key) { + return map.BucketNumber(key); + } + + template + static void Resize(T& map, size_t num_buckets) { + map.Resize(num_buckets); + } + + template + 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 -static int64_t median(Iterator i0, Iterator i1) { - std::vector v(i0, i1); +static absl::Duration median(absl::Span 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 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 FindBadInputs(Map& 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 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 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 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 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 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) { diff --git a/src/google/protobuf/map_test_util_impl.h b/src/google/protobuf/map_test_util_impl.h index 9f12f53a47..f5283ebcc2 100644 --- a/src/google/protobuf/map_test_util_impl.h +++ b/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 + #include @@ -100,6 +102,13 @@ class MapTestUtilImpl { // // Get pointers of map entries from release. // static std::vector 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 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()); diff --git a/src/google/protobuf/reflection_tester.cc b/src/google/protobuf/reflection_tester.cc index a26addfa86..6c1cd3d849 100644 --- a/src/google/protobuf/reflection_tester.cc +++ b/src/google/protobuf/reflection_tester.cc @@ -206,6 +206,10 @@ MapReflectionTester::MapReflectionTester(const Descriptor* base_descriptor) EXPECT_EQ(fdesc->containing_type()->map_value(), fdesc); } } + + // Must be heap allocated. + EXPECT_NE(long_string().capacity(), std::string().capacity()); + EXPECT_NE(long_string_2().capacity(), std::string().capacity()); } // Shorthand to get a FieldDescriptor for a field of unittest::TestMap. @@ -294,14 +298,14 @@ void MapReflectionTester::SetMapFieldsViaReflection(Message* message) { sub_message = reflection->AddMessage(message, F("map_string_string")); sub_message->GetReflection()->SetString(sub_message, map_string_string_key_, - "0"); + long_string()); sub_message->GetReflection()->SetString(sub_message, map_string_string_val_, - "0"); + long_string()); sub_message = reflection->AddMessage(message, F("map_int32_bytes")); sub_message->GetReflection()->SetInt32(sub_message, map_int32_bytes_key_, 0); sub_message->GetReflection()->SetString(sub_message, map_int32_bytes_val_, - "0"); + long_string()); sub_message = reflection->AddMessage(message, F("map_int32_enum")); sub_message->GetReflection()->SetInt32(sub_message, map_int32_enum_key_, 0); @@ -389,14 +393,14 @@ void MapReflectionTester::SetMapFieldsViaReflection(Message* message) { sub_message = reflection->AddMessage(message, F("map_string_string")); sub_message->GetReflection()->SetString(sub_message, map_string_string_key_, - "1"); + long_string_2()); sub_message->GetReflection()->SetString(sub_message, map_string_string_val_, - "1"); + long_string_2()); sub_message = reflection->AddMessage(message, F("map_int32_bytes")); sub_message->GetReflection()->SetInt32(sub_message, map_int32_bytes_key_, 1); sub_message->GetReflection()->SetString(sub_message, map_int32_bytes_val_, - "1"); + long_string_2()); sub_message = reflection->AddMessage(message, F("map_int32_enum")); sub_message->GetReflection()->SetInt32(sub_message, map_int32_enum_key_, 1); @@ -494,19 +498,19 @@ void MapReflectionTester::SetMapFieldsViaMapReflection(Message* message) { map_key, &map_val)); map_val.SetBoolValue(false); - map_key.SetStringValue("0"); + map_key.SetStringValue(long_string()); EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_string_string"), map_key, &map_val_const)); EXPECT_TRUE(reflection->InsertOrLookupMapValue( message, F("map_string_string"), map_key, &map_val)); - map_val.SetStringValue("0"); + map_val.SetStringValue(long_string()); map_key.SetInt32Value(0); EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_int32_bytes"), map_key, &map_val_const)); EXPECT_TRUE(reflection->InsertOrLookupMapValue(message, F("map_int32_bytes"), map_key, &map_val)); - map_val.SetStringValue("0"); + map_val.SetStringValue(long_string()); map_key.SetInt32Value(0); EXPECT_FALSE(reflection->LookupMapValue(*message, F("map_int32_enum"), @@ -594,15 +598,15 @@ void MapReflectionTester::SetMapFieldsViaMapReflection(Message* message) { &map_val); map_val.SetBoolValue(true); - map_key.SetStringValue("1"); + map_key.SetStringValue(long_string_2()); reflection->InsertOrLookupMapValue(message, F("map_string_string"), map_key, &map_val); - map_val.SetStringValue("1"); + map_val.SetStringValue(long_string_2()); map_key.SetInt32Value(1); reflection->InsertOrLookupMapValue(message, F("map_int32_bytes"), map_key, &map_val); - map_val.SetStringValue("1"); + map_val.SetStringValue(long_string_2()); map_key.SetInt32Value(1); reflection->InsertOrLookupMapValue(message, F("map_int32_enum"), map_key, @@ -743,7 +747,7 @@ void MapReflectionTester::ModifyMapFieldsViaReflection(Message* message) { &map_val); map_val.SetBoolValue(false); - map_key.SetStringValue("1"); + map_key.SetStringValue(long_string_2()); reflection->InsertOrLookupMapValue(message, F("map_string_string"), map_key, &map_val); map_val.SetStringValue("2"); @@ -1262,10 +1266,10 @@ void MapReflectionTester::ExpectMapFieldsSetViaReflection( } { absl::flat_hash_map map; - map["0"] = "0"; - map["1"] = "1"; - std::vector keys = {"0", "1"}; - std::vector vals = {"0", "1"}; + map[long_string()] = long_string(); + map[long_string_2()] = long_string_2(); + std::vector keys = {long_string(), long_string_2()}; + std::vector vals = {long_string(), long_string_2()}; for (int i = 0; i < 2; i++) { const internal::MapFieldBase& map_field = reflection->GetRaw(message, @@ -1292,8 +1296,8 @@ void MapReflectionTester::ExpectMapFieldsSetViaReflection( } { absl::flat_hash_map map; - map[0] = "0"; - map[1] = "1"; + map[0] = long_string(); + map[1] = long_string_2(); for (int i = 0; i < 2; i++) { const internal::MapFieldBase& map_field = reflection->GetRaw(message, @@ -1549,8 +1553,8 @@ void MapReflectionTester::ExpectMapFieldsSetViaReflectionIterator( } { absl::flat_hash_map map; - map["0"] = "0"; - map["1"] = "1"; + map[long_string()] = long_string(); + map[long_string_2()] = long_string_2(); int size = 0; for (MapIterator iter = reflection->MapBegin(message, F("map_string_string")); @@ -1569,8 +1573,8 @@ void MapReflectionTester::ExpectMapFieldsSetViaReflectionIterator( } { absl::flat_hash_map map; - map[0] = "0"; - map[1] = "1"; + map[0] = long_string(); + map[1] = long_string_2(); for (MapIterator iter = reflection->MapBegin(message, F("map_int32_bytes")); iter != reflection->MapEnd(message, F("map_int32_bytes")); ++iter) { EXPECT_EQ(map[iter.GetKey().GetInt32Value()], diff --git a/src/google/protobuf/reflection_tester.h b/src/google/protobuf/reflection_tester.h index 7a124affef..16a91fe7a3 100644 --- a/src/google/protobuf/reflection_tester.h +++ b/src/google/protobuf/reflection_tester.h @@ -68,6 +68,13 @@ class MapReflectionTester { MapIterator MapEnd(Message* message, const std::string& field_name); int MapSize(const Message& message, const std::string& field_name); + 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"; + } + private: const FieldDescriptor* F(const std::string& name); diff --git a/src/google/protobuf/testdata/map_test_data.txt b/src/google/protobuf/testdata/map_test_data.txt index bc2723214a..bdb58215e6 100644 --- a/src/google/protobuf/testdata/map_test_data.txt +++ b/src/google/protobuf/testdata/map_test_data.txt @@ -103,20 +103,20 @@ map_bool_bool { value: true } map_string_string { - key: "0" - value: "0" + key: "This is a very long string that goes in the heap" + value: "This is a very long string that goes in the heap" } map_string_string { - key: "1" - value: "1" + key: "This is another very long string that goes in the heap" + value: "This is another very long string that goes in the heap" } map_int32_bytes { key: 0 - value: "0" + value: "This is a very long string that goes in the heap" } map_int32_bytes { key: 1 - value: "1" + value: "This is another very long string that goes in the heap" } map_int32_enum { key: 0