stats: Use inlined primitive counter and gauge for HostStats in order to reduce per-host memory usage. by antoniovicente · Pull Request #8216 · envoyproxy/envoy

antoniovicente · 2019-09-11T18:23:08Z

Description: Introduce low memory overhead counter and gauges classes for use in HostStats. These new isolated stats objects are meant to be instantiated inline on stats structs rather than created by name from a stats storage and ref counted. Low-memory stats objects are important in the case of HostStats due to the large number of hosts that a proxy can end up talking to.
Risk Level: Low
Testing: unit test
Docs Changes: n/a
Release Notes: n/a
Fixes #7525

…uce per-host memory usage. Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

mattklein123 · 2019-09-11T19:33:41Z

@antoniovicente qq: are you still planning on having an option to completely remove host stats (or have them be canary only, a % of hosts, etc.)? Or is this good enough for your current case?

antoniovicente · 2019-09-11T21:11:24Z

@antoniovicente qq: are you still planning on having an option to completely remove host stats (or have them be canary only, a % of hosts, etc.)? Or is this good enough for your current case?

Stats overhead is down to 96 bytes per host which seems good enough for now. I'll likely start looking into improvements in other areas next, including possibly thinking about how to share upstream connections across serving threads or optimize route selection.

mattklein123 · 2019-09-11T23:20:45Z

I'll likely start looking into improvements in other areas next, including possibly thinking about how to share upstream connections across serving threads or optimize route selection.

Cool sounds great.

jmarantz

This is awesome stuff. I have one minor suggestion and a potential behavior difference to check.

include/envoy/stats/primitive_stats.h

source/server/http/admin.cc

include/envoy/upstream/host_description.h

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz

lgtm modulo minor comments/prefs.

include/envoy/upstream/host_description.h

test/integration/stats_integration_test.cc

…tats Signed-off-by: Antonio Vicente <avd@google.com>

…map<string,STAT> to vector<pair<string_view,STAT>> Signed-off-by: Antonio Vicente <avd@google.com>

…emory usage in non golden build configurations. Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz

just a few minor suggestions; up to you.

test/common/upstream/host_stats_test.cc

Signed-off-by: Antonio Vicente <avd@google.com>

test/common/upstream/host_stats_test.cc

…Gauge Signed-off-by: Antonio Vicente <avd@google.com>

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz

@envoyproxy/senior-maintainers for another pass (need non-googler)

mattklein123

Thanks this is awesome. Quick question.

/wait-any

mattklein123 · 2019-09-24T22:57:07Z

source/common/upstream/upstream_impl.h

  Stats::StatNameManagedStorage locality_zone_stat_name_;
-  Stats::IsolatedStoreImpl stats_store_;
-  HostStats stats_;
+  mutable HostStats stats_;


qq: what was the reasoning behind the mutable and const to non-const switch here? Presumably because we were previously violating the constness anyway and this makes it more explicit? It would be pretty cool to not do this. I assume you looked and it's not easy to fix this right now because we modify stats already though a const host?

Before this change HostStats contained non-const references to counter objects, while now it holds the counters themselves as members.

Consider host.stats().rq_success_.inc() before and after that change:
before: The HostStats returned by Host::stats is const, but rq_success_ is not const so the call to inc() is fine.
after: if HostStats is const its members are also const, attempts to call inc() on the const counter object will fail compilation.

We could instead make the counters and gauges inside HostStats as mutable. Making the stats() method non-const seems challenging due to widespread use of HostConstSharedPtr in load balancers. Calls to stats() on a HostConstSharedPtr followed by increment calls on a counter are widespread, see source/common/router/router.cc for some examples.

Should we consider switch from std::shared_ptr to std::shared_ptr as the primary way to reference Host objects in order to avoid use of mutable?

Should we consider switch from std::shared_ptr to std::shared_ptr as the primary way to reference Host objects in order to avoid use of mutable?

I'm not sure what the best thing to do here is. This is a pre-existing problem elsewhere, so it's not something you introduced, it's just never thrilled me that we ended up here and I wanted to make sure it's the same issue. I would say let's just merge this and we can consider further cleanups later if you are so inclined?

Sounds good. Thanks!

mattklein123

Thank you!

