Speedups to FakeSymbolTableTest based on microbenchmarks from #6161.#6293
Speedups to FakeSymbolTableTest based on microbenchmarks from #6161.#6293jmarantz merged 15 commits intoenvoyproxy:masterfrom
Conversation
…oxy#6161. Signed-off-by: Joshua Marantz <jmarantz@google.com>
…ternals to friendds. Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
ambuc
left a comment
ambuc
left a comment
There was a problem hiding this comment.
I have a lot of very small nits and questions but overall this LGTM.
My main issue is that this PR adds a bit of temporary complexity without a manifest for which of the new methods get removed and when. If you have github issues for the future work, please add TODO(issue/<num>) or TODO(jmarantz): Remove after issue/num comments inline.
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
/retest |
|
🔨 rebuilding |
|
/retest |
|
🔨 rebuilding |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
@mattklein123 I think this is ready for a look. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
@zuercher would you have time to look at this? Nothing too dramatic in here I think. This lacks a cross-company review so I don't want to ask Harvey or Alyssa. |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Looks good from a high level skim. A couple of small comments. Thank you!
/wait
include/envoy/stats/symbol_table.h
Outdated
| * @param num_names The number of names. | ||
| * @param symbol_table The symbol table in which to encode the names. | ||
| */ | ||
| virtual void populateList(absl::string_view* names, int32_t num_names, StatNameList& list) PURE; |
There was a problem hiding this comment.
nit: s/int32_t/uint32_t
Where is this storage coming from? Do we need to use a pointer/length vs. using std:array/std::vector or something like that?
include/envoy/stats/symbol_table.h
Outdated
| * must manually call free(symbol_vec) when it is freeing the backing store | ||
| * for a StatName. This way, the symbol table will grow and shrink | ||
| * dynamically, instead of being write-only. | ||
| * dynamically, instead of being write-only |
Signed-off-by: Joshua Marantz <jmarantz@google.com>
Signed-off-by: Joshua Marantz <jmarantz@google.com>
|
/retest |
|
🔨 rebuilding |
4 participants
Description: Pulls out a suite of improvements to FakeSymbolTable from #6161 -- that PR has grown too big to and this one of a series of logical partitions that can be broken out and reviewed. #6161 itself was intended as a less risky stepping stone to #4980 which changes the base representation fo all stat names to use a symbol table.
Risk Level: low currently -- this is not used yet, but in another PR it soon will be.
Testing: //test/common/stats/...
Docs Changes: n/a
Release Notes: n/a