stats: Add fake symbol table as an intermediate state to move to SymbolTable API without taking locks. #5414

jmarantz · 2018-12-25T03:51:20Z

Description: Adds an abstract interface for SymbolTable and alternate implementation FakeSymbolTableImpl, which doesn't take locks. Once all stat tokens are symbolized at construction time, this FakeSymbolTable implementation can be deleted, and real-symbol tables can be used, thereby reducing memory and improving stat construction time per #3585 and #4980 . Note that it is not necessary to pre-allocate all elaborated stat names because multiple StatNames can be joined together without taking locks, even in SymbolTableImpl.

This implementation simply stores the characters directly in the uint8_t[] that backs each StatName, so there is no sharing or memory savings, but also no state associated with the SymbolTable, and thus no locks needed.

Risk Level: low
Testing: //test/common/stats/...
Docs Changes: n/a
Release Notes: n/a

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

jmarantz · 2018-12-27T17:10:04Z

@mattklein123 any comments on this idea in general? TL;DR by switching to the SymbollTable API without changing the underlying rep for now, we can incrementally convert subsystems in Envoy to symbolize their stat names on construction with small-scope PRs. Then make the switch in SymbolTable impls after there are no hot runtime string-based stat lookups.

mattklein123 · 2018-12-28T03:36:20Z

@jmarantz if I understand the proposal at a high level this makes sense to me as a stepping stone. However, I do wonder if we should focus on getting rid of shared memory stats first? I'm once again having a really hard time keeping track of all the permutations we need to support and I feel that we would a) get much better prod/test coverage if everyone was using the same stats and b) it would be vastly easier to understand. Thoughts on priority?

jmarantz · 2018-12-28T15:59:51Z

Linking #4974 for context. I was reluctant to take that as a blocker for reducing stat memory as it's hard for me to anticipate the operational differences with the current hot-restart method, especially as we don't actually use hot-restart ourselves.

But I think it's a fair point that we need to be confident in every change that takes us toward a leaner stats architecture, and the three parallel implementations (hot-restart, non-hot-restart with fake symbol table, non-hot-restart with real symbol table) make that harder.

stale · 2019-01-04T16:27:58Z

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!

jmarantz · 2019-01-04T16:32:02Z

/wait

stale · 2019-01-11T19:18:14Z

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!

jmarantz · 2019-01-11T19:19:23Z

/wait

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

jmarantz · 2019-01-17T22:28:05Z

/wait

per DM with Matt we can go ahead with this in parallel with the hot-restart via RPC mechanism that @fredlas is working on.

htuch

Before diving into review, I'm curious if we have back-of-envelope SWAG for the potential savings here. I realize this PR isn't going to provide the measurable outcomes, but I'm curious what we're shooting for in eventual savings via symbol table. Do you have some thoughts @jmarantz?

jmarantz · 2019-01-18T19:23:17Z

Yes, from where we are now ~20k/cluster savings. I have a completed integration in #4980 that has potential lock-contention risk, but shows the memory savings.

htuch

I realize this is moving some existing interfaces around, but my main initial feedback is to make the public header for symbol_table.h to be linearly consumable and explain the concepts to the reader as if they've never seen the implementation and have no history of all the stats work. This is for both selfish reasons (as a reviewer who has been out of loop on the stats work, I don't have this context on hand) and also for the benefit of future folks who work in this part of the code.

include/envoy/stats/symbol_table.h

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

jmarantz

I hear you...I was hesitant about making SymbolTable into an abstract interface, as for performance/memory reasons it needs to expose some concepts to callers. The only reason I made it an interface here is to be able to have this intermediate state where we are using the SymbolTable API but with a fake implementation underneath that will never take locks.

Once we are fully transitioned to using the API, no locks will be taken in the hot-path. We could then remove the abstract interface.

Maybe a better approach is to skip the abstract API and just use more templating in the test. WDYT?

include/envoy/stats/symbol_table.h

ambuc

I think this looks like a good path forward. As always, the stats system is becoming more and more complex. Would it make sense to try and separate the inline docs into two portions: one inline function-level documentation about /how/ to use the stats system as it is, and one README-level design doc (maybe colocated in /stats) about /why/ the stats system is designed the way it is? That way a new developer can start running with the stats system at any time without needing to understand a larger roadmap for it.

include/envoy/stats/symbol_table.h

source/common/stats/symbol_table_impl.h

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

htuch

