Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
50 changes: 50 additions & 0 deletions include/envoy/stats/symbol_table.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#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;
virtual SymbolVec toSymbols() const PURE;

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

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.

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

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 still want a toSymbols() equivalent fn for testing, see b8b3f60.

};

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

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

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.

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.

* will produce a different symbol each time.
*/
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;
};

} // 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
76 changes: 76 additions & 0 deletions source/common/stats/symbol_table_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
#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.


std::string str = decode_search->second;

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.

const std::string&

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 ((encode_search->second).second == 0) {
decode_map_.erase(decode_search);
encode_map_.erase(encode_search);
}
}
}

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(curr_counter_, 1)});
// If the insertion took place, we mirror the insertion in the decode_map.
if (encode_insert.second) {
auto decode_insert = decode_map_.insert({curr_counter_, 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 = curr_counter_;
++curr_counter_;
} 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
78 changes: 78 additions & 0 deletions source/common/stats/symbol_table_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
#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 {

class SymbolTableImpl : public SymbolTable {
friend class StatNameImpl;

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.

I would put this at the top of private:


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 { return sizeof(SymbolTableImpl) + encode_map_.size() + decode_map_.size(); }

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.

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.


private:
/**
* 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

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

Symbol curr_counter_ = 0;

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: counter_{};


// 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_;

Symbol toSymbol(const std::string& str);

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: method decls before fields.

std::string fromSymbol(const Symbol symbol) const;
};

/**
* 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_); }
SymbolVec toSymbols() const override { return symbol_vec_; }

private:
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