stats: optimize stats-matchers instantiated with "prefix." pattern, to avoid having to do toString() mapping and caching by jmarantz · Pull Request #17018 · envoyproxy/envoy

jmarantz · 2021-06-17T01:25:19Z

Commit Message: Optimizes a useful special-case where we exclude stats by prefix. Ordinarily we must string-match against stringified StatName, and the stringification is both slow and requires a global symbol table lock. To avoid that becoming a performance/contention bottleneck, we keep excluded StatNames in the ThreadLocalStore caches, which can consume a lot of memory at scale. It consumes less than the stats, but it can still be significant.

To avoid this problem when matching with a full token prefix, we can do the exclusion comparison without converting to a string. This is fast and doesn't require taking any locks, so we don't need to save excluded StatNames in a map.

This is both faster and provides a significant reduction in memory.

Additional Description:
Risk Level: medium -- this adds some complexity to the StatsMatcher implementation
Testing: //test/... plus new performance tests, and targeted unit tests to hit corner cases
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes: #17027

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

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

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

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

jmarantz · 2021-06-17T21:34:00Z

/retest

repokitteh-read-only · 2021-06-17T21:34:04Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

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

…ejected by the fast path. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

jmarantz · 2021-06-20T18:48:48Z

Pradeep, can you take a pass through this? Don't hesitate to ping me with any questions, and I'm happy to break this up into 3 layered PRs if you like.

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

jmarantz · 2021-06-21T01:38:57Z

/retest

repokitteh-read-only · 2021-06-21T01:39:00Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

alyssawilk · 2021-06-24T15:29:14Z

Stephan's out for the end of the week - Yan can you do first pass?

zuercher

Other than some minor nits, the only thing that concerns me here is that the fast/slowRejects interface seems brittle. It's already kind of tricky to see that it's used correctly in the more complex cases.

What do you think about having fastRejects return a result type that's required as input to slowRejects? Something like:

using FastRejectResult = /* some simple struct that contains implements a 'bool rejected()` method. */

virtual FastRejectResult fastRejects(StatName name) const PURE;
virtual bool slowRejects(StatName name, FastRejectResult fast_result) const PURE;

It makes the if fastRejects(name) || slowRejects(name) a little more cumbersome, but maybe we can just have a helper for case?

envoy/stats/stats_matcher.h

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

…ed by slowRejects() Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2021-07-02T02:18:56Z

@zuercher I haven't taken care of the nits yet but I have a candidate patch I've committed for using an explicit intermediate state object per your suggestion (mostly). It makes things a quite bit more cumbersome at call-sites but it does make the implementation assumptions more explicit, so this is probably a win. The main problem is that the extra state has to be passed around to a a few different functions in ThreadLocalStore. WDYT?

zuercher · 2021-07-06T16:31:16Z

@jmarantz I agree this is a win: I find it easier to follow and easier to be confident that the fast/slow calls are done correctly.

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

jmarantz · 2021-07-06T20:36:55Z

OK ptal; I didn't really change anything from the prior state other than addressing the comment grammar issues and adding comments for the new nested class.

One thing I considered was protecting the default ctor of FastResult but it seems pretty cumbersome with mocks.

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

jmarantz · 2021-07-07T17:37:46Z

/retest

repokitteh-read-only · 2021-07-07T17:37:50Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

zuercher · 2021-07-08T04:33:06Z

source/common/stats/stats_matcher_impl.cc

