stats: add symbol table for future stat name encoding #3927

ambuc · 2018-07-23T15:04:47Z

Description: Add symbol table implementation, to be used for encoding heap-allocated stat names in the future. The symbol table grows and shrinks dynamically with the number of unique string segments in it, and guarantees x = decode(encode(x)) for as long as at least one instance of each string segment in x exists. Implicitly splits on period-delimited stat names. Does not enforce stat name length or string segment length.

Risk Level: Low, adds library but does not add dependencies on it anywhere.
Testing: See //test/common/stats:symbol_table_test.
Docs Changes: None
Release Notes: None

Signed-off-by: James Buckland <[email protected]>

mrice32

High level comments to start. Thanks for working on this!

mrice32 · 2018-07-23T15:15:57Z

source/common/stats/symbol_table_impl.h

+public:
+  std::vector<Symbol> encode(const std::string& name) override {
+    std::vector<Symbol> symbol_vec;
+    std::vector<std::string> name_vec = absl::StrSplit(name, '.');


Keep in mind that some prefixes/values contain . (ipv4 addresses, for instance), so this way of splitting won't necessarily line up with the individual elements that compose stats. I don't think it'll be a problem, however, because it'll just mean that some symbols will be more subdivided than one might expect.

This is a good note -- I'd bet that alongside other common patterns this implementation will be inefficient for large numbers of small string segments. We could do a first pass on encoding to regex-match IP addresses and encode them as unique symbols -- do you think this is worth doing in a separate PR?

Yes, definitely a separate PR. I wouldn't worry about it here. Another option would be to change how IP addresses are represented in stats by changing their delimiter, but I think that's probably a discussion for a different PR :).

Just leave a TODO for now I reckon.

I think actually we don't want to encode each IP as a unique symbol -- especially for Envoys with large throughput the cost of encoding each IP as a unique token (mapping an int to... another int?) might be too expensive. I'll put a TODO here for myself to think about this in the future.

mrice32 · 2018-07-23T15:17:08Z

source/common/stats/symbol_table_impl.h

+
+class SymbolTableImpl : public SymbolTable {
+public:
+  std::vector<Symbol> encode(const std::string& name) override {


Why are all of these methods implemented in the header?

No good reason -- I will move them to the .cc when responding to other incumbent reviews.

SG, thanks.

mrice32 · 2018-07-23T15:30:21Z

source/common/stats/symbol_table_impl.h

+
+  // 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_ = {};


Do you have a plan for how to get this into shared memory with no heap allocation? Isn't that the ultimate goal of this data structure?

In anticipation of #3710 which decouples the heap stats from shared-memory stats under the hood, we will only use this symbol table with the heap stat implementation.

Gotcha. That makes sense. Thanks for the explanation.

I'm thinking we need a better story here for how we will solve for shared memory, unless we want to make hot restart mutually exclusive with scalable stats (and scalable clusters). Istio, for example, sitll uses hot restart and wants to scale to very large cluster sizes. @mattklein123 @rshriram.

That was my thinking -- that users of hot-restart didn't have stat-name scalability issues. But I didn't realize Istio might be a counter-example. It's definitely way easier to solve this for non-hot-restart first in any case, and once #3710 is merged it is a separate problem.

Of course we don't have a problem making maps in shared memory -- BlockMemoryHashSet can be templated on a pair and use only .first for hashing/comparing. But the annoying part is the symbol characters. We'd have to tune the number and size of the symbols. It's much easier in the non-hot-restart case which doesn't require tuning that.

I would suggest that we discuss on the next community call.

FWIW I think it's probably OK to diverge the implementations between shared memory and the heap allocator, especially if we end up implementing #3804 via the "light hot restart" proposal that @bplotnick is doing. For example, I can imagine a configuration in which the user uses the heap allocator to save memory, but still wants to use light hot restart, and just deals with the fact that there are stat inconsistencies during hot restart.

mrice32

Some general comments. Mostly performance nits.

mrice32 · 2018-07-23T17:31:51Z

source/common/stats/symbol_table_impl.h

+
+  std::string decode(const std::vector<Symbol>& symbol_vec) const override {
+    std::vector<std::string> name;
+    name.resize(symbol_vec.size());


I think a slightly more efficient way to do this would be to make this a .reserve(...) and pass std::back_inserter(name) into the next function rather than name.begin(). Similar comment for the above use of std::transform.

mrice32 · 2018-07-23T17:38:40Z

source/common/stats/symbol_table_impl.h

+  std::string decode(const std::vector<Symbol>& symbol_vec) const override {
+    std::vector<std::string> name;
+    name.resize(symbol_vec.size());
+    std::transform(symbol_vec.begin(), symbol_vec.end(), name.begin(),


I think it would be better if you just loop through once and concatenate as you go to the output string rather than creating the intermediate vector of strings.

If we care we should micro-benchmark, but keep in mind that StrJoin will right-size the resulting string-array ahead of time, which is better than concatenating iteratively.

That makes sense. However, I think fewer allocations are generally better, and in the current implementation we're already allocating all those bytes in their individual strings within the vector in as well as allocating the vector itself before we even get to allocating the resulting string. Pre-micro benchmark I think fewer allocations can be assumed to be better until proven otherwise.

Your point makes sense, though. If we can put string_views or const std::string* (meaning fromSymbol() needs to return a string_view or const std::string&) in a preallocated vector and call StrJoin on that, it would likely be the best solution since we would get preallocation on both structures with no additional heap use.

Q: Before getting into micro perf optimizations, do you have any rough simulations of how much memory this scheme saves? Obviously the symbol table has overhead. I'm just curious where the inflection point is. Is it always better? At what point does it become better? How much better? Etc.

mrice32 · 2018-07-23T17:41:46Z

source/common/stats/symbol_table_impl.h

+    return result;
+  }
+
+  std::string fromSymbol(const Symbol symbol) const {


I think we should be able to return some sort of reference type here so we don't create a fresh temp string that will just be used to concat into a larger string.

mrice32 · 2018-07-23T17:53:43Z

source/common/stats/symbol_table_impl.h

+    if (encode_search != encode_map_.end()) {
+      // If the symbol exists. Return it and up its refcount.
+      result = encode_search->second.first;
+      (encode_search->second.second)++;


Tiny tiny note, it's generally preferred to use pre-increment (++x) as opposed to post-increment (x++) because, depending on the context, you may avoid the creation of a temporary variable. Here's a post explaining why it's different: https://stackoverflow.com/a/24904.

Specifically, this only comes up when you don't care about the return value. Clearly, if you do care about whether the return value is incremented, then that will dictate which one you use.

Thanks, this makes sense -- I'll switch these to ++x.

mrice32 · 2018-07-23T17:57:47Z

source/common/stats/symbol_table_impl.h

+  // 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_ = {};
+  std::unordered_map<Symbol, std::string> decode_map_ = {};


Is there a reason you added the = {};? It should have the same behavior without that portion, right?

Yep, this is left over from when I was initializing these maps with values. I'll remove this.

mrice32 · 2018-07-23T18:10:30Z

source/common/stats/symbol_table_impl.h

+  Symbol toSymbol(const std::string& str) {
+    Symbol result;
+    auto encode_search = encode_map_.find(str);
+    if (encode_search != encode_map_.end()) {


I think you can refactor this to make it a bit cleaner:

auto encode_search = encode_map_.find(str); if (encode_search == encode_map_.end()) { // initialization encode_search = encode_map_.insert({str, std::make_pair(curr_counter_, 0)}).first; decode_map_.insert({curr_counter_++, str}); } // May need parens after the ++ on the line below. ++encode_search->second.second; return encode_search->second.first;

I usually try to optimize for doing only one map operation. Do an insert and then use the returned bool to see whether something new got inserted or you are bumping the ref-count of the already-existing entry.

+1 - SGTM. I think that will be a little more complex, but it would probably yield perf gains.

mrice32 · 2018-07-23T18:16:58Z

source/common/stats/symbol_table_impl.h

+    std::vector<std::string> name_vec = absl::StrSplit(name, '.');
+    symbol_vec.resize(name_vec.size());
+    std::transform(name_vec.begin(), name_vec.end(), symbol_vec.begin(),
+                   [this](std::string x) { return this->toSymbol(x); });


I don't think the this is necessary in this->toSymbol(x);. Same for the similar call below.

Thanks, done.

mrice32 · 2018-07-23T18:22:29Z

source/common/stats/symbol_table_impl.h

+public:
+  std::vector<Symbol> encode(const std::string& name) override {
+    std::vector<Symbol> symbol_vec;
+    std::vector<std::string> name_vec = absl::StrSplit(name, '.');


Ideally, I'd like to get rid of the intermediate vector here and just generate each substring and pass it to toSymbol in one loop similar to the one below. But as a first step, can we use string_views instead of allocating full strings? The absl::string_view class is perfect for taking substrings from an existing string that you know will exist for the duration of the call, so this seems like the ideal way to pass substrings to toSymbol().

I think using string_view in this way isn't going to work -- absl::string_view doesn't implement hashable, so keeping an std::unordered_map<absl::string_view, ...> would require... a custom hasher which converts the string_view back into a string? Sadly I think we're stuck storing the full string in two locations.

A custom hasher == "not a barrier" :)

It's pretty straightforward to do and definitely worth it to avoid extra allocations IMO.

Won't this end up just being the cost of converting each string_view back into a string and taking the hash of that? Or is there a better way to write that custom hasher?

No, see:

envoy/source/common/stats/stats_impl.h

Line 174 in e89c9d6

std::unordered_map<absl::string_view, std::vector<TagExtractorPtr>, StringViewHash>

and

envoy/source/common/common/utility.h

Line 490 in 3bf15a3

struct StringViewHash {

I think this would undo the performance benefits of only having to perform one map operation on encode_map (from elsewhere in this thread).

Since we store two maps:

encode_map_ <std::string, std::pair<Symbol, uint32_t>>; decode_map_ <Symbol, std::string>;

we could replace the key in encode_map_ with a string_view, but we would still have to store a full string as the value in the decode_map_, since we don't want to rely on the continued lifetime of some string outside of the SymbolTable. This would mean that the key in encode_map_ should be a string_view into the value in the decode_map_, which means that we have to perform a .find() on the encode_map before the .insert() into the encode_map_.

jmarantz · 2018-07-23T15:29:22Z

include/envoy/stats/symbol_table.h

+  /**
+   * Decodes a vector of symbols back into its period-delimited stat name.
+   * If decoding fails on any part of the symbol_vec, that symbol will be decoded to the empty
+   * string ("").


can we return an error code instead? Maybe as a pair?

I think this should actually be instantly crashing with a release_assert upon trying to decode a bad symbol, since this should really never happen. Maybe I also want to add a release_assert that the counter is never more than maxint.

an assert or release_assert sounds good.

Done. I'll implement this for free() too, since I think it implies something similarly pathological.

jmarantz · 2018-07-23T15:30:27Z

include/envoy/stats/symbol_table.h

+ * Guarantees that x = decode(encode(x)) for any x.
+ *
+ * Even though the symbol table does manual reference counting, curr_counter_ is monotonically
+ * increasing. So encoding "foo", freeing the sole stat containing "foo", and then re-encoding "foo"


I think a long running server may never have more than 4G symbols, but I'm not sure we wouldn't wrap over the course of a long run with lots of config changes. Would it be hard to keep a free-list and recycle?

I also feel like it it makes things slightly more efficient then to use a vector rather than a map for the int-to-string mapping, but I don't really care that much.

If you have 100 clusters updated every 1s, this is 497 days. We probably have less frequent cluster updates but some deployments will be larger. I don't mind the idea of a free list to avoid this. I'm not fully sold on efficiency here of vector vs. map, it's a time-space tradeoff.

jmarantz · 2018-07-23T19:43:51Z

source/common/stats/symbol_table_impl.h

+  std::string decode(const std::vector<Symbol>& symbol_vec) const override {
+    std::vector<std::string> name;
+    name.resize(symbol_vec.size());
+    std::transform(symbol_vec.begin(), symbol_vec.end(), name.begin(),


If we care we should micro-benchmark, but keep in mind that StrJoin will right-size the resulting string-array ahead of time, which is better than concatenating iteratively.

jmarantz · 2018-07-23T19:44:26Z

source/common/stats/symbol_table_impl.h

+    for (const Symbol symbol : symbol_vec) {
+      successful_free = successful_free && freeSymbol(symbol);
+    }
+    return successful_free;


I think we should architect this to avoid needing a free(). I thought this should all be automatic by reference-counting.

I would prefer this approach as well. You can build an object to wrap the vector that does the free in its destructor.

See my above point on RAII and SharedPtr.

jmarantz · 2018-07-23T19:45:38Z

source/common/stats/symbol_table_impl.h

+  Symbol toSymbol(const std::string& str) {
+    Symbol result;
+    auto encode_search = encode_map_.find(str);
+    if (encode_search != encode_map_.end()) {


I usually try to optimize for doing only one map operation. Do an insert and then use the returned bool to see whether something new got inserted or you are bumping the ref-count of the already-existing entry.

htuch

Awesome to see this happening, this will be fantastic for reducing memory as we scale to large numbers of clusters.

htuch · 2018-07-24T03:04:54Z

include/envoy/stats/symbol_table.h

+ */
+class SymbolTable {
+public:
+  typedef uint32_t Symbol;


Nit: Some slight preference for using over typedef here.

htuch · 2018-07-24T03:09:11Z

include/envoy/stats/symbol_table.h

+ * Guarantees that x = decode(encode(x)) for any x.
+ *
+ * Even though the symbol table does manual reference counting, curr_counter_ is monotonically
+ * increasing. So encoding "foo", freeing the sole stat containing "foo", and then re-encoding "foo"


If you have 100 clusters updated every 1s, this is 497 days. We probably have less frequent cluster updates but some deployments will be larger. I don't mind the idea of a free list to avoid this. I'm not fully sold on efficiency here of vector vs. map, it's a time-space tradeoff.

htuch · 2018-07-24T03:10:12Z

include/envoy/stats/symbol_table.h

+   * @param name the stat name to encode.
+   * @return std::vector<Symbol> the encoded stat name.
+   */
+  virtual std::vector<Symbol> encode(const std::string& name) PURE;


Why is the fact that this is a vector of symbols made public to the world? Can we use an opaque type here to support multiple implementations (and later performance tuning)?

This makes sense -- I think the right way to do this is to

typedef uint32_t Symbol; typedef std::vector<Symbol> StatName;

inside the abstract interface, and then pass SymbolTable::StatName everywhere.

Oops, with using as above.

If you want to keep the fact that the rep is a vector an implementation detail, it should not go into the interface.

I think you want something like:

class SymbolizedString { public: SymbolizedString() {} virtual ~SymbolizedString(); virtual std::string() toString() const PURE; }; class SymbolizedStringImpl : public SymbolizedString { public: ~SymbolizedStringImpl() override { ... decrement ref count on all symbols via SymbolTable_; } std::string() toString() const override; private: std::vector<Symbol> symbols_; SymbolTable& symbol_table_; };

Done. We pass back a std::unique_ptr for this which has free() inside its destructor.

htuch · 2018-07-24T03:11:51Z

include/envoy/stats/symbol_table.h

+   *
+   * @return bool whether or not the total free operation was successful. Expected to be true.
+   */
+  virtual bool free(const std::vector<Symbol>& symbol_vec) PURE;


Why not make the returned value from encode a SharedPtr and then have RAII free semantics? I think this would be the modern C++ way. Also, I think there is too much implementation discussion in the above header comment for an abstract interface.

it'd be interesting to see you push the changes around the time you say "done", as I'm curious what direction you are going in ... even if it's not totally finished or passing format tests yet.

htuch · 2018-07-24T03:12:40Z

source/common/stats/symbol_table_impl.h

+public:
+  std::vector<Symbol> encode(const std::string& name) override {
+    std::vector<Symbol> symbol_vec;
+    std::vector<std::string> name_vec = absl::StrSplit(name, '.');


Just leave a TODO for now I reckon.

htuch · 2018-07-24T03:13:57Z

source/common/stats/symbol_table_impl.h

+    for (const Symbol symbol : symbol_vec) {
+      successful_free = successful_free && freeSymbol(symbol);
+    }
+    return successful_free;


See my above point on RAII and SharedPtr.

htuch · 2018-07-24T03:20:06Z

source/common/stats/symbol_table_impl.h

+
+  // 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_ = {};


I'm thinking we need a better story here for how we will solve for shared memory, unless we want to make hot restart mutually exclusive with scalable stats (and scalable clusters). Istio, for example, sitll uses hot restart and wants to scale to very large cluster sizes. @mattklein123 @rshriram.

jmarantz · 2018-07-24T16:27:06Z

source/common/stats/symbol_table_impl.h

+    return (search != decode_map_.end()) ? (search->second) : "";
+  }
+
+  // Returns true if the free was successful, false if the symbol was invalid.


should we just assert-fail if the symbol was invalid?

jmarantz · 2018-07-24T16:27:43Z

source/common/stats/symbol_table_impl.h

+    }
+    ((encode_search->second).second)--;
+    if ((encode_search->second).second == 0) {
+      decode_map_.erase(symbol);


erase the iterator, rather than the key, to avoid the extra map search.

Signed-off-by: James Buckland <[email protected]>

ambuc · 2018-07-24T18:59:15Z

I just pushed fixes to a fair number of the comments above, but have not yet addressed:

memory usage in shared memory (open question)
a free pool for reuse of Symbols
using more efficient strings or string_views in the SymbolTableImpl
writing a proper test for .decode() ing an invalid SymbolVec

jmarantz · 2018-07-24T19:50:25Z

source/common/stats/symbol_table_impl.cc

+    auto decode_search = decode_map_.find(symbol);
+    RELEASE_ASSERT(decode_search != decode_map_.end(), "");
+
+    std::string str = decode_search->second;


const std::string&

jmarantz · 2018-07-24T19:58:54Z

include/envoy/stats/symbol_table.h

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


I think we don't need to -- and should not -- expose this API or the Symbol type in the interface.

What I think you can do is exploit a C++ feature called something covariant returns (https://stackoverflow.com/questions/1260757/when-is-c-covariance-the-best-solution)

So SymbolTableImpl::encode() can return a StatNameImpl*, even though SymbolTable::encode() returns a StatName*. I'm a little hazy on whether this works with std::unique_ptr though, so that needs to be experimented.

But the higher level point is that users of this class should never be working with SymbolVec, and shouldn't see those in the API.

Maybe we just forward declare SymbolEncoding or whatevs and then hide the implementation? Whatever it takes to ensure that this is completely opaque.

I still want a toSymbols() equivalent fn for testing, see b8b3f60.

jmarantz · 2018-07-24T19:59:44Z

source/common/stats/symbol_table_impl.cc

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


absl::string_view all the way down :)

jmarantz · 2018-07-24T20:01:57Z

source/common/stats/symbol_table_impl.h

+  StatNamePtr encode(const std::string& name) override;
+
+  // For testing purposes only.
+  size_t size() const { return sizeof(SymbolTableImpl) + encode_map_.size() + decode_map_.size(); }


size() seems like a useful and idiomatic API to include, but I would expect it to be the number of symbols.

It's not entirely clear to me what this particular impl is returning though; you should doc that. the first sizeof returns a number of bytes, and the map size() calls return cardinalities which need to be multiplied by something to become bytes.

htuch · 2018-07-24T23:28:34Z

include/envoy/stats/symbol_table.h

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


Maybe we just forward declare SymbolEncoding or whatevs and then hide the implementation? Whatever it takes to ensure that this is completely opaque.

htuch · 2018-07-24T23:29:32Z

include/envoy/stats/symbol_table.h

+  virtual SymbolVec toSymbols() const PURE;
+};
+
+using StatNamePtr = std::unique_ptr<StatName>;


Should this be SharedPtr to support ref counted sharing?

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

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.

htuch · 2018-07-24T23:30:45Z

source/common/stats/symbol_table_impl.h

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


htuch · 2018-07-24T23:32:37Z

source/common/stats/symbol_table_impl.h

+   * 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.
+   *


Nit: @param

htuch · 2018-07-24T23:32:58Z

source/common/stats/symbol_table_impl.h

+   */
+  void free(const SymbolVec& symbol_vec);
+
+  Symbol curr_counter_ = 0;


Nit: counter_{};

htuch · 2018-07-24T23:33:21Z

source/common/stats/symbol_table_impl.h

+  std::unordered_map<std::string, std::pair<Symbol, uint32_t>> encode_map_;
+  std::unordered_map<Symbol, std::string> decode_map_;
+
+  Symbol toSymbol(const std::string& str);


Nit: method decls before fields.

htuch · 2018-07-24T23:34:31Z

source/common/stats/symbol_table_impl.h

+namespace Stats {
+
+class SymbolTableImpl : public SymbolTable {
+  friend class StatNameImpl;


I would put this at the top of private:

Signed-off-by: James Buckland <[email protected]>

jmarantz

This is really awesome. There are a large number of fairly trivial comments, but this is basically spot on. Nice job!

jmarantz · 2018-07-26T02:37:07Z

include/envoy/stats/symbol_table.h

+namespace Stats {
+
+using Symbol = uint32_t;
+using SymbolVec = std::vector<Symbol>;


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

jmarantz · 2018-07-26T02:37:38Z

source/common/stats/symbol_table_impl.cc

+// if we can help it.
+StatNamePtr SymbolTableImpl::encode(const std::string& name) {
+  SymbolVec symbol_vec;
+  std::vector<std::string> name_vec = absl::StrSplit(name, '.');


absl::string_view

jmarantz · 2018-07-26T02:38:03Z

source/common/stats/symbol_table_impl.cc

+  std::vector<std::string> name_vec = absl::StrSplit(name, '.');
+  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); });


absl::string_view

jmarantz · 2018-07-26T02:39:00Z

source/common/stats/symbol_table_impl.cc

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


absl::string_view

jmarantz · 2018-07-26T02:39:20Z

source/common/stats/symbol_table_impl.cc

+  return result;
+}
+
+std::string SymbolTableImpl::fromSymbol(const Symbol symbol) const {


absl::string_view

See 3687311.

jmarantz · 2018-07-26T02:52:07Z

test/common/stats/symbol_table_test.cc

+namespace Stats {
+
+class StatNameTest : public testing::Test {
+public:


jmarantz · 2018-07-26T02:52:21Z

test/common/stats/symbol_table_test.cc

+class StatNameTest : public testing::Test {
+public:
+  StatNameTest() {}
+  SymbolTableImpl table_;


move this to the bottom of the class.

jmarantz · 2018-07-26T02:54:23Z

source/common/stats/symbol_table_impl.h

+ */
+class SymbolTableImpl : public SymbolTable {
+public:
+  StatNamePtr encode(const std::string& name) override;


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.

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.

jmarantz · 2018-07-26T02:57:46Z

source/common/stats/symbol_table_impl.h

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


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

jmarantz · 2018-07-26T02:59:00Z

test/common/stats/symbol_table_test.cc

+}
+
+TEST_F(StatNameTest, TestUnusualDelimitersRoundtrip) {
+  std::vector<std::string> stat_names = {".",    "..",    "...",    "foo",    "foo.",


Signed-off-by: James Buckland <[email protected]>

stale · 2018-08-14T14:08:40Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

ambuc · 2018-08-14T14:14:09Z

So far my suspicion is that Amdahl's law is capping our savings here. Improving the individual performance of storing a string in memory pales in comparison to the need to store that string multiple times in the unordered maps inside ThreadLocalStore, ScopeImpl, etc. I'm still playing with ways to use a StatNamePtr as a key in these maps, or perhaps using string_views instead of strings (where the original string is assured to exist for the lifetime of the string_view.

TL;DR: Still working on this. Sorry, @stalebot.

Signed-off-by: James Buckland <[email protected]>

mattklein123 · 2018-08-14T15:57:30Z

@ambuc FWIW that was my intuition is to why this might not work out (string size vs. cost of all the maps) which is why I asked for the extra diligence. Thank you for working on this and hopefully we can find a good solution!

Signed-off-by: James Buckland <[email protected]>

stale · 2018-08-21T16:25:47Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: James Buckland <[email protected]>

ambuc · 2018-08-27T20:04:53Z

@mattklein123 @htuch @jmarantz @mrice32 I'm ready to present an implementation which uses this symbol table to encode strings within HeapStatData and ThreadLocalStore. Here are some preliminary findings w/r/t heap storage improvement.

As always, I have run these trials against configs with 10, 100, 1000, 10000 clusters, for configs with both short (~10 character) cluster names, and long (~60 character) cluster names. (All runs are against envoy-static with --disable-hot-restart.)

		short	short	long	long
		current impl	w/ symbol table	current impl	w/ symbol table	improvement	improvement
clusters	runtime	allocated on heap	allocated on heap	allocated on heap	allocated on heap	short	long
(#)	(sec)	(kB)	(kB)	(kB)	(kB)	(%)	(%)
10	30	2484	2481	2637	2564	-0.15%	-2.77%
100	45	7996	8008	9552	8854	0.14%	-7.31%
1000	60	63145	63158	78793	71702	0.02%	-9.00%
10000	90	3566177	3480433	5509859	4420003	-2.40%	-19.78%

You can see this implementation as a PR against my own fork of Envoy. (ambuc#5).

How should we proceed -- should I commit these changes to this branch? Or should we merge this branch into master, and then I will take out another PR for review against master with those changes?

mrice32 · 2018-08-27T20:15:11Z

Nice work! Did you change the underlying symbol table implementation in the other PR? If not, I would prefer that we just go ahead and merge this to keep the PRs small and easy to review.

ambuc · 2018-08-27T20:17:27Z

@mrice32 Yes, I added hash() and operator==() functions for StatNames, and a mutex within the SymbolTableImpl. But these are not fundamental changes, and are more obviously necessary in that context than in this one.

mrice32 · 2018-08-27T20:26:06Z

@ambuc either way is fine with me, but I slightly prefer keeping them split and adding those changes in the subsequent PR where they're used and implicitly tested for ease of review.

jmarantz · 2018-08-28T13:27:25Z

This is great work. I do have a question about the table: what's the meaning of the 'runtime' column? Does I was expecting that for a fixed amount of work, the runtimes might be slightly different between the symbol-table and non-symbol-table versions;.

ambuc · 2018-08-28T13:29:00Z

@jmarantz the runtime is how long I ran the trial for. For trials with small numbers of clusters, the entire Envoy spun up and then sat quietly fairly quickly -- for larger numbers of clusters, it took more time to initialize. That said, I agree that symbol-table trials probably took less time to initialize.

htuch

Makes sense to me, no substantive changes since last LGTM, so let's merge.

Pulling the following changes from github.com/envoyproxy/envoy: f936fc6 ssl: serialize accesses to SSL socket factory contexts (envoyproxy#4345) e34dcd6 Fix crash in tcp_proxy (envoyproxy#4323) ae6a252 router: fix matching when all domains have wildcards (envoyproxy#4326) aa06142 test: Stop fake_upstream methods from accidentally succeeding (envoyproxy#4232) 5d73187 rbac: update the authenticated.user to a StringMatcher. (envoyproxy#4250) c6bfc7d time: Event::TimeSystem abstraction to make it feasible to inject time with simulated timers (envoyproxy#4257) 752483e Fixing the fix (envoyproxy#4333) 83487f6 tls: update BoringSSL to ab36a84b (3497). (envoyproxy#4338) 7bc210e test: fixing interactions between waitFor and ignore_spurious_events (envoyproxy#4309) 69474b3 admin: order stats in clusters json admin (envoyproxy#4306) 2d155f9 ppc64le build (envoyproxy#4183) 07efc6d fix static initialization fiasco problem (envoyproxy#4314) 0b7e3b5 test: Remove declared but undefined class methods (envoyproxy#4297) 1485a13 lua: make sure resetting dynamic metadata wrapper when request info is marked dead d243cd6 test: set to zero when start_time exceeds limit (envoyproxy#4328) 0a1e92a test: fix heap use-after-free in ~IntegrationTestServer. (envoyproxy#4319) cddc732 CONTRIBUTING: Document 'kick-ci' trick. (envoyproxy#4335) f13ef24 docs: remove reference to deprecated value field (envoyproxy#4322) e947a27 router: minor doc fixes in stream idle timeout (envoyproxy#4329) 0c2e998 tcp-proxy: fixing a TCP proxy bug where we attempted to readDisable a closed connection (envoyproxy#4296) 00ffe44 utility: fix strftime overflow handling. (envoyproxy#4321) af1183c Re-enable TcpProxySslIntegrationTest and make the tests pass again. (envoyproxy#4318) 3553461 fuzz: fix H2 codec fuzzer post envoyproxy#4262. (envoyproxy#4311) 42f6048 Proto string issue fix (envoyproxy#4320) 9c492a0 Support Envoy to fetch secrets using SDS service. (envoyproxy#4256) a857219 ratelimit: revert `revert rate limit failure mode config` and add tests (envoyproxy#4303) 1d34172 dns: fix exception unsafe behavior in c-ares callbacks. (envoyproxy#4307) 1212423 alts: add gRPC TSI socket (envoyproxy#4153) f0363ae fuzz: detect client-side resets in H2 codec fuzzer. (envoyproxy#4300) 01aa3f8 test: hopefully deflaking echo integration test (envoyproxy#4304) 1fc0f4b ratelimit: link legacy proto when message is being used (envoyproxy#4308) aa4481e fix rare List::remove(&target) segfault (envoyproxy#4244) 89e0f23 headers: fixing fast fail of size-validation (envoyproxy#4269) 97eba59 build: bump googletest version. (envoyproxy#4293) 0057e22 fuzz: avoid false positives in HCM fuzzer. (envoyproxy#4262) 9d094e5 Revert ac0bd74 (envoyproxy#4295) ddb28a4 Add validation context provider (envoyproxy#4264) 3b47cba added histogram latency information to Hystrix dashboard stream (envoyproxy#3986) cf87d50 docs: update SNI FAQ. (envoyproxy#4285) f952033 config: fix update empty stat for eds (envoyproxy#4276) 329e591 router: Add ability of custom headers to rely on per-request data (envoyproxy#4219) 68d20b4 thrift: refactor build files and imports (envoyproxy#4271) 5fa8192 access_log: log requested_server_name in tcp proxy (envoyproxy#4144) fa45bb4 fuzz: libc++ clocks don't like nanos. (envoyproxy#4282) 53f8944 stats: add symbol table for future stat name encoding (envoyproxy#3927) c987b42 test infra: Remove timeSource() from the ClusterManager api (envoyproxy#4247) cd171d9 websocket: tunneling websockets (and upgrades in general) over H2 (envoyproxy#4188) b9dc5d9 router: disallow :path/host rewriting in request_headers_to_add. (envoyproxy#4220) 0c91011 network: skip socket options and source address for UDS client connections (envoyproxy#4252) da1857d build: fixing a downstream compile error by noting explicit fallthrough (envoyproxy#4265) 9857cfe fuzz: cleanup per-test environment after each fuzz case. (envoyproxy#4253) 52beb06 test: Wrap proto string in std::string before comparison (envoyproxy#4238) f5e219e extensions/thrift_proxy: Add header matching to thrift router (envoyproxy#4239) c9ce5d2 fuzz: track read_disable_count bidirectionally in codec_impl_fuzz_test. (envoyproxy#4260) 35103b3 fuzz: use nanoseconds for SystemTime in RequestInfo. (envoyproxy#4255) ba6ba98 fuzz: make runtime root hermetic in server_fuzz_test. (envoyproxy#4258) b0a9014 time: Add 'format' test to ensure no one directly instantiates Prod*Time from source. (envoyproxy#4248) 8567460 access_log: support beginning of epoch in START_TIME. (envoyproxy#4254) 28d5f41 proto: unify envoy_proto_library/api_proto_library. (envoyproxy#4233) f7d3cb6 http: fix allocation bug introduced in envoyproxy#4211. (envoyproxy#4245) Fixes istio/istio#8310 (once pulled into istio/istio). Signed-off-by: Piotr Sikora <[email protected]>

ambuc added 4 commits July 23, 2018 08:59

WIP Working symbol table with manual reference counting

8de2463

Signed-off-by: James Buckland <[email protected]>

Added tests, better error handling for null symbol lookups

1c45948

Signed-off-by: James Buckland <[email protected]>

Fix formatting

a3df93b

Signed-off-by: James Buckland <[email protected]>

Add common/pure header

a78ea8d

Signed-off-by: James Buckland <[email protected]>

ambuc mentioned this pull request Jul 23, 2018

Stats memory usage can probably be improved quite a bit #3585

Closed

mrice32 reviewed Jul 23, 2018

View reviewed changes

htuch self-assigned this Jul 23, 2018

mrice32 reviewed Jul 23, 2018

View reviewed changes

jmarantz reviewed Jul 23, 2018

View reviewed changes

htuch suggested changes Jul 24, 2018

View reviewed changes

jmarantz reviewed Jul 24, 2018

View reviewed changes

ambuc added 4 commits July 24, 2018 14:56

Add new StatNameImpl type for encapsulating the SymbolVec

59e8129

Signed-off-by: James Buckland <[email protected]>

Move the bulk of the logic to a .cc

475dcd3

Signed-off-by: James Buckland <[email protected]>

Update tests

9b3594b

Signed-off-by: James Buckland <[email protected]>

Add the SymbolTableImpl .cc

e7b619a

Signed-off-by: James Buckland <[email protected]>

jmarantz reviewed Jul 24, 2018

View reviewed changes

htuch reviewed Jul 24, 2018

View reviewed changes

ambuc added 9 commits July 25, 2018 10:58

Mark internal string as const ref

b3f1b7f

Signed-off-by: James Buckland <[email protected]>

More documentation around size()

bdc714c

Signed-off-by: James Buckland <[email protected]>

Decls before fields

0ded3e5

Signed-off-by: James Buckland <[email protected]>

Hide toSymbols() fn from public API

b8b3f60

Signed-off-by: James Buckland <[email protected]>

Add proper test for death on the decode of an invalid symbol

ae650c3

Signed-off-by: James Buckland <[email protected]>

Added documentation for toSymbol() / fromSymbol()

14e8bb3

Signed-off-by: James Buckland <[email protected]>

Add free pool for symbol re-use

86dc207

Signed-off-by: James Buckland <[email protected]>

Merge remote-tracking branch 'upstream/master' into symbol-table

cb6c754

Merge remote-tracking branch 'upstream/master' into symbol-table

ff30317

jmarantz reviewed Jul 26, 2018

View reviewed changes

Various nits and docs fixes

6e42c86

Signed-off-by: James Buckland <[email protected]>

ambuc dismissed htuch’s stale review via 19c6e65 August 1, 2018 19:47

ambuc added 2 commits August 1, 2018 16:29

Merge remote-tracking branch 'upstream/master' into symbol-table

da2b7d7

Signed-off-by: James Buckland <[email protected]>

Merge remote-tracking branch 'upstream/master' into symbol-table

9466aa3

Signed-off-by: James Buckland <[email protected]>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 14, 2018

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 14, 2018

ambuc added 2 commits August 14, 2018 10:17

Merge remote-tracking branch 'upstream/master' into symbol-table

86dab2c

Signed-off-by: James Buckland <[email protected]>

Merge remote-tracking branch 'upstream/master' into symbol-table

208512d

Signed-off-by: James Buckland <[email protected]>

Merge remote-tracking branch 'upstream/master' into symbol-table

27aa402

Signed-off-by: James Buckland <[email protected]>

PiotrSikora mentioned this pull request Aug 21, 2018

Per listener and per cluster memory overhead is too high #4196

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 21, 2018

Merge remote-tracking branch 'upstream/master' into symbol-table

aaba51b

Signed-off-by: James Buckland <[email protected]>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 21, 2018

ambuc added 3 commits August 24, 2018 10:46

Merge remote-tracking branch 'upstream/master' into symbol-table

278f92f

Signed-off-by: James Buckland <[email protected]>

Merge remote-tracking branch 'upstream/master' into symbol-table

ce87e7e

Signed-off-by: James Buckland <[email protected]>

Merge remote-tracking branch 'upstream/master' into symbol-table

efd3c6f

Signed-off-by: James Buckland <[email protected]>

htuch approved these changes Aug 28, 2018

View reviewed changes

htuch merged commit 53f8944 into envoyproxy:master Aug 28, 2018

ambuc mentioned this pull request Aug 28, 2018

stats: symbolize strings in HeapStatData and ThreadLocalStore #4281

Closed

ambuc deleted the symbol-table branch August 28, 2018 17:57

stats: add symbol table for future stat name encoding #3927

stats: add symbol table for future stat name encoding #3927

Conversation

ambuc commented Jul 23, 2018

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 Jul 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ambuc Jul 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrice32 Jul 23, 2018 •

edited

Loading

ambuc Jul 24, 2018 •

edited

Loading