Skip to content
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
8de2463
WIP Working symbol table with manual reference counting
ambuc Jul 23, 2018
1c45948
Added tests, better error handling for null symbol lookups
ambuc Jul 23, 2018
a3df93b
Fix formatting
ambuc Jul 23, 2018
a78ea8d
Add common/pure header
ambuc Jul 23, 2018
59e8129
Add new StatNameImpl type for encapsulating the SymbolVec
ambuc Jul 24, 2018
475dcd3
Move the bulk of the logic to a .cc
ambuc Jul 24, 2018
9b3594b
Update tests
ambuc Jul 24, 2018
e7b619a
Add the SymbolTableImpl .cc
ambuc Jul 24, 2018
b3f1b7f
Mark internal string as const ref
ambuc Jul 25, 2018
bdc714c
More documentation around size()
ambuc Jul 25, 2018
0ded3e5
Decls before fields
ambuc Jul 25, 2018
b8b3f60
Hide toSymbols() fn from public API
ambuc Jul 25, 2018
ae650c3
Add proper test for death on the decode of an invalid symbol
ambuc Jul 25, 2018
14e8bb3
Added documentation for toSymbol() / fromSymbol()
ambuc Jul 25, 2018
86dc207
Add free pool for symbol re-use
ambuc Jul 25, 2018
cb6c754
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Jul 25, 2018
ff30317
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Jul 25, 2018
6e42c86
Various nits and docs fixes
ambuc Jul 26, 2018
f848464
Use std::stack<> for free pool
ambuc Jul 26, 2018
3687311
Optimized bimap using string_views
ambuc Jul 26, 2018
2772592
Add string_view return type for fromSymbol()
ambuc Jul 26, 2018
edb35e3
Added symbolic struct for SharedSymbol
ambuc Jul 26, 2018
c872073
SymbolTable::encode() now accepts a string_view
ambuc Jul 26, 2018
2f83e68
Simpler string construction in toSymbol()
ambuc Jul 26, 2018
85c5d8b
Use ASSERT() instead of RELEASE_ASSERT()
ambuc Jul 27, 2018
7f93df9
Assorted nits, todos, renaming
ambuc Jul 27, 2018
190da3d
Various const nits, stronger assertion against overflow
ambuc Jul 27, 2018
f2da353
Remove accidentally committed file
ambuc Jul 27, 2018
9e68dfd
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Jul 27, 2018
8e794d5
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Jul 31, 2018
bd47676
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 1, 2018
19c6e65
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 1, 2018
da2b7d7
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 1, 2018
9466aa3
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 7, 2018
86dab2c
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 14, 2018
208512d
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 14, 2018
27aa402
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 14, 2018
aaba51b
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 21, 2018
278f92f
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 24, 2018
ce87e7e
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 27, 2018
efd3c6f
Merge remote-tracking branch 'upstream/master' into symbol-table
ambuc Aug 27, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/envoy/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ envoy_cc_library(
deps = ["//include/envoy/common:interval_set_interface"],
)

envoy_cc_library(
name = "symbol_table_interface",
hdrs = ["symbol_table.h"],
)

envoy_cc_library(
name = "timespan",
hdrs = ["timespan.h"],
Expand Down
51 changes: 51 additions & 0 deletions include/envoy/stats/symbol_table.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
#pragma once

#include <memory>
#include <string>
#include <vector>

#include "envoy/common/pure.h"

namespace Envoy {
namespace Stats {

using Symbol = uint32_t;
using SymbolVec = std::vector<Symbol>;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can move both of these to the _impl.h


/**
* Interface for storing a stat name, which is a wrapper around a SymbolVec.
*/
class StatName {
public:
StatName() {}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Default constructor not needed.

virtual ~StatName(){};
virtual std::string toString() const PURE;
};

using StatNamePtr = std::unique_ptr<StatName>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be SharedPtr to support ref counted sharing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so -- StatNames should be totally unique, AFAIK.

@jmarantz jmarantz Jul 25, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I had thought of this too and almost wrote the same comment. I don't think this needs to be shared, but it's possible, based on usage there might emerge a need.


/**
* Interface for shortening and retrieving stat names.
*
* Guarantees that x = encode(x).toString() for any x.
*/
class SymbolTable {
public:
virtual ~SymbolTable() {}

/**
* Encodes a stat name into a StatNamePtr. Expects the name to be period-delimited.
*
* @param name the stat name to encode.
* @return StatNamePtr a unique_ptr to the StatName class encapsulating the symbol vector.
*/
virtual StatNamePtr encode(const std::string& name) PURE;

/**
* Returns the size of a SymbolTable, as measured in number of symbols stored.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: @return docs.

*/
virtual size_t size() const PURE;
};

} // namespace Stats
} // namespace Envoy
12 changes: 12 additions & 0 deletions source/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ envoy_cc_library(
"libcircllhist",
],
deps = [
":symbol_table_lib",
"//include/envoy/common:time_interface",
"//include/envoy/server:options_interface",
"//include/envoy/stats:stats_interface",
Expand All @@ -33,6 +34,17 @@ envoy_cc_library(
],
)

envoy_cc_library(
name = "symbol_table_lib",
srcs = ["symbol_table_impl.cc"],
hdrs = ["symbol_table_impl.h"],
external_deps = ["abseil_base"],
deps = [
"//include/envoy/stats:symbol_table_interface",
"//source/common/common:assert_lib",
],
)

envoy_cc_library(
name = "thread_local_store_lib",
srcs = ["thread_local_store.cc"],
Expand Down
79 changes: 79 additions & 0 deletions source/common/stats/symbol_table_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
#include "common/stats/symbol_table_impl.h"

#include <memory>
#include <unordered_map>
#include <vector>

#include "common/common/assert.h"

namespace Envoy {
namespace Stats {

// TODO(ambuc): There is a possible performance optimization here for avoiding the encoding of IPs,
// if they appear in stat names. We don't want to waste time symbolizing an integer as an integer,
// if we can help it.
StatNamePtr SymbolTableImpl::encode(const std::string& name) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think encode() should take an absl::string_view as well.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, implemented in c872073.

SymbolVec symbol_vec;
std::vector<std::string> name_vec = absl::StrSplit(name, '.');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl::string_view

symbol_vec.reserve(name_vec.size());
std::transform(name_vec.begin(), name_vec.end(), std::back_inserter(symbol_vec),
[this](std::string x) { return toSymbol(x); });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl::string_view


return std::make_unique<StatNameImpl>(symbol_vec, this);
}

std::string SymbolTableImpl::decode(const SymbolVec& symbol_vec) const {
std::vector<std::string> name;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl::string_view

name.reserve(symbol_vec.size());
std::transform(symbol_vec.begin(), symbol_vec.end(), std::back_inserter(name),
[this](Symbol x) { return fromSymbol(x); });
return absl::StrJoin(name, ".");
}

void SymbolTableImpl::free(const SymbolVec& symbol_vec) {
for (const Symbol symbol : symbol_vec) {
auto decode_search = decode_map_.find(symbol);
RELEASE_ASSERT(decode_search != decode_map_.end(), "");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should not be release asserts. If these are invariants/preconditions, which I believe they are, the Envoy style is to use ASSERT and documentation. RELEASE_ASSERTs are reserved for places where we are eliding error handling, see https://github.com/envoyproxy/envoy/blob/master/STYLE.md#error-handling for full discussion.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed in 85c5d8b.


const std::string& str = decode_search->second;
auto encode_search = encode_map_.find(str);
RELEASE_ASSERT(encode_search != encode_map_.end(), "");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto


((encode_search->second).second)--;
// If that was the last remaining client usage of the symbol, erase the the current
// mappings and add the now-unused symbol to the reuse pool.
if ((encode_search->second).second == 0) {
decode_map_.erase(decode_search);
encode_map_.erase(encode_search);
pool_.push_back(symbol);
}
}
}

Symbol SymbolTableImpl::toSymbol(const std::string& str) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl::string_view all the way down :)

Symbol result;
auto encode_insert = encode_map_.insert({str, std::make_pair(current_symbol_, 1)});
// If the insertion took place, we mirror the insertion in the decode_map.
if (encode_insert.second) {
auto decode_insert = decode_map_.insert({current_symbol_, str});
// We expect the decode_map to be in lockstep.
RELEASE_ASSERT(decode_insert.second, "");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above points on RELEASE_ASSERT.

result = current_symbol_;
newSymbol();
} else {
// If the insertion didn't take place, return the actual value at that location
result = (encode_insert.first)->second.first;
// and up the refcount at that location
++(encode_insert.first)->second.second;
}
return result;
}

std::string SymbolTableImpl::fromSymbol(const Symbol symbol) const {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

absl::string_view

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See 3687311.

auto search = decode_map_.find(symbol);
RELEASE_ASSERT(search != decode_map_.end(), "");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

return search->second;
}

} // namespace Stats
} // namespace Envoy
122 changes: 122 additions & 0 deletions source/common/stats/symbol_table_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
#pragma once

#include <algorithm>
#include <memory>
#include <string>
#include <unordered_map>
#include <vector>

#include "envoy/common/exception.h"
#include "envoy/stats/symbol_table.h"

#include "common/common/assert.h"

#include "absl/strings/str_join.h"
#include "absl/strings/str_split.h"

namespace Envoy {
namespace Stats {

/**
* Underlying SymbolTableImpl implementation which assists with per-symbol reference counting.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/which assists with/manages/

*
* The underlying Symbol / SymbolVec data structures are not meant for public consumption. One side

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/not meant for public consumption/private to the impl/

* effect of the non-monotonically-increasing symbol counter is that if a string is encoded, the
* resulting stat is destroyed, and then that same string is re-encoded, it may or may not encode to
* the same underlying symbol.
*/
class SymbolTableImpl : public SymbolTable {
public:
StatNamePtr encode(const std::string& name) override;

@jmarantz jmarantz Jul 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try making this return std::unique_ptr<StatNameImpl>? That would help avoid the dynamic_cast in your tests. I'm not 100% sure if that's a valid covariant return, but it is worth a shot.

I think the dynamic_casts you have are OK if you need to have them, but let's see if we can avoid it if it's easy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that covariant returns won't work here because of a weird cross-dependency between SymbolTableImpl and StatNameImpl. StatNameImpl expects a SymbolTableImpl& to store internally, so if we declare a covariant return it's unclear which of the two impls to write first, since they both would depend on each other. I'm comfortable with the dynamic_cast<> if it avoids this problem.


// For testing purposes only.
size_t size() const override {
RELEASE_ASSERT(encode_map_.size() == decode_map_.size(), "");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

return encode_map_.size();
}

private:
friend class StatNameImpl;
friend class StatNameTest;
/**
* Decodes a vector of symbols back into its period-delimited stat name.
* If decoding fails on any part of the symbol_vec, we release_assert and crash hard, since this
* should never happen, and we don't want to continue running with a corrupt stats set.
*
* @param symbol_vec the vector of symbols to decode.
* @return std::string the retrieved stat name.
*/
std::string decode(const SymbolVec& symbol_vec) const;

/**
* Since SymbolTableImpl does manual reference counting, a client of SymbolTable (such as
* StatName) must manually call free(symbol_vec) when it is freeing the stat it represents. This
* way, the symbol table will grow and shrink dynamically, instead of being write-only.
*

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: @param

* @param SymbolVec& the vector of symbols to be freed.
* @return bool whether or not the total free operation was successful. Expected to be true.
*/
void free(const SymbolVec& symbol_vec);

/**
* Convenience function for encode(), symbolizing one string segment at a time.
*
* @param std::string& the individual string to be encoded as a symbol.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Envoy convention (which has an unknown relationship to actual doxygen :) is you need here the name of the parameter, rather than the type of it. In contrast, the return annotation needs the type, and there is no name...

* @return Symbol the encoded string.
*/
Symbol toSymbol(const std::string& str);

/**
* Convenience function for decode(), decoding one symbol at a time.
*
* @param Symbol the individual symbol to be decoded.
* @return std::string the decoded string.
*/
std::string fromSymbol(const Symbol symbol) const;

// Stages a new symbol for use. To be called after a successful insertion.
void newSymbol() {
if (pool_.empty()) {
RELEASE_ASSERT(monotonic_counter_ < std::numeric_limits<Symbol>::max(), "");
current_symbol_ = ++monotonic_counter_;
} else {
current_symbol_ = pool_.front();
pool_.pop_front();
}
}

// Stores the symbol to be used at next insertion. This should exist ahead of insertion time so
// that if insertion succeeds, the value written is the correct one.
Symbol current_symbol_{};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does the {} do here? Would it be clearer to do " = 0" as the initializer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think it would be clearer. I'll change it.


// If the free pool is exhausted, we monotonically increase this counter.
Symbol monotonic_counter_{};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you need tests to access this, you should still define a private accessor, rather than dereferencing the member var from outside the class.


// Bimap implementation.
// The encode map stores both the symbol and the ref count of that symbol.
std::unordered_map<std::string, std::pair<Symbol, uint32_t>> encode_map_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I was messing around in some of my own experiments, and want to propose an alternative to using pair here, to make call sites more readable. Declare a struct:

struct SharedSymbol { Symbol symbol_; uint32_t ref_count_; };

Dereferences this are more readable because they are symbolic.
Construction can also be done with this syntax:

