diff --git a/source/common/stats/symbol_table.cc b/source/common/stats/symbol_table.cc index 28279ccdb9da4..085e3e37d404a 100644 --- a/source/common/stats/symbol_table.cc +++ b/source/common/stats/symbol_table.cc @@ -261,6 +261,12 @@ void SymbolTable::Encoding::appendToMemBlock(StatName stat_name, } } +void SymbolTable::Encoding::appendStatNameDataTo(StatName stat_name, + MemBlockBuilder& mem_block) { + auto sv = stat_name.dataAsStringView(); + mem_block.appendData(absl::MakeSpan(reinterpret_cast(sv.data()), sv.size())); +} + SymbolTable::SymbolTable() // Have to be explicitly initialized, if we want to use the ABSL_GUARDED_BY macro. : next_symbol_(FirstValidSymbol), monotonic_counter_(FirstValidSymbol) {} @@ -551,7 +557,7 @@ StatNameStorage::StatNameStorage(absl::string_view name, SymbolTable& table) StatNameStorage::StatNameStorage(StatName src, SymbolTable& table) { const size_t size = src.size(); MemBlockBuilder storage(size); // Note: MemBlockBuilder takes uint64_t. - src.copyToMemBlock(storage); + SymbolTable::Encoding::appendToMemBlock(src, storage); setBytes(storage.release()); table.incRefCount(statName()); } @@ -666,7 +672,7 @@ SymbolTable::StoragePtr SymbolTable::join(const StatNameVec& stat_names) const { MemBlockBuilder mem_block(Encoding::totalSizeBytes(num_bytes)); Encoding::appendEncoding(num_bytes, mem_block); for (StatName stat_name : stat_names) { - stat_name.appendDataToMemBlock(mem_block); + Encoding::appendStatNameDataTo(stat_name, mem_block); } ASSERT(mem_block.capacityRemaining() == 0); return mem_block.release(); diff --git a/source/common/stats/symbol_table.h b/source/common/stats/symbol_table.h index 8f2fc96631dd8..715ff412ef81c 100644 --- a/source/common/stats/symbol_table.h +++ b/source/common/stats/symbol_table.h @@ -178,6 +178,14 @@ class SymbolTable final { */ static void appendEncoding(uint64_t number, MemBlockBuilder& mem_block); + /** + * Appends stat_name's data into storage. + * + * @param stat_name the stat_name to append. + * @param mem_block the memory block to append to. + */ + static void appendStatNameDataTo(StatName stat_name, MemBlockBuilder& mem_block); + /** * Appends stat_name's bytes into mem_block, which must have been allocated to * allow for stat_name.size() bytes. @@ -633,36 +641,15 @@ class StatName { size_t dataSize() const { return dataAsStringView().size(); } /** - * @return size_t the number of bytes in the symbol array, including the - * overhead for the size itself. + * @return size_t the number of bytes in the symbol array, including the overhead for the size + * itself. NOTE: this will return at least one byte even when there is no backing storage, to + * indicate the encoded length prefix when encoding the StatName into a StatNameList and so on. + * However, you can NEVER assume that data()/dataIncludingSize() will return non-nullptr just + * because this returns non-zero; the empty()/dataSize() APIs should be used to check whether the + * StatName has valid data. */ size_t size() const { return SymbolTable::Encoding::totalSizeBytes(dataSize()); } - /** - * Copies the entire StatName representation into a MemBlockBuilder, including - * the length metadata at the beginning. The MemBlockBuilder must not have - * any other data in it. - * - * @param mem_block_builder the builder to receive the storage. - */ - void copyToMemBlock(MemBlockBuilder& mem_block_builder) { - ASSERT(mem_block_builder.size() == 0); - mem_block_builder.appendData(absl::MakeSpan(size_and_data_, size())); - } - - /** - * Appends the data portion of the StatName representation into a - * MemBlockBuilder, excluding the length metadata. This is appropriate for - * join(), where several stat-names are combined, and we only need the - * aggregated length metadata. - * - * @param mem_block_builder the builder to receive the storage. - */ - void appendDataToMemBlock(MemBlockBuilder& storage) { - auto sv = dataAsStringView(); - storage.appendData(absl::MakeSpan(reinterpret_cast(sv.data()), sv.size())); - } - #ifndef ENVOY_CONFIG_COVERAGE void debugPrint(); #endif @@ -695,7 +682,6 @@ class StatName { */ bool startsWith(StatName symbolic_prefix) const; -private: /** * Casts the raw data as a string_view, decoding the varint length prefix. * All accessors that need the data pointer and/or size should delegate to @@ -709,6 +695,7 @@ class StatName { return {reinterpret_cast(size_and_data_ + prefix_size), data_size}; } +private: const uint8_t* size_and_data_{nullptr}; }; diff --git a/test/common/stats/symbol_table_impl_test.cc b/test/common/stats/symbol_table_impl_test.cc index 803b252e290e8..483a2e79716ba 100644 --- a/test/common/stats/symbol_table_impl_test.cc +++ b/test/common/stats/symbol_table_impl_test.cc @@ -701,6 +701,15 @@ TEST_F(StatNameTest, StorageCopy) { EXPECT_NE(a.data(), c.data()); } +TEST_F(StatNameTest, StorageFromEmptyStatName) { + StatName empty; + StatNameStorage b_storage(empty, table_); + const StatName b = b_storage.statName(); + EXPECT_EQ(empty, b); + EXPECT_NE(empty.data(), b.data()); + b_storage.free(table_); +} + TEST_F(StatNameTest, AddingToPoolViaStatNamePreservesDynamicSegments) { const StatNameDynamicStorage tag_name("tag", table_); const StatNameDynamicStorage tag_value("value", table_);