stats: cache the results of the stats-matcher class.#6207
stats: cache the results of the stats-matcher class.#6207jmarantz merged 19 commits intoenvoyproxy:masterfrom
Conversation
2 reasons to do this: - avoid doing regex matches on hot-path - set the stage for stats symbol tables, where we don't have the elaborated strings to send into the matcher, without taking a global symbol table lock. The disadvantage is that we'll dull some of the benefit provided by stats-matcher, since there we'll have to retain the elaborated names of all rejected stats. That's still significantly better than retaining the complete stats, including multiple copies of the names, map entries, tags and tag-extracted names. And once we've converted to symbol tables, the cost of the maps of rejected stat names will be signficantly cheaper. Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
…ThreadLocalStore object, and out of the scope-based central cache. Signed-off-by: Joshua Marantz <[email protected]>
ambuc
left a comment
ambuc
left a comment
There was a problem hiding this comment.
I know you're just looking for a first round / outsourcing your CI to upstream. Just a few comments inline.
I think the elaborated reasons make sense, but I'm worried about unbounded growth for generated stat names. Is there a way to make the rejected stats set LRU?
… that it's easier to reason about behavior with moves. Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
|
I think your point about unbounded growth is a good one. The main issue is if scopes disappear due to dynamic config. We'll keep the strings and the references to them in the TLS caches. This is also a pre-existing issue for stats I think. Once they're in the TLS cache as shared_ptr, they will never disappear until a hot restart, IIUC. So my inclination is that this is still better than making stats for the rejected names. And the extra cost will become much smaller once symbol tables are integrated. |
…ore sets. Signed-off-by: Joshua Marantz <[email protected]>
jmarantz
changed the title
|
OK I think this is ready for review. |
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
ambuc
left a comment
ambuc
left a comment
There was a problem hiding this comment.
LGTM. Just a few nits about maybe placing shared utilities in /common.
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Cool stuff. I want to take another pass, but 1 question and 1 comment. Thanks.
/wait
| // as a absl::flat_hash_map<char*>, so the keys will be stable and safe | ||
| // to reference from other threads even as the hash-map resizes. | ||
| // | ||
| // Note that once a stat or rejected name enters into the TLS Cache, it |
There was a problem hiding this comment.
Can we upgrade this to a TODO? IMO I would consider this a bug for a long running Envoy? WDYT?
source/common/stats/char_star_set.cc
Outdated
| StringUtil::strlcpy(ptr, str.data(), bytes); | ||
| auto insertion = hash_set_.insert(ptr); | ||
| if (!insertion.second) { | ||
| delete[] ptr; |
There was a problem hiding this comment.
Couldn't we have the hash key be a unique_ptr<char[]> which would greatly simplify a bunch of the memory management in this class?
There was a problem hiding this comment.
I think that making the set key be a std::unique_ptr<char[]> would simplify some things but would complicate the implementation of find(), which would need to construct a unique_ptr object from a const char* it doesn't own in order to do the set lookup. That seems worse than the current state.
However I did change the destructor and insert() to use a std::unique_ptr as a temp, and I think that does help a little -- reduces lines of code anyway. PTAL. I could go either way. Or if you have a more specific suggestion about how to implement this interface more cleanly I'm fine with that too.
Signed-off-by: Joshua Marantz <[email protected]>
…management in insert() and dtor. Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
…ce tactic for the stats themselves. Signed-off-by: Joshua Marantz <[email protected]>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Thanks like this implementation a lot better, 1 question.
/wait-any
Signed-off-by: Joshua Marantz <[email protected]>
…*, SharedString>, saving memory. Signed-off-by: Joshua Marantz <[email protected]>
3 participants
Description: 2 reasons to do this:
The disadvantage is that we'll dull some of the benefit provided by stats-matcher, by retaining the names of all rejected stats. That's still significantly better than retaining the complete stats, including multiple copies of the names, map entries, tags and tag-extracted names. And once we've converted to symbol tables, the cost of the maps of rejected stat names will be significantly cheaper.
Note that disallowing all stats will not affect memory use -- that's special-cased and short-circuited.
Risk Level: medium in general, and in particular there's a risk of temporarily increased memory usage with a matcher expression that eliminates large numbers of stats. The memory usage increase will be reduced significantly once symbol table integration is complete.
Testing: test/common/stats/... -- my local build environment stopped working for //test/... due to a luajit issue so I want to see what CI does.
Docs Changes: n/a -- though maybe I should update doc to indicate regexes would no longer be a dumpster fire of perf after this.
Release Notes: n/a