+
+StatsMatcher::FastResult StatsMatcherImpl::fastRejects(StatName stat_name) const {
+  if (rejectsAll()) {
+    return FastResult::NoMatch;


Can’t this return “rejects”?

yes, good catch. that was working before I switched to enums, but there was not a test that discerned the difference between fast-reject and slow-reject of this case. I added a couple of tests for this.

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

zuercher

This lgtm, modulo the test flake.

jmarantz · 2021-07-09T01:43:19Z

/retest

repokitteh-read-only · 2021-07-09T01:43:22Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

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

see: more, trace.

…o avoid having to do toString() mapping and caching (envoyproxy#17018) Commit Message: Optimizes a useful special-case where we exclude stats by prefix. Ordinarily we must string-match against stringified StatName, and the stringification is both slow and requires a global symbol table lock. To avoid that becoming a performance/contention bottleneck, we keep excluded StatNames in the ThreadLocalStore caches, which can consume a lot of memory at scale. It consumes less than the stats, but it can still be significant. To avoid this problem when matching with a full token prefix, we can do the exclusion comparison without converting to a string. This is fast and doesn't require taking any locks, so we don't need to save excluded StatNames in a map. This is both faster and provides a significant reduction in memory. Additional Description: Risk Level: medium -- this adds some complexity to the StatsMatcher implementation Testing: //test/... plus new performance tests, and targeted unit tests to hit corner cases Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Fixes: envoyproxy#17027 Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 6 commits June 16, 2021 19:47

add API to do a symbolic prefix match on StatNames without taking a l…

5c29ac0

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

add prefix-optimized stats_matcher impl.

b134c93

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

Use the new stats-matcher API that optikmizes for prefixes.

68b7d41

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

Skip remembering rejected stats when specified cheaply.

f5068cc

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

add unit tests showing memory benefit, and benchmark tests showing CP…

bb11ad9

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

Merge branch 'main' into stat-name-prefix-match

7dc0ff0

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

jmarantz added 7 commits June 17, 2021 21:04

add missing arg.

b6a4edb

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

NOLINT for clang-tidy nits

c7a1147

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

short-circuit more code that doesn't need to run for stats that are r…

2dd3d57

…ejected by the fast path. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'main' into stat-name-prefix-match

d338dae

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

Fix spelling and simplify the comment.

8add54b

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

cleanup and add tests.

bfd6241

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

Remove StatMatcher::hasStringMatchers which is no longer needed.

b738222

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

jmarantz changed the title ~~WiP stats: optimize stats-matchers instantiated with "prefix." pattern, to avoid having to do toString() mapping.~~ stats: optimize stats-matchers instantiated with "prefix." pattern, to avoid having to do toString() mapping. Jun 20, 2021

jmarantz assigned pradeepcrao Jun 20, 2021

jmarantz marked this pull request as ready for review June 20, 2021 18:47

jmarantz added 2 commits June 20, 2021 15:16

improve commenting

051f412

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

Merge branch 'main' into stat-name-prefix-match

b1d572d

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

jmarantz changed the title ~~stats: optimize stats-matchers instantiated with "prefix." pattern, to avoid having to do toString() mapping.~~ stats: optimize stats-matchers instantiated with "prefix." pattern, to avoid having to do toString() mapping and caching. Jun 21, 2021

jmarantz changed the title ~~stats: optimize stats-matchers instantiated with "prefix." pattern, to avoid having to do toString() mapping and caching.~~ stats: optimize stats-matchers instantiated with "prefix." pattern, to avoid having to do toString() mapping and caching Jun 21, 2021

alyssawilk assigned zuercher and yanavlasov Jun 21, 2021

zuercher reviewed Jun 28, 2021

View reviewed changes

envoy/stats/stats_matcher.h Outdated Show resolved Hide resolved

envoy/stats/stats_matcher.h Outdated Show resolved Hide resolved

Merge branch 'main' into stat-name-prefix-match

7dd1905

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

introduce an intermediate result resulted by fastRejects() and requir…

1ff784f

…ed by slowRejects() Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 2 commits July 6, 2021 16:04

Merge branch 'main' into stat-name-prefix-match

5d4668f

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

Cleanup comments.

60e8c00

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

Switch from a class to an enum for FastResult; it's a bit cleaner.

fa7c3ca

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

zuercher reviewed Jul 8, 2021

View reviewed changes

jmarantz added 2 commits July 8, 2021 11:41

fast-reject the RejectsAll case, and test for that.

8dd325e

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

Merge branch 'main' into stat-name-prefix-match

2b21989

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

zuercher approved these changes Jul 8, 2021

View reviewed changes

jmarantz merged commit dd7bab7 into envoyproxy:main Jul 9, 2021

jmarantz deleted the stat-name-prefix-match branch July 9, 2021 03:19

Conversation

jmarantz commented Jun 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmarantz commented Jun 17, 2021

Uh oh!

repokitteh-read-only bot commented Jun 17, 2021

Uh oh!

jmarantz commented Jun 20, 2021

Uh oh!

jmarantz commented Jun 21, 2021

Uh oh!

repokitteh-read-only bot commented Jun 21, 2021

Uh oh!

alyssawilk commented Jun 24, 2021

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Jul 2, 2021

Uh oh!

zuercher commented Jul 6, 2021

Uh oh!

jmarantz commented Jul 6, 2021

Uh oh!

jmarantz commented Jul 7, 2021

Uh oh!

repokitteh-read-only bot commented Jul 7, 2021

Uh oh!

zuercher Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

jmarantz Jul 8, 2021

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Jul 9, 2021

Uh oh!

repokitteh-read-only bot commented Jul 9, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jmarantz commented Jun 17, 2021 •

edited

Loading