stats: Use SymbolTable API for creating and representing stat names. by jmarantz · Pull Request #6161 · envoyproxy/envoy

jmarantz · 2019-03-04T20:12:07Z

Description: This PR takes a significant step toward resolving #3585 and #4196 by integrating the symbol table API natively into heap-based stats. There is no real symbol-table live in the code yet. The benefit of this PR is that it allows us to begin a series of PRs throughout the codebase to use the symbol-table API to lookup/create stats rather than raw strings.

In this PR we use a fake symbol table implementation, which reduces the functional risk, but it also doesn't yield 100% of the memory benefit yet.

It does yield significant benefit though -- because it changes the storage model for tags and tag-extracted names to store them all contiguously in a block of memory rather than as discrete std::string objects. This alone reduces memory usage about 25% based on HeapStatsThreadLocalStoreTest.MemoryWithoutTls and HeapStatsThreadLocalStoreTest.MemoryWithTls.

The next step is a series of fairly small PRs that will change various subsystems to use this interface rather than specifying stats by string literals. Once this is done, with the constituent symbols in most cases allocated at startup time rather than ad-hoc, the symbol-table implementation can be switched out from a fake version (that just uses raw strings underneath) into the real version, where there is sharing of symbols.

Risk Level: medium -- this is a pretty large refactor. There is some functional risk as well as performance risk. However this is intended to be an essentially non-functional refactor.
Testing: //test/...
Docs Changes: Updated stats.md to reflect the plan.
Release Notes: n/a

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…mbol tables. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…s impacting microbenchmarks. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…lls. Helped but not too much. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ing each element. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…temp creation. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

…, etc. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…oxy#6161. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

) * Plumb through symbol tables everywhere. Pulled from #6161. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

Flushing out some comments. The main thing to sort out is merge order between this and the hot restart change. My personal preference is to land the hot restart change first since it's pretty close but will let you and @fredlas sort it out.

The other thing I would like to figure out is how to avoid the manual memory management that callers need to do here. I think we can provide some type of smart pointer, RAII guard, etc. to handle this?

Thank you!

/wait

include/envoy/stats/tag_producer.h

source/common/stats/heap_stat_data.h

source/common/stats/metric_impl.cc

source/common/stats/metric_impl.h

source/common/stats/raw_stat_data.cc

@ambuc

…ons. (#6688) Per @ambuc's comment on #6161 I think it would be better to keep a log of the memory consumed by stats, and also use exact comparisons. That way we can get historical perspective into the relative impact of adding new stats or families of stats. Risk Level: low, but could cause more changes to this one test. Testing: just this one test. Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

/wait-any

source/extensions/stat_sinks/hystrix/hystrix.cc

…strix. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…t most call-sites outside of stats to use. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-04-25T17:00:51Z

/retest

repokitteh-read-only · 2019-04-25T17:00:58Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6161 (comment) was created by @jmarantz.

see: more, trace.

jmarantz · 2019-04-25T19:19:35Z

/retest

repokitteh-read-only · 2019-04-25T19:19:39Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6161 (comment) was created by @jmarantz.

see: more, trace.

jmarantz · 2019-04-25T21:15:07Z

/retest

repokitteh-read-only · 2019-04-25T21:15:12Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #6161 (comment) was created by @jmarantz.

see: more, trace.

mattklein123

LGTM with one thing. Very cool stuff!

/wait

mattklein123 · 2019-04-25T23:58:06Z

source/common/stats/thread_local_store.cc

-    // can complete properly, even if the tag values are partially truncated.
-    std::string tag_extracted_name = parent_.getTagsForName(name, tags);
+    TagExtraction extraction(parent_, name);
+    // std::shared_ptr<StatType> stat = make_stat(parent_.alloc_, extraction.truncatedStatName(),


Ouch, yes. WDYT about a quick follow-up for that?

never mind; deleted.

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123

Sorry, follow up would have been fine. Very nice!

jmarantz · 2019-04-26T11:44:25Z

/retest

repokitteh-read-only · 2019-04-26T11:44:29Z

🔨 rebuilding ci/circleci: release (failed build)

🐱

Caused by: a #6161 (comment) was created by @jmarantz.

see: more, trace.

…nvoyproxy#6161) * Use SymbolTable API for creating and representing stat names. Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Jeff Piazza <jeffpiazza@google.com>

jmarantz added 3 commits March 4, 2019 14:59

Use SymbolTable API for creating and representing stat names.

f6d3ec4

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Rename counterx to counterFromStatName; ditto for gaugex, histogramx.

9e47dfe

Signed-off-by: Joshua Marantz <jmarantz@google.com>

avoid extra copy when encoding strings into FakeSymbolTableImpl.

f384eeb

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 self-assigned this Mar 10, 2019

jmarantz added 11 commits March 11, 2019 12:57

Use FakeSymbolTable more consistently. Start updating stats.md for sy…

f138212

…mbol tables. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-symtab-interface

342cb7c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

checkpoint with impl-specific Encoding class to remove a copy that wa…

52e07df

…s impacting microbenchmarks. Signed-off-by: Joshua Marantz <jmarantz@google.com>

hacks to use stack allocated arrays for temps to reduce new/delete ca…

08c35f5

…lls. Helped but not too much. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Make a separate populateList for Fake table that doesn't bother encod…

94fba4f

…ing each element. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Use string_view through to the regexes to avoid an extra std::string …

c103f22

…temp creation. Signed-off-by: Joshua Marantz <jmarantz@google.com>

format

91ec93c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

cleanups.

c02132c

Signed-off-by: Joshua Marantz <jmarantz@google.com>

remove commented out reference to std::cerr, caught by spelling :)

