stats: Shared scopes phase 1, changes scope APIs to use ScopeSharedPtr, leaving ScopePtr alias behind by jmarantz · Pull Request #19791 · envoyproxy/envoy

jmarantz · 2022-02-03T05:44:21Z

Commit Message: To support an algorithmic solution to a long burst of CPU on admin /stats with a large # clusters, we need to hold scopes in shared pointers. This PR makes that change and updates the usage within stats to reference scopes as ScopeSharedPtr rather than ScopePtr, and adds enable_shared_from_this as a Scope super-class so existing APIs that pass Scope& don't need to change.

This leaves behind a ScopePtr alias to ScopeSharedPtr temporarily to (a) make this PR be reviewable and (b) avoid breaking other repos that may reference ScopePtr. It will be easier to submit this PR first, then a external repos can rename their references, and we can do a follow-up PR that removes ScopePtr to avoid longer term confusion. That PR would like like #19790 which has 90 changed files in addition to the semantic change. Once this is merged, that PR will just be a strict rename and will be a lot easier to review.

This PR blocks #19693 which implements the admin change requiring holding onto a shared scope ptr.

Additional Description:
Risk Level: low
Testing: //test/...
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

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

…Ptr to make a smaller 1st review. Signed-off-by: Joshua Marantz <jmarantz@google.com>

repokitteh-read-only · 2022-02-03T05:44:24Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #19791 was opened by jmarantz.

see: more, trace.

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

jmarantz · 2022-02-03T13:52:00Z

/retest

repokitteh-read-only · 2022-02-03T13:52:04Z

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

🐱

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

see: more, trace.

pradeepcrao

Hi Josh, this change looks good on its own. I will spend some time tomorrow looking at how this fits in with the rest of the changes.

pradeepcrao · 2022-02-04T00:46:47Z

source/common/stats/isolated_store_impl.cc

      null_counter_(new NullCounterImpl(symbol_table)),
-      null_gauge_(new NullGaugeImpl(symbol_table)) {}
+      null_gauge_(new NullGaugeImpl(symbol_table)),
+      default_scope_(std::make_shared<ScopePrefixer>("", *this)) {}


Can we run into a situation wherein we have a shared_ptr to ScopePrefixer for the admin panel, but not the underlying scope?

The lifetime concerns are not changed by this PR; the Store needs to outlive all the Scopes declared from it.

In general if you instantiate any Scope-derived object on the stack, and you will get some time of failure (e.g. a weak_ptr exception).

That is why I changed default_scope_ to be allocated with make_shared, enabling #19693 to hold onto a reference to it, and mirroring the implementation in thread_local_store.cc.

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

…g over the default scope an extra time. Signed-off-by: Joshua Marantz <jmarantz@google.com>

pradeepcrao · 2022-02-07T22:09:37Z

The main doubt I have is that can the scopes be held by the admin page indefinitely, or is there a timeout mechanism? If the scopes live through several xDS updates, there is the possibility of Envoy OOMing.

jmarantz · 2022-02-07T22:15:38Z

This is a very insightful point! Let's dive in.

I don't think in practice this is a problem assuming requests have server-enforced timeouts. At the end of any request the shared_ptrs will be released. But it would be good to see if that's actually true.

Note we trust admin clients (a malicious one could just send /quitquitquit) but a broken admin client could just read the first chunk from a /stats request and then leave the connection open and start a new one. Note also that won't leak scopes quickly because it will just bump ref-counts on scopes that are held anyway in the configuration. The concern would be if xDS updates the config and drops the scope, held alive only by an in-progress admin request.

In the end I think it's request timeouts that prevent this from being a problem.

jmarantz · 2022-02-07T22:15:58Z

Also that's not an issue for this PR which is not changing the Admin behavior at all.

pradeepcrao · 2022-02-07T22:20:28Z

This is a very insightful point! Let's dive in.

I don't think in practice this is a problem assuming requests have server-enforced timeouts. At the end of any request the shared_ptrs will be released. But it would be good to see if that's actually true.