… to reduce per-host memory usage. (envoyproxy#8216) Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente added 2 commits September 11, 2019 14:06

Use inlined primitive counter and gauge for HostStats in order to red…

1809930

…uce per-host memory usage. Signed-off-by: Antonio Vicente <avd@google.com>

Set PR number on stats_integration_test history.

f36da29

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente changed the title ~~Use inlined primitive counter and gauge for HostStats in order to reduce per-host memory usage.~~ stats: Use inlined primitive counter and gauge for HostStats in order to reduce per-host memory usage. Sep 11, 2019

mattklein123 assigned jmarantz Sep 11, 2019

jmarantz reviewed Sep 14, 2019

View reviewed changes

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

source/server/http/admin.cc Outdated Show resolved Hide resolved

include/envoy/upstream/host_description.h Show resolved Hide resolved

antoniovicente added 2 commits September 16, 2019 13:08

Disable copy constructors by inheriting from NonCopyable

258548a

Signed-off-by: Antonio Vicente <avd@google.com>

Fix typo in comment

93d7556

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz reviewed Sep 16, 2019

View reviewed changes

include/envoy/upstream/host_description.h Show resolved Hide resolved

test/integration/stats_integration_test.cc Outdated Show resolved Hide resolved

antoniovicente added 3 commits September 20, 2019 14:59

Merge remote-tracking branch 'upstream/master' into stats_primitive_s…

5e70b28

…tats Signed-off-by: Antonio Vicente <avd@google.com>

Change return type of HostStats::counters and HostStats::gauges from …

422d9f1

…map<string,STAT> to vector<pair<string_view,STAT>> Signed-off-by: Antonio Vicente <avd@google.com>

Reduce m_per_host to 1283 to gather info on a better upper bound on m…

cfba788

…emory usage in non golden build configurations. Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz previously approved these changes Sep 20, 2019

View reviewed changes

test/common/upstream/host_stats_test.cc Outdated Show resolved Hide resolved

test/common/upstream/host_stats_test.cc Outdated Show resolved Hide resolved

test/common/upstream/host_stats_test.cc Outdated Show resolved Hide resolved

Tighten memory upper bound based on macOS CI result.

e50e2b0

Signed-off-by: Antonio Vicente <avd@google.com>

antoniovicente dismissed jmarantz’s stale review via e50e2b0 September 23, 2019 09:52

Address nits in host_stats_test

6213c57

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz reviewed Sep 23, 2019

View reviewed changes

test/common/upstream/host_stats_test.cc Outdated Show resolved Hide resolved

antoniovicente added 3 commits September 24, 2019 12:26

Add std::reference_wrapper aliases for PrimitiveCounter and Primitive…

4a41f87

…Gauge Signed-off-by: Antonio Vicente <avd@google.com>

fix whitespace in comments

8f01b22

Signed-off-by: Antonio Vicente <avd@google.com>

Fix format

4c89616

Signed-off-by: Antonio Vicente <avd@google.com>

jmarantz approved these changes Sep 24, 2019

View reviewed changes

mattklein123 self-assigned this Sep 24, 2019

mattklein123 reviewed Sep 24, 2019

View reviewed changes

repokitteh-read-only bot added waiting:any and removed waiting:any labels Sep 24, 2019

mattklein123 approved these changes Sep 25, 2019

View reviewed changes

mattklein123 merged commit 3bf4b47 into envoyproxy:master Sep 25, 2019

antoniovicente deleted the stats_primitive_stats branch September 26, 2019 09:52

danzh2010 pushed a commit to danzh2010/envoy that referenced this pull request Oct 4, 2019

stats: Use inlined primitive counter and gauge for HostStats in order…

61c88fc

… to reduce per-host memory usage. (envoyproxy#8216) Signed-off-by: Antonio Vicente <avd@google.com>

Conversation

antoniovicente commented Sep 11, 2019

Uh oh!

mattklein123 commented Sep 11, 2019

Uh oh!

antoniovicente commented Sep 11, 2019

Uh oh!

mattklein123 commented Sep 11, 2019

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

antoniovicente Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 Sep 25, 2019

Choose a reason for hiding this comment

Uh oh!

antoniovicente Sep 26, 2019

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants