From 76058f69283ae7610743218c8c3ffb5bb080f52d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 30 Apr 2019 17:11:34 -0400 Subject: [PATCH 1/4] Add a new class StatNameManagedContainer to abstract some StatName memory management. Signed-off-by: Joshua Marantz --- source/common/stats/fake_symbol_table_impl.h | 5 +-- source/common/stats/symbol_table_impl.cc | 15 +++++++ source/common/stats/symbol_table_impl.h | 32 +++++++++++++- test/common/stats/symbol_table_impl_test.cc | 44 ++++++++++++-------- test/common/stats/thread_local_store_test.cc | 27 +++--------- 5 files changed, 79 insertions(+), 44 deletions(-) diff --git a/source/common/stats/fake_symbol_table_impl.h b/source/common/stats/fake_symbol_table_impl.h index 71560908c1574..f26d7513ea11e 100644 --- a/source/common/stats/fake_symbol_table_impl.h +++ b/source/common/stats/fake_symbol_table_impl.h @@ -109,9 +109,8 @@ class FakeSymbolTableImpl : public SymbolTable { SymbolTable::StoragePtr join(const std::vector& names) const override { std::vector strings; for (StatName name : names) { - absl::string_view str = toStringView(name); - if (!str.empty()) { - strings.push_back(str); + if (!name.empty()) { + strings.push_back(toStringView(name)); } } return encodeHelper(absl::StrJoin(strings, ".")); diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index 6a96a340d72c8..3c7f623d6f9b4 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -317,6 +317,21 @@ void StatNameStorage::free(SymbolTable& table) { bytes_.reset(); } +StatNameManagedContainer::~StatNameManagedContainer() { + for (StatNameStorage& storage : storage_vector_) { + storage.free(symbol_table_); + } +} + +uint8_t* StatNameManagedContainer::addReturningStorage(absl::string_view str) { + storage_vector_.push_back(Stats::StatNameStorage(str, symbol_table_)); + return storage_vector_.back().bytes(); +} + +StatName StatNameManagedContainer::add(absl::string_view str) { + return StatName(addReturningStorage(str)); +} + StatNameStorageSet::~StatNameStorageSet() { // free() must be called before destructing StatNameStorageSet to decrement // references to all symbols. diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index 173ad3e7a0176..e8de5d81dcf95 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -287,6 +287,8 @@ class StatNameStorage { */ inline StatName statName() const; + uint8_t* bytes() { return bytes_.get(); } + private: SymbolTable::StoragePtr bytes_; }; @@ -353,10 +355,15 @@ class StatName { #endif /** - * @return uint8_t* A pointer to the first byte of data (skipping over size bytes). + * @return A pointer to the first byte of data (skipping over size bytes). */ const uint8_t* data() const { return size_and_data_ + StatNameSizeEncodingBytes; } + /** + * @return whether this is empty. + */ + bool empty() const { return size_and_data_ == nullptr || dataSize() == 0; } + private: const uint8_t* size_and_data_; }; @@ -400,6 +407,29 @@ class StatNameManagedStorage : public StatNameStorage { SymbolTable& symbol_table_; }; +class StatNameManagedContainer { +public: + explicit StatNameManagedContainer(SymbolTable& symbol_table) : symbol_table_(symbol_table) {} + ~StatNameManagedContainer(); + + /** + * @param name the name to add the container. + * @return the StatName held in the container for this name. + */ + StatName add(absl::string_view name); + + /** + * @param name the name to add the container. + * @return a pointer to the bytes held in the container for this name, suitable for + * using to construct a StatName. + */ + uint8_t* addReturningStorage(absl::string_view name); + +private: + SymbolTable& symbol_table_; + std::vector storage_vector_; +}; + // Represents an ordered container of StatNames. The encoding for each StatName // is byte-packed together, so this carries less overhead than allocating the // storage separately. The tradeoff is there is no random access; you can only diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index a53e6ad8979c4..e03d803ffb146 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -45,17 +45,18 @@ class StatNameTest : public testing::TestWithParam { table_ = std::move(table); break; } + initStorage(); } + ~StatNameTest() override { clearStorage(); } void clearStorage() { - for (auto& stat_name_storage : stat_name_storage_) { - stat_name_storage.free(*table_); - } - stat_name_storage_.clear(); + stat_name_storage_.reset(); EXPECT_EQ(0, table_->numSymbols()); } + void initStorage() { stat_name_storage_ = std::make_unique(*table_); } + SymbolVec getSymbols(StatName stat_name) { return SymbolTableImpl::Encoding::decodeSymbols(stat_name.data(), stat_name.dataSize()); } @@ -67,18 +68,13 @@ class StatNameTest : public testing::TestWithParam { return table_->toString(makeStat(stat_name)); } - StatNameStorage makeStatStorage(absl::string_view name) { return StatNameStorage(name, *table_); } - - StatName makeStat(absl::string_view name) { - stat_name_storage_.emplace_back(makeStatStorage(name)); - return stat_name_storage_.back().statName(); - } + StatName makeStat(absl::string_view name) { return stat_name_storage_->add(name); } FakeSymbolTableImpl* fake_symbol_table_{nullptr}; SymbolTableImpl* real_symbol_table_{nullptr}; std::unique_ptr table_; - std::vector stat_name_storage_; + std::unique_ptr stat_name_storage_; }; INSTANTIATE_TEST_CASE_P(StatNameTest, StatNameTest, @@ -93,6 +89,12 @@ TEST_P(StatNameTest, TestArbitrarySymbolRoundtrip) { } } +TEST_P(StatNameTest, TestEmpty) { + EXPECT_TRUE(stat_name_storage_->add("").empty()); + EXPECT_FALSE(stat_name_storage_->add("x").empty()); + EXPECT_TRUE(StatName().empty()); +} + TEST_P(StatNameTest, Test100KSymbolsRoundtrip) { for (int i = 0; i < 100 * 1000; ++i) { const std::string stat_name = absl::StrCat("symbol_", i); @@ -141,6 +143,7 @@ TEST_P(StatNameDeathTest, TestBadDecodes) { // Decoding a symbol vec that exists is perfectly normal... EXPECT_NO_THROW(decodeSymbolVec(vec_1)); clearStorage(); + initStorage(); // But when the StatName is destroyed, its symbols are as well. EXPECT_DEATH(decodeSymbolVec(vec_1), ""); } @@ -170,7 +173,7 @@ TEST_P(StatNameTest, TestSameValueOnPartialFree) { // This should hold true for components as well. Since "foo" persists even when "foo.bar" is // freed, we expect both instances of "foo" to have the same symbol. makeStat("foo"); - StatNameStorage stat_foobar_1(makeStatStorage("foo.bar")); + StatNameStorage stat_foobar_1("foo.bar", *table_); SymbolVec stat_foobar_1_symbols = getSymbols(stat_foobar_1.statName()); stat_foobar_1.free(*table_); StatName stat_foobar_2(makeStat("foo.bar")); @@ -201,6 +204,7 @@ TEST_P(StatNameTest, FreePoolTest) { EXPECT_EQ(monotonicCounter(), 5); EXPECT_EQ(table_->numSymbols(), 5); clearStorage(); + initStorage(); } EXPECT_EQ(monotonicCounter(), 5); EXPECT_EQ(table_->numSymbols(), 0); @@ -226,22 +230,26 @@ TEST_P(StatNameTest, TestShrinkingExpectation) { // ::size() is a public function, but should only be used for testing. size_t table_size_0 = table_->numSymbols(); - StatNameStorage stat_a(makeStatStorage("a")); + auto make_stat_storage = [this](absl::string_view name) -> StatNameStorage { + return StatNameStorage(name, *table_); + }; + + StatNameStorage stat_a(make_stat_storage("a")); size_t table_size_1 = table_->numSymbols(); - StatNameStorage stat_aa(makeStatStorage("a.a")); + StatNameStorage stat_aa(make_stat_storage("a.a")); EXPECT_EQ(table_size_1, table_->numSymbols()); - StatNameStorage stat_ab(makeStatStorage("a.b")); + StatNameStorage stat_ab(make_stat_storage("a.b")); size_t table_size_2 = table_->numSymbols(); - StatNameStorage stat_ac(makeStatStorage("a.c")); + StatNameStorage stat_ac(make_stat_storage("a.c")); size_t table_size_3 = table_->numSymbols(); - StatNameStorage stat_acd(makeStatStorage("a.c.d")); + StatNameStorage stat_acd(make_stat_storage("a.c.d")); size_t table_size_4 = table_->numSymbols(); - StatNameStorage stat_ace(makeStatStorage("a.c.e")); + StatNameStorage stat_ace(make_stat_storage("a.c.e")); size_t table_size_5 = table_->numSymbols(); EXPECT_GE(table_size_5, table_size_4); diff --git a/test/common/stats/thread_local_store_test.cc b/test/common/stats/thread_local_store_test.cc index 86c883b7412a4..813b6ae6eb316 100644 --- a/test/common/stats/thread_local_store_test.cc +++ b/test/common/stats/thread_local_store_test.cc @@ -480,34 +480,17 @@ TEST_F(StatsThreadLocalStoreTest, HotRestartTruncation) { class LookupWithStatNameTest : public testing::Test { public: - LookupWithStatNameTest() : alloc_(symbol_table_), store_(options_, alloc_) {} - ~LookupWithStatNameTest() override { - store_.shutdownThreading(); - clearStorage(); - } - - void clearStorage() { - for (auto& stat_name_storage : stat_name_storage_) { - stat_name_storage.free(store_.symbolTable()); - } - stat_name_storage_.clear(); - EXPECT_EQ(0, store_.symbolTable().numSymbols()); - } - - StatName makeStatName(absl::string_view name) { - stat_name_storage_.emplace_back(makeStatStorage(name)); - return stat_name_storage_.back().statName(); - } + LookupWithStatNameTest() + : alloc_(symbol_table_), store_(options_, alloc_), stat_name_storage_(symbol_table_) {} + ~LookupWithStatNameTest() override { store_.shutdownThreading(); } - StatNameStorage makeStatStorage(absl::string_view name) { - return StatNameStorage(name, store_.symbolTable()); - } + StatName makeStatName(absl::string_view name) { return stat_name_storage_.add(name); } Stats::FakeSymbolTableImpl symbol_table_; HeapStatDataAllocator alloc_; StatsOptionsImpl options_; ThreadLocalStoreImpl store_; - std::vector stat_name_storage_; + StatNameManagedContainer stat_name_storage_; }; TEST_F(LookupWithStatNameTest, All) { From 029693b123a36be724def0c914d57e4591fbc874 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 1 May 2019 19:38:22 -0400 Subject: [PATCH 2/4] Add usage to StatNamePool doc-string. Signed-off-by: Joshua Marantz --- source/common/stats/symbol_table_impl.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index f3a2f7365ba33..24e1d0224ccb6 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -412,6 +412,13 @@ class StatNameManagedStorage : public StatNameStorage { * StatNameManagedStorage, this has an RAII usage model, taking * care of decrementing ref-counts in the SymbolTable for all * contained StatNames on destruction or on clear(); + * + * Example usage: + * StatNamePool pool(symbol_table); + * StatName name1 = pool.add("name1"); + * StatName name2 = pool.add("name2"); + * uint8_t* storage = pool.addReturningStorage("name3"); + * StatName name3(storage); */ class StatNamePool { public: From bbcf9e984edc8d2d2f44645f1ddb613f5fe1f832 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 1 May 2019 19:41:56 -0400 Subject: [PATCH 3/4] Add more detailed description of what addReturningStorage() does. Signed-off-by: Joshua Marantz --- source/common/stats/symbol_table_impl.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index 24e1d0224ccb6..d61c6b9f36a3d 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -437,6 +437,11 @@ class StatNamePool { StatName add(absl::string_view name); /** + * Does essentially the same thing as add(), but returns the storage as a + * pointer which can later be used to create a StatName. This can be used + * to accumulate a vector of uint8_t* which can later be used to create + * StatName objects on demand. + * * @param name the name to add the container. * @return a pointer to the bytes held in the container for this name, suitable for * using to construct a StatName. From aa0956ea58259690e5ed8c1821592e1a543047e0 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Mon, 6 May 2019 18:27:23 -0400 Subject: [PATCH 4/4] Remove addReturningStorage() for now and comment use of StatNameStorage and explicit dtor. Signed-off-by: Joshua Marantz --- source/common/stats/symbol_table_impl.cc | 6 ++---- source/common/stats/symbol_table_impl.h | 17 +++-------------- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/source/common/stats/symbol_table_impl.cc b/source/common/stats/symbol_table_impl.cc index c9b06f6f56fce..9b85687a66bcf 100644 --- a/source/common/stats/symbol_table_impl.cc +++ b/source/common/stats/symbol_table_impl.cc @@ -324,13 +324,11 @@ void StatNamePool::clear() { storage_vector_.clear(); } -uint8_t* StatNamePool::addReturningStorage(absl::string_view str) { +StatName StatNamePool::add(absl::string_view str) { storage_vector_.push_back(Stats::StatNameStorage(str, symbol_table_)); - return storage_vector_.back().bytes(); + return StatName(storage_vector_.back().bytes()); } -StatName StatNamePool::add(absl::string_view str) { return StatName(addReturningStorage(str)); } - StatNameStorageSet::~StatNameStorageSet() { // free() must be called before destructing StatNameStorageSet to decrement // references to all symbols. diff --git a/source/common/stats/symbol_table_impl.h b/source/common/stats/symbol_table_impl.h index d61c6b9f36a3d..366963f790c77 100644 --- a/source/common/stats/symbol_table_impl.h +++ b/source/common/stats/symbol_table_impl.h @@ -417,8 +417,6 @@ class StatNameManagedStorage : public StatNameStorage { * StatNamePool pool(symbol_table); * StatName name1 = pool.add("name1"); * StatName name2 = pool.add("name2"); - * uint8_t* storage = pool.addReturningStorage("name3"); - * StatName name3(storage); */ class StatNamePool { public: @@ -436,19 +434,10 @@ class StatNamePool { */ StatName add(absl::string_view name); - /** - * Does essentially the same thing as add(), but returns the storage as a - * pointer which can later be used to create a StatName. This can be used - * to accumulate a vector of uint8_t* which can later be used to create - * StatName objects on demand. - * - * @param name the name to add the container. - * @return a pointer to the bytes held in the container for this name, suitable for - * using to construct a StatName. - */ - uint8_t* addReturningStorage(absl::string_view name); - private: + // We keep the stat names in a vector of StatNameStorage, storing the + // SymbolTable reference separately. This saves 8 bytes per StatName, + // at the cost of having a destructor that calls clear(). SymbolTable& symbol_table_; std::vector storage_vector_; };