stats: add mechanism to create dynamic tokens without locks or sharing by jmarantz · Pull Request #9355 · envoyproxy/envoy

jmarantz · 2019-12-15T04:54:41Z

Description:
Solves the issue of lock contention for stat-name creation for names not known at compile-time.

This could be broken up into as many 3 PRs, but I think it's reviewable as is. If we did break it up we could have:

one new PR to just add the StatNameDynamicStorage and StatNameDynamicPool abstractions
another PR to change all the uses of StatNameSet::getDynamic() to StatNameDynamicStorage
a final PR to remove the StatNameSet::getDynamic capabilities, as well as the recent_lookups_
infrastructure from StatNameSet

In any case, a follow-up PR could remove the dynamics from ip_tagging_filter.cc which appears not to need them; I think all the possible tags are known at config-time. This is not included in this PR.

Risk Level: medium -- lots of bytes being messed with
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Fixes: #7003

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

…, or on the length of a fake-symbol table stat-name. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

…es or symbols. it's bytes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

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

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

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

… into stats-fuzzer-not-too-long Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

jmarantz

Thanks for diving in. I know this is pretty hairy low-level stuff. Hopefully we've got the testing & fizzing covered.

I will follow up with more detail about the representation in stats.md.

jmarantz · 2020-01-10T14:14:19Z

test/common/stats/symbol_table_impl_test.cc


 class StatNameDeathTest : public StatNameTest {};

+#if 0


Good catch; forgot I had deferred reviving this test. Done.

jmarantz · 2020-01-10T14:14:34Z

source/common/stats/symbol_table_impl.cc

+
+// When storing Symbol arrays, we disallow Symbol 0, which is the only Symbol
+// that will decode into uint8_t array starting (and ending) with {0}. Thus we
+// can use a leading 0 in in the first byte to indicate that what follows is


jmarantz · 2020-01-10T14:14:40Z

source/common/stats/symbol_table_impl.cc

+
+  // payload_bytes is the total number of bytes needed to represent the characters
+  // in name, plus their encoded size, plus the literal indicator.
+  uint64_t payload_bytes = SymbolTableImpl::Encoding::totalSizeBytes(name.size()) + 1;


jmarantz · 2020-01-10T14:41:01Z

source/common/stats/symbol_table_impl.cc

+  // So the layout is
+  //   [ length-of-whole-StatName, LiteralStringIndicator, length-of-name, name ]
+  // So we need to figure out how many bytes we need to represent length-of-name and
+  // bytes


jmarantz · 2020-01-10T14:54:19Z

source/extensions/filters/http/dynamo/dynamo_filter.cc

      time_source_.monotonicTime() - start_decode_);

  size_t group_index = DynamoStats::groupIndex(status);
+  Stats::StatNameDynamicPool dynamic(stats_->symbolTable());


I think the perf implication is in the noise when there is no contention. Previously this code would construct some strings and look them up in a map, re-using a StatName on a hit. Now we construct the same strings and create an encoding of them unconditionally, skipping the map lookup and mutex acquisition. I would guess that the successful map lookup, which has to touch all the bytes of the string during the hash and again in the compare, is roughly on par with the cost of doing the unconditional encoding.

In the new scenario we'll have to do a memory allocation even for a symbol we've seen before, but in most of the cases we'll have to do a StrCat first anyway.

If you compare this to a scenario where we don't use symbol tables at all, there's some overhead because we need to re-encode the string into a block of memory that's roughly equivalent in size. But we'll still benefit when doing join() because the non-dynamic fragments (e.g. scope) that are being joined with will likely be 4 bytes or less.

Probably I should try to borrow or steal some of the infrastructure in common/http/codes.cc though so we don't have to make dynamic entries for upstream_req_total_XX and upstream_rq_time_XX, though I don't know if the XX codes are constrained the same way http codes are.

I'll try to summarize this for stats.md.

jmarantz · 2020-01-10T14:59:01Z

source/extensions/filters/http/fault/fault_filter.cc

+    if (downstream_cluster_.empty()) {
+      downstream_cluster_storage_.reset();


jmarantz · 2020-01-10T15:05:02Z

source/extensions/filters/http/ip_tagging/ip_tagging_filter.cc

    tag_data.emplace_back(ip_tag.ip_tag_name(), cidr_set);
  }
  trie_ = std::make_unique<Network::LcTrie::LcTrie<std::string>>(tag_data);
+  // TODO(jmarantz): save stat-names for each remote address as stat_name_set builtins.


Not sure; I used 'address' here as trie is constructed from:

std::vector<std::string> tags = config_->trie().getData(callbacks_->streamInfo().downstreamRemoteAddress());

but happy to use whatever terminology you think makes the most sense. Changed.

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

mattklein123

Thanks generally LGTM but will wait for the final updates to statsd.md for further review. Thank you!

/wait

mattklein123 · 2020-01-10T18:38:32Z

source/docs/stats.md

 as SSL ciphers or Redis commands.

-### Dynamic stat tokens
+### Dynnamic stat tokens


Done; also added a diagram showing memory layouts of a few scenarios, and a table of all the classes related to symbol tables.

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

mattklein123 · 2020-01-11T00:18:48Z

@tonya11en this is the PR I mentioned to you. Can you help review the symbol table changes? Thank you.

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

jmarantz · 2020-01-13T13:12:58Z

/retest

repokitteh-read-only · 2020-01-13T13:13:04Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

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

see: more, trace.

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

mattklein123

Thanks for the added docs and diagram. It's super clear to me now. This LGTM so I'm fine with merging, but I would still like @tonya11en to post review when he has time. Thank you!

jmarantz · 2020-01-17T18:08:50Z