Note we trust admin clients (a malicious one could just send /quitquitquit) but a broken admin client could just read the first chunk from a /stats request and then leave the connection open and start a new one. Note also that won't leak scopes quickly because it will just bump ref-counts on scopes that are held anyway in the configuration. The concern would be if xDS updates the config and drops the scope, held alive only by an in-progress admin request.

In the end I think it's request timeouts that prevent this from being a problem.

Thanks, that was my only concern.

LGTM

jmarantz · 2022-02-07T22:37:40Z

Thanks Pradeep

@ggreenway can you take a look?

jmarantz · 2022-02-11T20:50:47Z

@ggreenway will you be able to take a look sometime? Or shall I re-assign?

ggreenway

I think this looks fine. I'd like to get @mattklein123 's opinion on this also.

ggreenway · 2022-02-14T20:16:35Z

envoy/stats/scope.h

  virtual ~Scope() = default;

+  /** @return a shared_ptr for this */
+  ScopeSharedPtr makeShared() { return shared_from_this(); }


I think this should be named getShared() or similar; makeShared() is too similar to std::make_shared() but this one doesn't construct a new object.

good point: done.

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

…e_shared which creates a new object. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

mattklein123

LGTM with small nit. I have some vague concerns about circular references and memory leaks but we will know soon enough. :)

/wait

mattklein123 · 2022-02-15T22:57:17Z

envoy/stats/scope.h

+// files with trivial changes. Furthermore, code that depends on the Envoy stats
+// infrastructure that's not in this repo will stop compiling when we remove
+// ScopePtr. We should fully remove this alias in a future PR and change all the
+// references, once known parthers that might break from this change of a chance


Suggested change

// references, once known parthers that might break from this change of a chance

// references, once known consumers that might break from this change have a chance

?

looks good; done by hand to avoid what I fear would be a DCO violation :) Committing shortly.

mattklein123 · 2022-02-15T23:02:30Z