31c8a61

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Removed no-longer-needed class FakeSymbolTableImpl::Encoding.

cb9e6f2

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove commented out blocks, add 'override' attribute to virtual dtor…

944b811

…, etc. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added a commit to jmarantz/envoy that referenced this pull request Mar 15, 2019

Speedups to FakeSymbolTableTest based on microbenchmarks from envoypr…

1e11c8b

…oxy#6161. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz mentioned this pull request Mar 15, 2019

Speedups to FakeSymbolTableTest based on microbenchmarks from #6161. #6293

Merged

jmarantz added 2 commits March 14, 2019 23:11

Remove commented-out code.

e3a9eae

Signed-off-by: Joshua Marantz <jmarantz@google.com>

tweaks pulled over from envoyproxy#6293.

8c72c2d

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added a commit to jmarantz/envoy that referenced this pull request Mar 15, 2019

Plumb through symbol tables everywhere. Pulled from envoyproxy#6161.

ea4d73f

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 3 commits March 15, 2019 14:38

back out some changes that are not needed.

26700c8

Signed-off-by: Joshua Marantz <jmarantz@google.com>

backout router changes which are not needed.

323b005

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Remove more changes that are not clearly needed yet in this PR.

9449596

Signed-off-by: Joshua Marantz <jmarantz@google.com>

mattklein123 added the waiting label Mar 17, 2019

jmarantz mentioned this pull request Mar 17, 2019

stats: Plumb through symbol tables everywhere. Pulled from #6161. #6299

Merged

patches from envoyproxy#6299

61ea4e2

Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added a commit that referenced this pull request Mar 19, 2019

stats: Plumb through symbol tables everywhere. Pulled from #6161. (#6299

8593e6a

) * Plumb through symbol tables everywhere. Pulled from #6161. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 2 commits March 19, 2019 21:07

Merge branch 'master' into stats-symtab-interface

e6c4ea4

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-symtab-interface

38acb61

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label Mar 20, 2019

mattklein123 requested changes Apr 24, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 24, 2019

jmarantz added 2 commits April 24, 2019 17:41

Add more comments to help clarify review comments.

30b188e

Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-symtab-interface

81b9cef

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label Apr 24, 2019

mattklein123 reviewed Apr 25, 2019

View reviewed changes

source/extensions/stat_sinks/hystrix/hystrix.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting:any label Apr 25, 2019

jmarantz added 2 commits April 25, 2019 10:40

Rename StatNameTempStorage to StatNameManagedStorage and use it in Hy…

8183533

…strix. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Improve commenting for StatNameManagedStorage as that's what we expec…

ead848f

…t most call-sites outside of stats to use. Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting:any label Apr 25, 2019

mattklein123 requested changes Apr 25, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 25, 2019

delete commented-out code.

18ae590

Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only bot removed the waiting label Apr 26, 2019

mattklein123 approved these changes Apr 26, 2019

View reviewed changes

jmarantz merged commit 304ec56 into envoyproxy:master Apr 26, 2019

jmarantz deleted the stats-symtab-interface branch April 26, 2019 14:52

jmarantz mentioned this pull request May 1, 2019

stats: Add iterate methods for tags. #6764

Merged

Conversation

jmarantz commented Mar 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jmarantz commented Apr 25, 2019

Uh oh!

repokitteh-read-only bot commented Apr 25, 2019

Uh oh!

jmarantz commented Apr 25, 2019

Uh oh!

repokitteh-read-only bot commented Apr 25, 2019

Uh oh!

jmarantz commented Apr 25, 2019

Uh oh!

repokitteh-read-only bot commented Apr 25, 2019

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Apr 25, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

jmarantz Apr 26, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Apr 26, 2019

Uh oh!

repokitteh-read-only bot commented Apr 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarantz commented Mar 4, 2019 •

edited

Loading