stats: Reduce stat memory by removing duplicate string storage in map keys. by jmarantz · Pull Request #4711 · envoyproxy/envoy

jmarantz · 2018-10-13T01:27:58Z

**Note: this code is ready IMO, but #4714 should be checked in first to get a baseline. **

Description: A significant amount of storage is wasted in the maps in ThreadLocalStore with copies of the strings. This changes the maps to use const char* keys, which are held in every form of stat. This is one step toward resolving #4196. It reduces memory from 68k/cluster to 54k/cluster in the Istio test referenced #4196.

A significant follow-up to this would be to tackle the redundant storage for tagExtractedName and TagVector.

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

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

… as a key. Also reduce HeapStatAllocator key overhead by 8 bytes by just using the char*. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

…ction boundaries around HeapStatData construction/destruction. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

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

mattklein123

Cool stuff. Two comments from a quick skim.

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

jmarantz · 2018-10-15T12:49:15Z

Closing temporarily until (a) #4714 can be merged and (b) I write a stats impl overview per @mattklein123's suggestion above.

…tats are robust at runtime. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

@mattklein123

…ce/doc/stats/md (#4729) per @mattklein123 's suggestion on #4711 , adds a stats design doc. In this rev it only pulls in material from a comment in thread_local_store.h. A few TBD sections were added to suggest structure, but the intent is not to fill them out in this PR, which just moves the existing material and adds an overview for context. Future stats-related PRs should include edits to this file to keep it up-to-date. This is one step toward resolving #4196. Risk Level: low Testing: none Docs Changes: this is a doc change. Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

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

jmarantz · 2018-10-16T21:50:42Z

Re-opening so we can discuss more about the code.

It would be nice to get #4741 and #4714 merged first and then this PR (once master-merged) will show more clearly the stats doc changes and the memory consumption win.

… name structure (#4741) Describe the current state of stat name representation, as a pre-cursor to #4711 which should be updated to modify the stat naming doc. Risk Level: low Testing: none Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com>

@mattklein123

…ce/doc/stats/md (envoyproxy#4729) per @mattklein123 's suggestion on envoyproxy#4711 , adds a stats design doc. In this rev it only pulls in material from a comment in thread_local_store.h. A few TBD sections were added to suggest structure, but the intent is not to fill them out in this PR, which just moves the existing material and adds an overview for context. Future stats-related PRs should include edits to this file to keep it up-to-date. This is one step toward resolving envoyproxy#4196. Risk Level: low Testing: none Docs Changes: this is a doc change. Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Yang Song <yasong@yasong00.cam.corp.google.com>

… name structure (envoyproxy#4741) Describe the current state of stat name representation, as a pre-cursor to envoyproxy#4711 which should be updated to modify the stat naming doc. Risk Level: low Testing: none Docs Changes: n/a Release Notes: n/a Signed-off-by: Joshua Marantz <jmarantz@google.com> Signed-off-by: Yang Song <yasong@yasong00.cam.corp.google.com>

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

…eflect intent. Would appreciate suggestions on how to improve the name, if you have any. Signed-off-by: Joshua Marantz <jmarantz@google.com>

…ings-in-map-keys Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

jmarantz

ptal -- this now directly shows memory-reduction in unit-tests, as well as an associated edit of the stats design doc.

mattklein123

In general this makes sense to me. However, do you want to get a speed benchmark in place before we merge this? Even for this change it seems worth it?

mattklein123 · 2018-10-25T21:40:20Z

-  auto data = std::make_unique<HeapStatData>(name);
+  // required to use this allocator. Note that data must be freed by calling
+  // its free() method, and not by destructing or unique_ptr. So this method
+  // cannot call anything that might throw, and thus it cannot through itself.


s/through/throw

no longer needed.

mattklein123 · 2018-10-25T21:42:37Z

@@ -77,7 +77,7 @@ struct RawStatData {
  /**
   * Returns the name as a std::string.


nit: update

done and switched to ersatz-doxygen here and elsewhere in the class.

mattklein123 · 2018-10-25T21:45:46Z

-  // required to use this allocator.
-  auto data = std::make_unique<HeapStatData>(name);
+  // required to use this allocator. Note that data must be freed by calling
+  // its free() method, and not by destructing or unique_ptr. So this method


Up to you, but you could use CSmartPtr (search code) to use RAII around calling free().

CSmartPtr doesn't quite work in this scenario, but unique_ptr does support a custom deletion functor so I just used it directly.

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

jmarantz

perf test in #4867 and merged here, showing a slight degradation in performance with non TLS, and a slight increase in performance with TLS, though these could be due to transient noisy neighbors as these runs were not properly interleaved.

The big win in both perf and memory comes from combining this and flat-hash-map. @mattklein123 would you prefer I just issue a combined PR for those?

old:

--------------------------------------------------------------
Benchmark                       Time           CPU Iterations
--------------------------------------------------------------
BM_StatsNoTls_mean       16249144 ns   16248983 ns         37
BM_StatsNoTls_median     16176011 ns   16175443 ns         37
BM_StatsNoTls_stddev       484905 ns     484909 ns         37
BM_StatsWithTls_mean     20958468 ns   20958244 ns         30
BM_StatsWithTls_median   20899829 ns   20899217 ns         30
BM_StatsWithTls_stddev     576702 ns     576784 ns         30

new:

--------------------------------------------------------------
Benchmark                       Time           CPU Iterations
--------------------------------------------------------------
BM_StatsNoTls_mean       17098922 ns   17099054 ns         34
BM_StatsNoTls_median     17049225 ns   17049419 ns         34
BM_StatsNoTls_stddev       354827 ns     354787 ns         34
BM_StatsWithTls_mean     20593702 ns   20593854 ns         30
BM_StatsWithTls_median   20527260 ns   20527525 ns         30
BM_StatsWithTls_stddev     532875 ns     532881 ns         30

flat-hash-map (#4715)

--------------------------------------------------------------
Benchmark                       Time           CPU Iterations
--------------------------------------------------------------
BM_StatsNoTls_mean       13582401 ns   13582484 ns         40
BM_StatsNoTls_median     13519546 ns   13519464 ns         40
BM_StatsNoTls_stddev       269552 ns     269544 ns         40
BM_StatsWithTls_mean     18171743 ns   18171829 ns         28
BM_StatsWithTls_median   18116579 ns   18116550 ns         28
BM_StatsWithTls_stddev     257126 ns     257189 ns         28

combined (#4872)

--------------------------------------------------------------
Benchmark                       Time           CPU Iterations
--------------------------------------------------------------
BM_StatsNoTls_mean       13779759 ns   13779854 ns         39
BM_StatsNoTls_median     13630165 ns   13630325 ns         39
BM_StatsNoTls_stddev       576447 ns     576471 ns         39
BM_StatsWithTls_mean     16336826 ns   16336967 ns         35
BM_StatsWithTls_median   16240568 ns   16240563 ns         35
BM_StatsWithTls_stddev     544943 ns     544942 ns         35

jmarantz · 2018-10-26T12:43:27Z

-  // required to use this allocator.
-  auto data = std::make_unique<HeapStatData>(name);
+  // required to use this allocator. Note that data must be freed by calling
+  // its free() method, and not by destructing or unique_ptr. So this method


CSmartPtr doesn't quite work in this scenario, but unique_ptr does support a custom deletion functor so I just used it directly.

jmarantz · 2018-10-26T12:53:34Z

-  auto data = std::make_unique<HeapStatData>(name);
+  // required to use this allocator. Note that data must be freed by calling
+  // its free() method, and not by destructing or unique_ptr. So this method
+  // cannot call anything that might throw, and thus it cannot through itself.


no longer needed.

jmarantz · 2018-10-26T12:55:27Z

@@ -77,7 +77,7 @@ struct RawStatData {
  /**
   * Returns the name as a std::string.


done and switched to ersatz-doxygen here and elsewhere in the class.

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

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

jmarantz · 2018-10-26T20:37:42Z

I decided to close this for now in favor of #4872 which incorporates the flat_hash_map change. That avoids the risk that this PR on its own has of slightly degrading one axis of performance (though improving several others).

LMK if you still think this one makes sense to go in on its own; the argument for that is that this change is complicated but bounded in scope.

Reduce stat memory by removing duplicate string storage in map keys.

633ee0f

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

zyfjeff reviewed Oct 13, 2018

View reviewed changes

Comment thread source/common/stats/heap_stat_data.cc

zyfjeff reviewed Oct 13, 2018

View reviewed changes

Comment thread source/common/common/hash.h

jmarantz added 8 commits October 12, 2018 23:36

Fix sanitization errors due to use of operator[] saving a temp string…

f5547b2

… as a key. Also reduce HeapStatAllocator key overhead by 8 bytes by just using the char*. Signed-off-by: Joshua Marantz <jmarantz@google.com>

improve commenting.

0d6d4d3

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

Fix issue with pointer to variable in internal scope.

55b88b4

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

Be stack-conscious with TlsHistogram creation too, and improve abstra…

3f4a592

…ction boundaries around HeapStatData construction/destruction. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Add test for the memory consumption of TLS stats maps.

cd2d533

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

Add baseline test.

d5cff26

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

Add stat memory testing infra and tests for thread-local-store and sy…

89eb789

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

format & spelling.

1ba37e5

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

jmarantz changed the title ~~stat: Reduce stat memory by removing duplicate string storage in map keys.~~ WIP stat: Reduce stat memory by removing duplicate string storage in map keys. Oct 13, 2018

jmarantz added 4 commits October 13, 2018 13:50

only run memory tests on vanilla builds, not tsan/asan/coverage.

70d0fc3

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

Only run memory tests under tcamlloc.

bc74a62

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

Also conditionalize memory-testing of symbol tables.

e146dfc

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

Merge branch 'stats-mem-tests' into no-dup-strings-in-map-keys

d1cda9b

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

mattklein123 reviewed Oct 13, 2018

View reviewed changes

Comment thread source/common/stats/thread_local_store.h

Comment thread include/envoy/stats/stats.h

jmarantz changed the title ~~WIP stat: Reduce stat memory by removing duplicate string storage in map keys.~~ WIP stats: Reduce stat memory by removing duplicate string storage in map keys. Oct 13, 2018

Merge branch 'master' into stats-mem-tests

1a59a29

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

jmarantz closed this Oct 15, 2018

jmarantz mentioned this pull request Oct 15, 2018

stats: Move stats impl discussion from thread_local_store.h into source/doc/stats/md #4729

Merged

jmarantz added 4 commits October 15, 2018 18:09

Address review comments, and make the decision about whether malloc s…

2158788

…tats are robust at runtime. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Update dates marking when certain byte-counts were measured.

86c9f33

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

fix comment referencing old #ifdef when we now use a runtime function.

7f77ffd

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

Use a better test for determinism in the malloc stats.

ff419a3

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

jmarantz added 3 commits October 15, 2018 23:05

Merge branch 'master' into no-dup-strings-in-map-keys

22458cf

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

Merge branch 'stats-mem-tests' into no-dup-strings-in-map-keys

594a729

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

add more context for TLS doc and establish a baseline for stat name s…

2f07f06

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

Add doc update relative to envoyproxy#4741

6ec8030

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

jmarantz reopened this Oct 16, 2018

mattklein123 self-assigned this Oct 23, 2018

jmarantz added 5 commits October 24, 2018 20:22

Merge branch 'master' into stats-mem-tests

f3ff5f9

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

Rename forEachStat to forEachSampleStat and update documentation to r…

25f1f92

…eflect intent. Would appreciate suggestions on how to improve the name, if you have any. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge remote-tracking branch 'origin/stats-mem-tests' into no-dup-str…

2f62e89

…ings-in-map-keys Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into no-dup-strings-in-map-keys

e4cdd92

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

clean up comments.

c81b435

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

jmarantz changed the title ~~WIP stats: Reduce stat memory by removing duplicate string storage in map keys.~~ stats: Reduce stat memory by removing duplicate string storage in map keys. Oct 25, 2018

jmarantz commented Oct 25, 2018

View reviewed changes

jmarantz mentioned this pull request Oct 25, 2018

WIP util: Add absl's new flat_hash_map and flat_hash_set to the build system and use it in a couple of places #4715

Closed

mattklein123 reviewed Oct 25, 2018

View reviewed changes

jmarantz added 4 commits October 25, 2018 18:23

adds a stats speed test.

c7830bb

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

checkout on speed test attempting to go multi-threaded.

880eeb4

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

checkpoint

249162e

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

enable tls-aware version of test, no multi-threading.

f7a360f

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

jmarantz mentioned this pull request Oct 26, 2018

Stats speed test #4867

Merged

Merge branch 'stats-speed-test' into no-dup-strings-in-map-keys

fb45eea

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

jmarantz commented Oct 26, 2018

View reviewed changes

jmarantz added 4 commits October 26, 2018 09:01

fix typos, use unique_ptr with custom deleter, and doxygenize some cl…

cd4f7c5

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

format

551f2cf

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

clang-tidy fix

11b0f39

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

Merge branch 'stats-speed-test' into no-dup-strings-in-map-keys

fefe18c

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

jmarantz closed this Oct 26, 2018

jmarantz mentioned this pull request Oct 26, 2018

stats: use flat_hash_map and make thread-local stat maps use key storage from stat #4872

Merged

		@@ -77,7 +77,7 @@ struct RawStatData {
		/**
		* Returns the name as a std::string.

Conversation

jmarantz commented Oct 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jmarantz commented Oct 15, 2018

Uh oh!

jmarantz commented Oct 16, 2018

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!

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

Choose a reason for hiding this comment

Uh oh!

jmarantz commented Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmarantz commented Oct 13, 2018 •

edited

Loading

jmarantz left a comment •

edited

Loading

jmarantz commented Oct 26, 2018 •

edited

Loading