From e21a418aaef4d2d6541e9d26742b0dc57995b18d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 4 Oct 2018 12:53:36 -0700 Subject: [PATCH] Add compaction to channelz registry --- src/core/lib/channel/channelz_registry.cc | 60 ++++++++++++++++++--- src/core/lib/channel/channelz_registry.h | 16 +++++- src/core/lib/gprpp/inlined_vector.h | 8 +++ test/core/channel/channelz_registry_test.cc | 59 +++++++++++--------- 4 files changed, 110 insertions(+), 33 deletions(-) diff --git a/src/core/lib/channel/channelz_registry.cc b/src/core/lib/channel/channelz_registry.cc index 841f1c6104b..406ce17fac5 100644 --- a/src/core/lib/channel/channelz_registry.cc +++ b/src/core/lib/channel/channelz_registry.cc @@ -56,23 +56,71 @@ ChannelzRegistry::~ChannelzRegistry() { gpr_mu_destroy(&mu_); } intptr_t ChannelzRegistry::InternalRegister(BaseNode* node) { MutexLock lock(&mu_); entities_.push_back(node); - intptr_t uuid = entities_.size(); - return uuid; + return ++uuid_generator_; +} + +void ChannelzRegistry::MaybePerformCompaction() { + constexpr double kEmptinessTheshold = 1 / 3; + double emptiness_ratio = + double(num_empty_slots_) / double(entities_.capacity()); + if (emptiness_ratio > kEmptinessTheshold) { + int front = 0; + for (size_t i = 0; i < entities_.size(); ++i) { + if (entities_[i] != nullptr) { + entities_[front++] = entities_[i]; + } + } + for (int i = 0; i < num_empty_slots_; ++i) { + entities_.pop_back(); + } + num_empty_slots_ = 0; + } +} + +int ChannelzRegistry::FindByUuid(intptr_t target_uuid) { + int left = 0; + int right = entities_.size() - 1; + while (left <= right) { + int true_middle = left + (right - left) / 2; + int first_non_null = true_middle; + while (first_non_null < right && entities_[first_non_null] == nullptr) { + first_non_null++; + } + if (entities_[first_non_null] == nullptr) { + right = true_middle - 1; + continue; + } + intptr_t uuid = entities_[first_non_null]->uuid(); + if (uuid == target_uuid) { + return first_non_null; + } + if (uuid < target_uuid) { + left = first_non_null + 1; + } else { + right = true_middle - 1; + } + } + return -1; } void ChannelzRegistry::InternalUnregister(intptr_t uuid) { GPR_ASSERT(uuid >= 1); MutexLock lock(&mu_); - GPR_ASSERT(static_cast(uuid) <= entities_.size()); - entities_[uuid - 1] = nullptr; + GPR_ASSERT(uuid <= uuid_generator_); + int idx = FindByUuid(uuid); + GPR_ASSERT(idx >= 0); + entities_[idx] = nullptr; + num_empty_slots_++; + MaybePerformCompaction(); } BaseNode* ChannelzRegistry::InternalGet(intptr_t uuid) { MutexLock lock(&mu_); - if (uuid < 1 || uuid > static_cast(entities_.size())) { + if (uuid < 1 || uuid > uuid_generator_) { return nullptr; } - return entities_[uuid - 1]; + int idx = FindByUuid(uuid); + return idx < 0 ? nullptr : entities_[idx]; } char* ChannelzRegistry::InternalGetTopChannels(intptr_t start_channel_id) { diff --git a/src/core/lib/channel/channelz_registry.h b/src/core/lib/channel/channelz_registry.h index d0d660600d9..f9dacd95a33 100644 --- a/src/core/lib/channel/channelz_registry.h +++ b/src/core/lib/channel/channelz_registry.h @@ -30,6 +30,10 @@ namespace grpc_core { namespace channelz { +namespace testing { +class ChannelzRegistryPeer; +} + // singleton registry object to track all objects that are needed to support // channelz bookkeeping. All objects share globally distributed uuids. class ChannelzRegistry { @@ -61,6 +65,7 @@ class ChannelzRegistry { private: GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_NEW GPRC_ALLOW_CLASS_TO_USE_NON_PUBLIC_DELETE + friend class testing::ChannelzRegistryPeer; ChannelzRegistry(); ~ChannelzRegistry(); @@ -82,9 +87,18 @@ class ChannelzRegistry { char* InternalGetTopChannels(intptr_t start_channel_id); char* InternalGetServers(intptr_t start_server_id); - // protects entities_ and uuid_ + // If entities_ has a over a certain threshold of empty slots, it will + // compact the vector and move all used slots to the front. + void MaybePerformCompaction(); + + // Performs binary search on entities_ to find the index with that uuid. + int FindByUuid(intptr_t uuid); + + // protects members gpr_mu mu_; InlinedVector entities_; + intptr_t uuid_generator_ = 0; + int num_empty_slots_ = 0; }; } // namespace channelz diff --git a/src/core/lib/gprpp/inlined_vector.h b/src/core/lib/gprpp/inlined_vector.h index 76e2f0a7850..65c2b9634f2 100644 --- a/src/core/lib/gprpp/inlined_vector.h +++ b/src/core/lib/gprpp/inlined_vector.h @@ -123,6 +123,14 @@ class InlinedVector { void push_back(T&& value) { emplace_back(std::move(value)); } + void pop_back() { + assert(!empty()); + size_t s = size(); + T& value = data()[s - 1]; + value.~T(); + size_--; + } + void copy_from(const InlinedVector& v) { // if v is allocated, copy over the buffer. if (v.dynamic_ != nullptr) { diff --git a/test/core/channel/channelz_registry_test.cc b/test/core/channel/channelz_registry_test.cc index c02d525c819..02f0968caf1 100644 --- a/test/core/channel/channelz_registry_test.cc +++ b/test/core/channel/channelz_registry_test.cc @@ -43,56 +43,63 @@ namespace grpc_core { namespace channelz { namespace testing { +class ChannelzRegistryPeer { + const InlinedVector* entities() { + return &ChannelzRegistry::Default()->entities_; + } + int num_empty_slots() { + return ChannelzRegistry::Default()->num_empty_slots_; + } +}; + TEST(ChannelzRegistryTest, UuidStartsAboveZeroTest) { - BaseNode* channelz_channel = nullptr; - intptr_t uuid = ChannelzRegistry::Register(channelz_channel); + UniquePtr channelz_channel( + new BaseNode(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 " "https://github.com/grpc/proposal/blob/master/" "A14-channelz.md"; - ChannelzRegistry::Unregister(uuid); } TEST(ChannelzRegistryTest, UuidsAreIncreasing) { - BaseNode* channelz_channel = nullptr; - std::vector uuids; - uuids.reserve(10); + std::vector> channelz_channels; + channelz_channels.reserve(10); for (int i = 0; i < 10; ++i) { - // reregister the same object. It's ok since we are just testing uuids - uuids.push_back(ChannelzRegistry::Register(channelz_channel)); + channelz_channels.push_back(UniquePtr( + new BaseNode(BaseNode::EntityType::kTopLevelChannel))); } - for (size_t i = 1; i < uuids.size(); ++i) { - EXPECT_LT(uuids[i - 1], uuids[i]) << "Uuids must always be increasing"; + for (size_t i = 1; i < channelz_channels.size(); ++i) { + EXPECT_LT(channelz_channels[i - 1]->uuid(), channelz_channels[i]->uuid()) + << "Uuids must always be increasing"; } } TEST(ChannelzRegistryTest, RegisterGetTest) { - // we hackily jam an intptr_t into this pointer to check for equality later - BaseNode* channelz_channel = (BaseNode*)42; - intptr_t uuid = ChannelzRegistry::Register(channelz_channel); - BaseNode* retrieved = ChannelzRegistry::Get(uuid); - EXPECT_EQ(channelz_channel, retrieved); + UniquePtr channelz_channel( + new BaseNode(BaseNode::EntityType::kTopLevelChannel)); + BaseNode* retrieved = ChannelzRegistry::Get(channelz_channel->uuid()); + EXPECT_EQ(channelz_channel.get(), retrieved); } TEST(ChannelzRegistryTest, RegisterManyItems) { - // we hackily jam an intptr_t into this pointer to check for equality later - BaseNode* channelz_channel = (BaseNode*)42; + std::vector> channelz_channels; for (int i = 0; i < 100; i++) { - intptr_t uuid = ChannelzRegistry::Register(channelz_channel); - BaseNode* retrieved = ChannelzRegistry::Get(uuid); - EXPECT_EQ(channelz_channel, retrieved); + channelz_channels.push_back(UniquePtr( + new BaseNode(BaseNode::EntityType::kTopLevelChannel))); + BaseNode* retrieved = ChannelzRegistry::Get(channelz_channels[i]->uuid()); + EXPECT_EQ(channelz_channels[i].get(), retrieved); } } TEST(ChannelzRegistryTest, NullIfNotPresentTest) { - // we hackily jam an intptr_t into this pointer to check for equality later - BaseNode* channelz_channel = (BaseNode*)42; - intptr_t uuid = ChannelzRegistry::Register(channelz_channel); + UniquePtr channelz_channel( + new BaseNode(BaseNode::EntityType::kTopLevelChannel)); // try to pull out a uuid that does not exist. - BaseNode* nonexistant = ChannelzRegistry::Get(uuid + 1); + BaseNode* nonexistant = ChannelzRegistry::Get(channelz_channel->uuid() + 1); EXPECT_EQ(nonexistant, nullptr); - BaseNode* retrieved = ChannelzRegistry::Get(uuid); - EXPECT_EQ(channelz_channel, retrieved); + BaseNode* retrieved = ChannelzRegistry::Get(channelz_channel->uuid()); + EXPECT_EQ(channelz_channel.get(), retrieved); } } // namespace testing