From 3d5849df50efca3a5b7d3ac13d9809dbf9d29648 Mon Sep 17 00:00:00 2001 From: James Buckland Date: Wed, 25 Apr 2018 16:39:41 -0400 Subject: [PATCH 1/7] First working draft of simpler reference-counted heap allocator Signed-off-by: James Buckland --- source/common/stats/stats_impl.cc | 29 ++++++++++++++++++++++++----- source/common/stats/stats_impl.h | 3 +++ 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 6d3dace509568..128937e4b8a28 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -150,9 +150,21 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector } RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { - // This must be zero-initialized - RawStatData* data = static_cast(::calloc(RawStatData::size(), 1)); - data->initialize(name); + absl::string_view key = name; + + if (key.size() > Stats::RawStatData::maxNameLength()) { + key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); + } + + RawStatData* data = stats_set_[std::string(key)]; + + if (stats_set_.count(std::string(key)) == 1) { // TODO do better + // didn't exist before now, actually create + data = static_cast(::calloc(RawStatData::size(), 1)); + data->initialize(name); + } else { + ++data->ref_count_; + } return data; } @@ -256,8 +268,15 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon } void HeapRawStatDataAllocator::free(RawStatData& data) { - // This allocator does not ever have concurrent access to the raw data. - ASSERT(data.ref_count_ == 1); + ASSERT(data.ref_count_ > 0); + if (--data.ref_count_ > 0) { + return; + } + + std::cout << data.key() << "\n"; + size_t key_removed = stats_set_.erase(std::string(data.key())); + std::cout << key_removed << "\n"; + ASSERT(key_removed >= 1); ::free(&data); } diff --git a/source/common/stats/stats_impl.h b/source/common/stats/stats_impl.h index 7168e00ba7b51..a8d2daa964671 100644 --- a/source/common/stats/stats_impl.h +++ b/source/common/stats/stats_impl.h @@ -414,6 +414,9 @@ class HeapRawStatDataAllocator : public RawStatDataAllocator { // RawStatDataAllocator RawStatData* alloc(const std::string& name) override; void free(RawStatData& data) override; + +private: + std::unordered_map stats_set_; }; /** From b6aa02971fc6596fae379b3ba0a2c9282a6ba833 Mon Sep 17 00:00:00 2001 From: James Buckland Date: Wed, 25 Apr 2018 17:11:21 -0400 Subject: [PATCH 2/7] Fixed reference to pointer to RawStatData Signed-off-by: James Buckland --- source/common/stats/stats_impl.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 128937e4b8a28..6416f4fcb3df2 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -156,9 +156,11 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); } - RawStatData* data = stats_set_[std::string(key)]; + bool name_not_in_map = (stats_set_.find(std::string(key)) == stats_set_.end()); - if (stats_set_.count(std::string(key)) == 1) { // TODO do better + RawStatData*& data = stats_set_[std::string(key)]; + + if (name_not_in_map) { // didn't exist before now, actually create data = static_cast(::calloc(RawStatData::size(), 1)); data->initialize(name); From d615eefcab538a6f34a826ec98241ec4387d5486 Mon Sep 17 00:00:00 2001 From: James Buckland Date: Wed, 25 Apr 2018 17:18:01 -0400 Subject: [PATCH 3/7] Add HeapAlloc test for HeapRawStatDataAllocator Signed-off-by: James Buckland --- test/common/stats/stats_impl_test.cc | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/common/stats/stats_impl_test.cc b/test/common/stats/stats_impl_test.cc index b01bff0a776a1..148a63172e4c0 100644 --- a/test/common/stats/stats_impl_test.cc +++ b/test/common/stats/stats_impl_test.cc @@ -478,5 +478,14 @@ TEST(RawStatDataTest, Truncate) { alloc.free(*stat); } +TEST(RawStatDataTest, HeapAlloc) { + HeapRawStatDataAllocator alloc; + RawStatData* stat_1 = alloc.alloc("ref_name"); + RawStatData* stat_2 = alloc.alloc("ref_name"); + EXPECT_EQ(stat_1, stat_2); + ::free(stat_1); + ::free(stat_2); +} + } // namespace Stats } // namespace Envoy From cdd8418a8f4fc5d4f5d8589ef8110edaca581292 Mon Sep 17 00:00:00 2001 From: James Buckland Date: Wed, 25 Apr 2018 17:45:16 -0400 Subject: [PATCH 4/7] Initialized data based on truncated name Signed-off-by: James Buckland --- source/common/stats/stats_impl.cc | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 6416f4fcb3df2..ccdb0f7456927 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -154,16 +154,18 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { if (key.size() > Stats::RawStatData::maxNameLength()) { key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); + ENVOY_LOG_MISC( + warn, + "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, + key.size(), Stats::RawStatData::maxNameLength()); } - bool name_not_in_map = (stats_set_.find(std::string(key)) == stats_set_.end()); - - RawStatData*& data = stats_set_[std::string(key)]; - - if (name_not_in_map) { + auto ret = stats_set_.insert(std::pair(std::string(key), nullptr)); + RawStatData*& data = ret.first->second; + if (ret.second) { // didn't exist before now, actually create data = static_cast(::calloc(RawStatData::size(), 1)); - data->initialize(name); + data->initialize(key); } else { ++data->ref_count_; } @@ -284,12 +286,6 @@ void HeapRawStatDataAllocator::free(RawStatData& data) { void RawStatData::initialize(absl::string_view key) { ASSERT(!initialized()); - if (key.size() > maxNameLength()) { - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), maxNameLength()); - } ref_count_ = 1; // key is not necessarily nul-terminated, but we want to make sure name_ is. From 101c7aa1f0330f99936825c49eefe12e92bd57d8 Mon Sep 17 00:00:00 2001 From: James Buckland Date: Wed, 25 Apr 2018 18:01:17 -0400 Subject: [PATCH 5/7] Remove leftover comments Signed-off-by: James Buckland --- source/common/stats/stats_impl.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index ccdb0f7456927..fdfabb5cd0589 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -163,7 +163,6 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { auto ret = stats_set_.insert(std::pair(std::string(key), nullptr)); RawStatData*& data = ret.first->second; if (ret.second) { - // didn't exist before now, actually create data = static_cast(::calloc(RawStatData::size(), 1)); data->initialize(key); } else { From 282c56bd4ddebac8434bbf97e4c2a8453982ac04 Mon Sep 17 00:00:00 2001 From: James Buckland Date: Wed, 25 Apr 2018 18:06:41 -0400 Subject: [PATCH 6/7] Initialized data based on non-truncated name Signed-off-by: James Buckland --- source/common/stats/stats_impl.cc | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index fdfabb5cd0589..7045f177f962a 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -154,17 +154,13 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { if (key.size() > Stats::RawStatData::maxNameLength()) { key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); - ENVOY_LOG_MISC( - warn, - "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, - key.size(), Stats::RawStatData::maxNameLength()); } auto ret = stats_set_.insert(std::pair(std::string(key), nullptr)); RawStatData*& data = ret.first->second; if (ret.second) { data = static_cast(::calloc(RawStatData::size(), 1)); - data->initialize(key); + data->initialize(name); } else { ++data->ref_count_; } @@ -285,6 +281,13 @@ void HeapRawStatDataAllocator::free(RawStatData& data) { void RawStatData::initialize(absl::string_view key) { ASSERT(!initialized()); + std::cout << key << "\n"; + if (key.size() > Stats::RawStatData::maxNameLength()) { + ENVOY_LOG_MISC( + warn, + "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, + key.size(), Stats::RawStatData::maxNameLength()); + } ref_count_ = 1; // key is not necessarily nul-terminated, but we want to make sure name_ is. From 830c718aad40d76428e00b4fcaa01e6152f5070c Mon Sep 17 00:00:00 2001 From: James Buckland Date: Wed, 25 Apr 2018 18:11:09 -0400 Subject: [PATCH 7/7] Initialized data based on truncated name, and also warn in both allocators Signed-off-by: James Buckland --- source/common/stats/stats_impl.cc | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/source/common/stats/stats_impl.cc b/source/common/stats/stats_impl.cc index 7045f177f962a..7387b7064832c 100644 --- a/source/common/stats/stats_impl.cc +++ b/source/common/stats/stats_impl.cc @@ -154,13 +154,17 @@ RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) { if (key.size() > Stats::RawStatData::maxNameLength()) { key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength()); + ENVOY_LOG_MISC( + warn, + "Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key, + key.size(), Stats::RawStatData::maxNameLength()); } auto ret = stats_set_.insert(std::pair(std::string(key), nullptr)); RawStatData*& data = ret.first->second; if (ret.second) { data = static_cast(::calloc(RawStatData::size(), 1)); - data->initialize(name); + data->initialize(key); } else { ++data->ref_count_; }