  { .symbol_ = sym, .ref_count_ = 1; }

etc.

https://en.cppreference.com/w/c/language/struct

I think this is relatively new syntax, as in multiple decades of C/C++ I never saw this before. But it's pretty nifty IMO.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You were right, I missed this comment. Implementing this in edb35e3.

std::unordered_map<Symbol, std::string> decode_map_;

// Free pool of symbols for re-use.
std::deque<Symbol> pool_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deques are pretty slow; any reason not to use a stack or queue?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, what do you think is best here? I don't have a strong opinion for reading from this collection in any particular order, but it could get fairly large if lots of stats are freed in succession.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a commit here f848464 making this a std::stack instead :)

};

/**
* Implements RAII for Symbols, since the StatName destructor does the work of freeing its component

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

* symbols.
*/
class StatNameImpl : public StatName {
public:
StatNameImpl(SymbolVec symbol_vec, SymbolTableImpl* symbol_table)
: symbol_vec_(symbol_vec), symbol_table_(symbol_table) {}
~StatNameImpl() override { symbol_table_->free(symbol_vec_); }
std::string toString() const override { return symbol_table_->decode(symbol_vec_); }

private:
friend class StatNameTest;
SymbolVec symbol_vec_;
SymbolTableImpl* symbol_table_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Envoy convention here is to use a non-const ref.

};

} // namespace Stats
} // namespace Envoy
12 changes: 12 additions & 0 deletions test/common/stats/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,22 @@ envoy_cc_test(
],
)

envoy_cc_test(
name = "symbol_table_test",
srcs = ["symbol_table_test.cc"],
deps = [
"//source/common/stats:symbol_table_lib",
"//test/mocks/stats:stats_mocks",
"//test/test_common:logging_lib",
"//test/test_common:utility_lib",
],
)

envoy_cc_test(
name = "thread_local_store_test",
srcs = ["thread_local_store_test.cc"],
deps = [
"//source/common/stats:symbol_table_lib",
"//source/common/stats:thread_local_store_lib",
"//test/mocks/event:event_mocks",
"//test/mocks/server:server_mocks",
Expand Down
Loading