source/extensions/filters/network/mongo_proxy/proxy.cc

    names.reserve(6); // 2 entries are added by chargeQueryStats().
    names.push_back(mongo_stats_->collection_);
-    names.push_back(mongo_stats_->getDynamic(active_query->query_info_.collection()));
+    Stats::StatNameDynamicPool dynamic(mongo_stats_->symbolTable());


@jplevyak PTAL at this change even if you won't be able to review the entire PR. Note taht StatNameSet::getDynamic() is being removed in this PR in preference to this new model of making StatNames from dynamically changing strings that inline all the bytes and do not share or take locks (inspired by our discussion last month).

I imagine some of these sorts of changes will need to happen in the wasm fork.

tonya11en

Actually- editing that last comment. I still need to grok what's going on in the symbol table files.

I'm spread kind of thin this week, so please don't block on me. I'm still coming up to speed on stats code.

tonya11en · 2020-01-20T04:49:34Z

source/docs/stats.md

+are scenarios where the names are newly discovered from data in requests. To
+avoid taking locks in this case, tokens can be formed dynamically using
+`StatNameDynamicStorage` or `StatNameDynamicPool`. In this case we lose
+substring sharing but we avoid taking locks. Dynamically generaeted tokens can


nit: spelling (generaeted)

thanks will do a quick follow-up for that so I can merge this in.

jmarantz added 30 commits October 26, 2019 21:44

Avoid crashes in fuzz-tests due to input strings being too long.

5d40fc4

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

Eliminate 64k limit on # of segments in a real symbol-table stat-name…

8f9ab93

…, or on the length of a fake-symbol table stat-name. Signed-off-by: Joshua Marantz <jmarantz@google.com>

some cleanup

beb4684

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

Share helper methods for encoding/decoding symbols and sizes.

d143365

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

cleanup

5ef1a49

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

typo

c425551

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

fix confusing about whether the encoding-length was the number of byt…

f680ccc

…es or symbols. it's bytes. Signed-off-by: Joshua Marantz <jmarantz@google.com>

fix over-aggressive inequality for fake symbol tables

9fee9f5

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

don't recompute the number of bytes consumed.

184fbed

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

Merge branch 'master' into stats-fuzzer-not-too-long

6ac3dee

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

remove dynamo-stats test change which is not needed

9c62a9e

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

use totalSizeBytes to simplify some code.

4482ca7

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

Merge branch 'master' into stats-fuzzer-not-too-long

14220bb

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

add MemBlock abstraction to isolate risky memory operations.

4d345d0

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

reduce need to do byte access to the memory buffers.

5b6f0db

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

remove remainder of memcpy and pointer arithmetic outside MemBlock cl…

70ffe33

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

format

9f1eae7

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

some cleanup -- still needs more unit tests.

cf33e18

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

remove superfluous methods.

14b8740

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

add unit test.

eb6eb51

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

improve class comment.

6d4b8bc

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

rename MemBlock to MemBlockBuilder.

cf71f0b

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

Rename MemBlock to MemBlockBuilder to better reflect intended usage m…

256a857

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

Merge branch 'master' into stats-fuzzer-not-too-long

cb17d28

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

format.

7b3150e

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

improve comments and function naming.

c4ddd4a

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

Merge branch 'master' into stats-fuzzer-not-too-long

c728651

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

guard memcpy against 0-byte calls, which show up as asan failures.

4824977

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

Merge branch 'stats-fuzzer-not-too-long' of github.com:jmarantz/envoy…

3c9b184

… into stats-fuzzer-not-too-long Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into stats-fuzzer-not-too-long

5f4ea86

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

jmarantz added 2 commits January 10, 2020 10:08

review comments.

184f216

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

added cost discussion to the dynamic stats section in stats.md.

ed39356

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

repokitteh-read-only bot removed the waiting label Jan 10, 2020

jmarantz commented Jan 10, 2020

View reviewed changes

formatting

090b4f3

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

mattklein123 requested changes Jan 10, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jan 10, 2020

jmarantz added 2 commits January 10, 2020 18:06

add diagram

a8c886c

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

spelling

f4276dc

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

repokitteh-read-only bot removed the waiting label Jan 10, 2020

jmarantz added 3 commits January 10, 2020 18:40

clean up diagram and add some text.

9871570

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

add class dictionary

15ca059

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

typos

490ea3e

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

mattklein123 assigned tonya11en Jan 11, 2020

jmarantz added 4 commits January 12, 2020 13:22

add reference to diagram source so others can edit.

f1e8100

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

fix link

e21d80b

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

fix link try 2

cfb6c43

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

clean up diagram a little (small fonts 7 to 8, etc).

d89e3c6

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

jmarantz added 2 commits January 14, 2020 11:30

Merge branch 'master' into stats-dynamic-escape

8711d30

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

fix merge-conflict remnant.

c3d7b72

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

mattklein123 approved these changes Jan 16, 2020

View reviewed changes

jmarantz commented Jan 17, 2020

View reviewed changes

tonya11en approved these changes Jan 21, 2020

View reviewed changes

jmarantz merged commit 7c78372 into envoyproxy:master Jan 21, 2020

jmarantz deleted the stats-dynamic-escape branch January 21, 2020 13:35

jmarantz mentioned this pull request Jan 21, 2020

stats: fix typo in doc #9755

Merged

		if (downstream_cluster_.empty()) {
		downstream_cluster_storage_.reset();

Conversation

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

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmarantz Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Jan 11, 2020

Uh oh!

jmarantz commented Jan 13, 2020

Uh oh!

repokitteh-read-only bot commented Jan 13, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonya11en left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarantz commented Dec 15, 2019 •

edited

Loading

jmarantz Jan 10, 2020 •

edited

Loading

tonya11en left a comment •

edited

Loading