source/common/stats/isolated_store_impl.h

  void forEachScope(SizeFn f_size, StatFn<const Scope> f_stat) const override {
    if (f_size != nullptr) {
-      f_size(1);
+      f_size(scopes_.size() + 1);


This is unrelated to this change but because of this change we can now do the API correctly? Is that right?

Sorry, which API are you referring to?

forEachScope

jmarantz · 2022-02-16T00:03:59Z

At the moment scopes don't reference other scopes so I'm not seeing where the circular reference would come from.

But @pradeepcrao also pointed out that we might hold onto shared_ptr<scope> keeping it alive longer if there is a long-running admin operation. But I think that scopes appearing/disappearing a lot slower than even a long admin request.

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

jmarantz · 2022-02-16T03:58:07Z

/retest

repokitteh-read-only · 2022-02-16T03:58:09Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

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

see: more, trace.

jmarantz · 2022-02-16T04:49:36Z

/retest

repokitteh-read-only · 2022-02-16T04:49:39Z

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

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

see: more, trace.

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

addressed the requested change.

…eSharedPtr (#20888) Commit Message: Follow-up to #19791, #19790 and #20871, finishing off all remaining ScopePtr references. One more PR to go which will be to remove the ScopePtr definition, but first need to make sure this doesn't break any references outside the repo. Testing: //test/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

…eSharedPtr (envoyproxy#20888) Commit Message: Follow-up to envoyproxy#19791, envoyproxy#19790 and envoyproxy#20871, finishing off all remaining ScopePtr references. One more PR to go which will be to remove the ScopePtr definition, but first need to make sure this doesn't break any references outside the repo. Testing: //test/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

…eSharedPtr (#20888) Commit Message: Follow-up to envoyproxy/envoy#19791, #19790 and #20871, finishing off all remaining ScopePtr references. One more PR to go which will be to remove the ScopePtr definition, but first need to make sure this doesn't break any references outside the repo. Testing: //test/... Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz added 2 commits February 2, 2022 21:18

change Stats::Scope usage to shared_ptr, eliminating ScopePtr as a de…

dfac8eb

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

back out most of the changes in the repo from ScopePtr to SharedScope…

fa3229e

…Ptr to make a smaller 1st review. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Clarify TODO comment.

80a85ac

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

jmarantz changed the title ~~stats: Shared scopes phase 1, leaves an alias behind and doesn't change most references.~~ stats: Shared scopes phase 1, leaving an alias behind and doesn't change most references. Feb 3, 2022

jmarantz assigned pradeepcrao Feb 3, 2022

jmarantz marked this pull request as ready for review February 3, 2022 16:21

jmarantz mentioned this pull request Feb 3, 2022

stats: change Stats::ScopePtr references to Stats::ScopeSharedPtr #19790

Merged

jmarantz changed the title ~~stats: Shared scopes phase 1, leaving an alias behind and doesn't change most references.~~ stats: Shared scopes phase 1, changes scope APIs to use ScopeSharedPtr, leaving ScopePtr alias behind Feb 3, 2022

pradeepcrao reviewed Feb 4, 2022

View reviewed changes

jmarantz added 2 commits February 3, 2022 20:37

Merge branch 'main' into shared-scopes-1

f3b90e7

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

add some tests for shared_ptr functionality, and also clean up loopin…

a0782ea

…g over the default scope an extra time. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz assigned ggreenway Feb 7, 2022

ggreenway previously requested changes Feb 14, 2022

View reviewed changes

ggreenway assigned mattklein123 Feb 14, 2022

jmarantz added 4 commits February 14, 2022 17:24

Merge branch 'main' into shared-scopes-1

6e81807

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

Rename make*Shared methods to get*Shared to distinguish from std::mak…

10c6ce3

…e_shared which creates a new object. Signed-off-by: Joshua Marantz <jmarantz@google.com>

format

b87e4d3

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

Merge branch 'main' into shared-scopes-1

3e6e71d

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

mattklein123 requested changes Feb 15, 2022

View reviewed changes

repokitteh-read-only bot added the waiting label Feb 15, 2022

comment cleanup

1e54c9d

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

repokitteh-read-only bot removed the waiting label Feb 16, 2022

mattklein123 approved these changes Feb 16, 2022

View reviewed changes

Merge branch 'main' into shared-scopes-1

b7516cd

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

jmarantz enabled auto-merge (squash) February 16, 2022 15:21

jmarantz merged commit 5671b08 into envoyproxy:main Feb 16, 2022

jmarantz deleted the shared-scopes-1 branch February 16, 2022 15:56

jmarantz mentioned this pull request Feb 16, 2022

stats: fix a std::make_unique which should have been make_shared. #19988

Merged

This was referenced Apr 19, 2022

stats: change Stats::ScopePtr references to Stats::ScopeSharedPtr #20871

Merged

stats: change all remaining Stats::ScopePtr references to Stats::ScopeSharedPtr #20888

Merged

jmarantz mentioned this pull request Nov 7, 2022

Stats::Store should not inherit from Stats::Scope #23862

Closed

jmarantz mentioned this pull request Mar 8, 2026

stats: excessive ref-count inc/dec causing performance bottleneck #43836

Open

	// references, once known parthers that might break from this change of a chance
	// references, once known consumers that might break from this change have a chance

Conversation

jmarantz commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 3, 2022

Uh oh!

jmarantz commented Feb 3, 2022

Uh oh!

repokitteh-read-only bot commented Feb 3, 2022

Uh oh!

pradeepcrao 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!

pradeepcrao commented Feb 7, 2022

Uh oh!

jmarantz commented Feb 7, 2022

Uh oh!

jmarantz commented Feb 7, 2022

Uh oh!

pradeepcrao commented Feb 7, 2022

Uh oh!

jmarantz commented Feb 7, 2022

Uh oh!

jmarantz commented Feb 11, 2022

Uh oh!

ggreenway 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 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!

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Feb 16, 2022

Uh oh!

jmarantz commented Feb 16, 2022

Uh oh!

repokitteh-read-only bot commented Feb 16, 2022

Uh oh!

jmarantz commented Feb 16, 2022

Uh oh!

repokitteh-read-only bot commented Feb 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jmarantz commented Feb 3, 2022 •

edited

Loading