@jmarantz abstract API is fine, my ask around linear docs I think would be good either way, also +1 on what @ambuc suggests. I think this is a good move, just needs to be made clearer what is going on for folks who aren't living in the implementation day-to-day.

include/envoy/stats/symbol_table.h

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

… that need access. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2019-01-27T15:45:23Z

@ambuc the README you are thinking of is in source/docs/stats.md . The stats system is still not using SymbolTable in any way, so I did not edit that doc in this PR.

include/envoy/stats/symbol_table.h

source/common/stats/symbol_table_impl.cc

source/common/stats/symbol_table_impl.h

source/common/stats/symbol_table_impl.cc

source/common/stats/symbol_table_impl.h

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

include/envoy/stats/symbol_table.h

source/common/stats/symbol_table_impl.cc

htuch · 2019-01-29T19:56:59Z

source/common/stats/symbol_table_impl.h

+//
+// The maximum size of the list is 255 elements, so the length can fit in a
+// byte. It would not be difficult to increase this, but there does not appear
+// to be a current need.


As an aside, I keep thinking that we shouldn't have to reinvent the byte packed design here, I'm sure this is done elsewhere for efficient string packing, but I don't see anything in STD or absl..

Understood; I'm sure this has been implemented somewhere, notably utf-8 encoders, but if it isn't in an already-used library it seems like it's easier to just have a robust tested impl here. WDYT?

I'd also be willing to factor this out and make it a separate tested packed-int abstraction in source/common/common, but I'd prefer to do that in a separate PR, as that code is already stable in the system and this PR is just abstrating the API to it.

test/common/stats/symbol_table_impl_test.cc

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

…ol table case. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

htuch

Great, nice PR!

