From 4b36519c40db7619a335cf3a01ee78730ff2ccdb Mon Sep 17 00:00:00 2001 From: ncteisen Date: Fri, 5 Oct 2018 12:33:03 -0700 Subject: [PATCH] reviewer feedback --- src/core/lib/channel/channelz_registry.cc | 10 ++--- src/core/lib/channel/channelz_registry.h | 6 +-- test/core/channel/channelz_registry_test.cc | 48 ++++++++++----------- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/src/core/lib/channel/channelz_registry.cc b/src/core/lib/channel/channelz_registry.cc index 3cc8bd8ea78..4deb8c29aa6 100644 --- a/src/core/lib/channel/channelz_registry.cc +++ b/src/core/lib/channel/channelz_registry.cc @@ -59,7 +59,7 @@ intptr_t ChannelzRegistry::InternalRegister(BaseNode* node) { return ++uuid_generator_; } -void ChannelzRegistry::MaybePerformCompaction() { +void ChannelzRegistry::MaybePerformCompactionLocked() { constexpr double kEmptinessTheshold = 1 / 3; double emptiness_ratio = double(num_empty_slots_) / double(entities_.capacity()); @@ -77,7 +77,7 @@ void ChannelzRegistry::MaybePerformCompaction() { } } -int ChannelzRegistry::FindByUuid(intptr_t target_uuid) { +int ChannelzRegistry::FindByUuidLocked(intptr_t target_uuid) { size_t left = 0; size_t right = entities_.size() - 1; while (left <= right) { @@ -107,11 +107,11 @@ void ChannelzRegistry::InternalUnregister(intptr_t uuid) { GPR_ASSERT(uuid >= 1); MutexLock lock(&mu_); GPR_ASSERT(uuid <= uuid_generator_); - int idx = FindByUuid(uuid); + int idx = FindByUuidLocked(uuid); GPR_ASSERT(idx >= 0); entities_[idx] = nullptr; num_empty_slots_++; - MaybePerformCompaction(); + MaybePerformCompactionLocked(); } BaseNode* ChannelzRegistry::InternalGet(intptr_t uuid) { @@ -119,7 +119,7 @@ BaseNode* ChannelzRegistry::InternalGet(intptr_t uuid) { if (uuid < 1 || uuid > uuid_generator_) { return nullptr; } - int idx = FindByUuid(uuid); + int idx = FindByUuidLocked(uuid); return idx < 0 ? nullptr : entities_[idx]; } diff --git a/src/core/lib/channel/channelz_registry.h b/src/core/lib/channel/channelz_registry.h index f9dacd95a33..9c43d960d38 100644 --- a/src/core/lib/channel/channelz_registry.h +++ b/src/core/lib/channel/channelz_registry.h @@ -87,12 +87,12 @@ class ChannelzRegistry { char* InternalGetTopChannels(intptr_t start_channel_id); char* InternalGetServers(intptr_t start_server_id); - // If entities_ has a over a certain threshold of empty slots, it will + // If entities_ has over a certain threshold of empty slots, it will // compact the vector and move all used slots to the front. - void MaybePerformCompaction(); + void MaybePerformCompactionLocked(); // Performs binary search on entities_ to find the index with that uuid. - int FindByUuid(intptr_t uuid); + int FindByUuidLocked(intptr_t uuid); // protects members gpr_mu mu_; diff --git a/test/core/channel/channelz_registry_test.cc b/test/core/channel/channelz_registry_test.cc index 7a181d018dd..fdfc8eec94a 100644 --- a/test/core/channel/channelz_registry_test.cc +++ b/test/core/channel/channelz_registry_test.cc @@ -62,8 +62,8 @@ class ChannelzRegistryTest : public ::testing::Test { }; TEST_F(ChannelzRegistryTest, UuidStartsAboveZeroTest) { - UniquePtr channelz_channel( - new BaseNode(BaseNode::EntityType::kTopLevelChannel)); + UniquePtr channelz_channel = + MakeUnique(BaseNode::EntityType::kTopLevelChannel); intptr_t uuid = channelz_channel->uuid(); EXPECT_GT(uuid, 0) << "First uuid chose must be greater than zero. Zero if " "reserved according to " @@ -75,8 +75,8 @@ TEST_F(ChannelzRegistryTest, UuidsAreIncreasing) { std::vector> channelz_channels; channelz_channels.reserve(10); for (int i = 0; i < 10; ++i) { - channelz_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); + channelz_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); } for (size_t i = 1; i < channelz_channels.size(); ++i) { EXPECT_LT(channelz_channels[i - 1]->uuid(), channelz_channels[i]->uuid()) @@ -85,8 +85,8 @@ TEST_F(ChannelzRegistryTest, UuidsAreIncreasing) { } TEST_F(ChannelzRegistryTest, RegisterGetTest) { - UniquePtr channelz_channel( - New(BaseNode::EntityType::kTopLevelChannel)); + UniquePtr channelz_channel = + MakeUnique(BaseNode::EntityType::kTopLevelChannel); BaseNode* retrieved = ChannelzRegistry::Get(channelz_channel->uuid()); EXPECT_EQ(channelz_channel.get(), retrieved); } @@ -94,16 +94,16 @@ TEST_F(ChannelzRegistryTest, RegisterGetTest) { TEST_F(ChannelzRegistryTest, RegisterManyItems) { std::vector> channelz_channels; for (int i = 0; i < 100; i++) { - channelz_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); + channelz_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); BaseNode* retrieved = ChannelzRegistry::Get(channelz_channels[i]->uuid()); EXPECT_EQ(channelz_channels[i].get(), retrieved); } } TEST_F(ChannelzRegistryTest, NullIfNotPresentTest) { - UniquePtr channelz_channel( - New(BaseNode::EntityType::kTopLevelChannel)); + UniquePtr channelz_channel = + MakeUnique(BaseNode::EntityType::kTopLevelChannel); // try to pull out a uuid that does not exist. BaseNode* nonexistant = ChannelzRegistry::Get(channelz_channel->uuid() + 1); EXPECT_EQ(nonexistant, nullptr); @@ -121,10 +121,10 @@ TEST_F(ChannelzRegistryTest, TestCompaction) { std::vector> odd_channels; odd_channels.reserve(kLoopIterations); for (int i = 0; i < kLoopIterations; i++) { - even_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); - odd_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); + even_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); + odd_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); } } // without compaction, there would be exactly kLoopIterations empty slots at @@ -146,10 +146,10 @@ TEST_F(ChannelzRegistryTest, TestGetAfterCompaction) { std::vector> odd_channels; odd_channels.reserve(kLoopIterations); for (int i = 0; i < kLoopIterations; i++) { - even_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); - odd_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); + even_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); + odd_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); odd_uuids.push_back(odd_channels[i]->uuid()); } } @@ -173,18 +173,18 @@ TEST_F(ChannelzRegistryTest, TestAddAfterCompaction) { std::vector> odd_channels; odd_channels.reserve(kLoopIterations); for (int i = 0; i < kLoopIterations; i++) { - even_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); - odd_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); + even_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); + odd_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); odd_uuids.push_back(odd_channels[i]->uuid()); } } std::vector> more_channels; more_channels.reserve(kLoopIterations); for (int i = 0; i < kLoopIterations; i++) { - more_channels.push_back(UniquePtr( - New(BaseNode::EntityType::kTopLevelChannel))); + more_channels.push_back( + MakeUnique(BaseNode::EntityType::kTopLevelChannel)); BaseNode* retrieved = ChannelzRegistry::Get(more_channels[i]->uuid()); EXPECT_EQ(more_channels[i].get(), retrieved); }