…olTable API without taking locks. (envoyproxy#5414) Adds an abstract interface for SymbolTable and alternate implementation FakeSymbolTableImpl, which doesn't take locks. Once all stat tokens are symbolized at construction time, this FakeSymbolTable implementation can be deleted, and real-symbol tables can be used, thereby reducing memory and improving stat construction time per envoyproxy#3585 and envoyproxy#4980 . Note that it is not necessary to pre-allocate all elaborated stat names because multiple StatNames can be joined together without taking locks, even in SymbolTableImpl. This implementation simply stores the characters directly in the uint8_t[] that backs each StatName, so there is no sharing or memory savings, but also no state associated with the SymbolTable, and thus no locks needed. Risk Level: low Testing: //test/common/stats/... Signed-off-by: Joshua Marantz <jmarantz@google.com>

…olTable API without taking locks. (envoyproxy#5414) Adds an abstract interface for SymbolTable and alternate implementation FakeSymbolTableImpl, which doesn't take locks. Once all stat tokens are symbolized at construction time, this FakeSymbolTable implementation can be deleted, and real-symbol tables can be used, thereby reducing memory and improving stat construction time per envoyproxy#3585 and envoyproxy#4980 . Note that it is not necessary to pre-allocate all elaborated stat names because multiple StatNames can be joined together without taking locks, even in SymbolTableImpl. This implementation simply stores the characters directly in the uint8_t[] that backs each StatName, so there is no sharing or memory savings, but also no state associated with the SymbolTable, and thus no locks needed. Risk Level: low Testing: //test/common/stats/... Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>

jmarantz added 8 commits December 17, 2018 21:36

catch up with symtab-read-lock and to-string-on-symtab.

b1fcd49

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

refactor toString

121ec75

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

Merge branch 'master' into fake-symbol-table

a54a2bc

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

virtualize symbol-table.

4b9570c

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

use virtual interface in tests.

0e9303b

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

all tests working.

4fa1eb2

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

Fix asan failures, add comments, cleanup.

8a6aec1

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

clang-tidy fixes.

2209115

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

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 4, 2019

repokitteh-read-only bot added the waiting label Jan 4, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 4, 2019

jmarantz mentioned this pull request Jan 5, 2019

rewrite buffer implementation to eliminate evbuffer dependency #5441

Merged

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 11, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 11, 2019

Merge branch 'master' into fake-symbol-table

adf956e

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

repokitteh-read-only bot removed the waiting label Jan 14, 2019

Merge branch 'master' into fake-symbol-table

a92121d

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

repokitteh-read-only bot added the waiting label Jan 17, 2019

jmarantz requested review from ambuc and htuch January 18, 2019 15:57

htuch suggested changes Jan 18, 2019

View reviewed changes

htuch suggested changes Jan 22, 2019

View reviewed changes

jmarantz added 2 commits January 22, 2019 06:42

Merge branch 'master' into fake-symbol-table

c5e25e1

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

Sink Storage type nicknames into SymbolTable class.

a5a112e

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

repokitteh-read-only bot removed the waiting label Jan 22, 2019

jmarantz commented Jan 22, 2019

View reviewed changes

include/envoy/stats/symbol_table.h Outdated Show resolved Hide resolved

include/envoy/stats/symbol_table.h Outdated Show resolved Hide resolved

include/envoy/stats/symbol_table.h Outdated Show resolved Hide resolved

ambuc previously approved these changes Jan 22, 2019

View reviewed changes

include/envoy/stats/symbol_table.h Outdated Show resolved Hide resolved

source/common/stats/symbol_table_impl.h Show resolved Hide resolved

comment cleanup.

b37dc32

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

mattklein123 assigned htuch Jan 23, 2019

htuch suggested changes Jan 23, 2019

View reviewed changes

jmarantz added 2 commits January 26, 2019 15:52

Merge branch 'master' into fake-symbol-table

9140665

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

Privatize SymbolTable::free and incRefCount, friending helper classes…

11fd3c0

… that need access. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz dismissed ambuc’s stale review via 11fd3c0 January 27, 2019 15:31

htuch suggested changes Jan 28, 2019

View reviewed changes

jmarantz added 2 commits January 27, 2019 21:34

Improve comments, fix nits, typos, etc.

675b9d6

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

Remove 2-arg form of join().

35709b3

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

htuch suggested changes Jan 29, 2019

View reviewed changes

jmarantz added 6 commits January 29, 2019 17:30

Merge branch 'master' into fake-symbol-table

392a0be

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

Review style nits and actually test for zero contentions in fake symb…

e9f2b50

…ol table case. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Only start tracking the contentions right before doing the accesses.

71df963

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

Add missing include for vector.

ecb1b88

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

Merge branch 'master' into fake-symbol-table

586103c

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

Merge branch 'master' into fake-symbol-table

d804664

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

htuch approved these changes Jan 30, 2019

View reviewed changes

htuch merged commit 69964ba into envoyproxy:master Jan 30, 2019

jmarantz deleted the fake-symbol-table branch January 30, 2019 23:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stats: Add fake symbol table as an intermediate state to move to SymbolTable API without taking locks. #5414

stats: Add fake symbol table as an intermediate state to move to SymbolTable API without taking locks. #5414

jmarantz commented Dec 25, 2018 •

edited

Loading

jmarantz commented Dec 27, 2018

mattklein123 commented Dec 28, 2018

jmarantz commented Dec 28, 2018

stale bot commented Jan 4, 2019

jmarantz commented Jan 4, 2019

stale bot commented Jan 11, 2019

jmarantz commented Jan 11, 2019

jmarantz commented Jan 17, 2019

htuch left a comment

jmarantz commented Jan 18, 2019

htuch left a comment

jmarantz left a comment

ambuc left a comment

htuch left a comment

jmarantz commented Jan 27, 2019

htuch Jan 29, 2019

jmarantz Jan 29, 2019

jmarantz Jan 30, 2019

htuch left a comment

stats: Add fake symbol table as an intermediate state to move to SymbolTable API without taking locks. #5414

stats: Add fake symbol table as an intermediate state to move to SymbolTable API without taking locks. #5414

Conversation

jmarantz commented Dec 25, 2018 • edited Loading

jmarantz commented Dec 27, 2018

mattklein123 commented Dec 28, 2018

jmarantz commented Dec 28, 2018

stale bot commented Jan 4, 2019

jmarantz commented Jan 4, 2019

stale bot commented Jan 11, 2019

jmarantz commented Jan 11, 2019

jmarantz commented Jan 17, 2019

htuch left a comment

Choose a reason for hiding this comment

jmarantz commented Jan 18, 2019

htuch left a comment

Choose a reason for hiding this comment

jmarantz left a comment

Choose a reason for hiding this comment

ambuc left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

jmarantz commented Jan 27, 2019

htuch Jan 29, 2019

Choose a reason for hiding this comment

jmarantz Jan 29, 2019

Choose a reason for hiding this comment

jmarantz Jan 30, 2019

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

jmarantz commented Dec 25, 2018 •

edited

Loading