stats: Use a more compact rep for SymbolTable and add tests for memory usage by jmarantz · Pull Request #4696 · envoyproxy/envoy

jmarantz · 2018-10-11T22:32:05Z

Description: SymbolTables are conceptually and structurally really powerful ways to reduce memory overhead for repeated patterns. However the space savings was limited by some taxes:

references in structures to aid in RAII -- we can use assert statement to get most of the value.
lots of very small integers held in arrays of uint32_t -- we can a utf-8-like encoding scheme
16 bytes overhead for vectors, to store size and capacity -- we can store fixed size in 2 uint8s.

With these changes the raw space taken by all the stats in a 1k cluster system are reduced by 4x.
This is one step toward resolving #4196.

We will also be able to concatenate StatNames without accessing the global symbol table, enabling lock-free scoped name lookup.

Risk Level: low for now as they are not used yet, but probably wants to be fuzz-tested.
Testing: //test/common/stats/...
Docs Changes: n/a
Release Notes: n/a

…mpl_test.cc. This just corrects that for two tests. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

…an RAII to avoid leaks. The RAII concept is nice but it's expensive; need to keep 8-byte SymbolTable reference with each stat-name. Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

jmarantz · 2018-10-12T13:15:59Z

@ambuc -- still looking at CI issues.

…nd comment out broken SSL test for now. See envoyproxy#4703 Signed-off-by: Joshua Marantz <jmarantz@google.com>

…tions. I think virtualizing these interfaces has a significant cost in per-stat memory that I'd like to avoid. I don't at this point see a compelling need to mock symbol tables or have alternate representations. We could of course virtualize SymbolTable which there's only one of, but StatName does not want to be virtual. Signed-off-by: Joshua Marantz <jmarantz@google.com>

ambuc · 2018-10-12T14:01:10Z

This is great work -- One thing I would do is explicitly move the internal-only helper functions, bit-shifting magic, etc. into its own set of files, to make very explicit which parts are the interface for external use and which parts are private/protected. Ideally just the symbol table API would be exposed. I'd also make some differentiation between the two encode() functions for greppability.

…ializtion) and clean up stale pure virtual interface. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz · 2018-10-12T14:35:27Z

-  bssl::UniquePtr<X509> cert = readCertFromFile("test/common/ssl/test_data/san_dns_cert.pem");
-  EXPECT_EQ(270, Utility::getDaysUntilExpiration(cert.get()));
-}
+/*


I will revert this once #4701 is submitted.

jmarantz · 2018-10-12T15:10:54Z

RE " internal-only helper functions, ... own set of files" -- I am not sure exactly what you are suggesting. We could have a separate file for stat_name, but in general I've put the functions that will be needed in the stats system as public, and the ones that that are needed only internal to the symbol table as private. All the non-trivial impls are in the .cc file.

Can you be more specific about what's exposed in a public function in the .h that shouldn't be?

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

ambuc · 2018-10-12T16:54:35Z

You're right -- I looked at the header again and it's plenty differentiated, nvm on that.

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

jmarantz · 2018-10-12T19:48:00Z

@ambuc any more comments?

Any senior committers want to start taking a look at some fun bit-hacking?

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

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

… can be shared with tests for ThreadLocalStore improvements. 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>

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

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

stale · 2018-10-20T19:08:17Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

jmarantz · 2018-10-20T19:34:40Z

Ping

…

On Sat, Oct 20, 2018, 3:08 PM stale[bot] ***@***.*** wrote: This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4696 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB2kPc9QCgxXAvElEjTagdORt6rWIKZ4ks5um3S2gaJpZM4XYe42> .

mattklein123 · 2018-10-21T02:37:41Z

I will review.

jmarantz · 2018-10-21T17:53:10Z

If there are follow-up edits I hope to get to them on Wednesday.... Don't have access to my computer now. I was pinging just to avoid stalebot. The benchmark PR would be good to get in first anyway, which would show more clearly the incremental benefit of this change to StatName.

…

On Sat, Oct 20, 2018, 10:37 PM Matt Klein ***@***.*** wrote: I will review. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4696 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB2kPd5RwZhhYmfbwe7qVMp4mo-rmt9Iks5um94DgaJpZM4XYe42> .

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>

…le-uint8-array Signed-off-by: Joshua Marantz <jmarantz@google.com>

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

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

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

jmarantz · 2018-10-29T00:16:23Z

this should be ready to go; ptal.

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

mattklein123

At a high level this makes sense to me. Some random comments from a read through. Cool stuff!

mattklein123 · 2018-10-29T18:20:14Z

+  /**
+   * Decodes a uint8_t array into a SymbolVec.
+   */
+  static SymbolVec decodeSymbols(const SymbolStorage array, size_t size);


nit: generally prefer explicit types. I would probably replace size_t everywhere with either uint32_t or uint64_t

OK, done, but WDYT about loop variables when looping until you hit stl_container.size()?

Sure that's fine.

mattklein123 · 2018-10-29T18:26:37Z

+   *                 overhead for the size itself.
+   */
+  size_t numBytes() const {
+    return symbol_array_[0] | (static_cast<size_t>(symbol_array_[1]) << 8);


Do we care about endian-ness here? Can we just cast/store the 2 bytes and then retrieve them? Might be simpler to read?

No not endianness, but alignment. In a future PR modeled off my experiments I was going to put all the StatNames needed for a Metric (tag-extracted name, and each tag and value) into a single allocated block. Will comment.

mattklein123 · 2018-10-29T18:34:53Z

+    next_symbol_ = pool_.top();
+    pool_.pop();
+  }
+  // This should catch integer overflow for the new symbol.


What happens if we do overflow? Can this happen with a very long running server? Sorry I didn't originally review this code so am unfamiliar with the details.

nothing good will happen :). But we'd probably also run out of memory before we allocate 4B symbols (say averaging 32 bytes including std::string overhead).

Note that symbols are reference-counted and recycled, so there would have to be 4B referenced symbols.

mattklein123 · 2018-10-29T18:35:52Z

+}
+
+StatNameStorage::~StatNameStorage() {
+  // StatNameStorage is not fully RAII: you must call free(SymbolTable&) to


Sorry how does this save 8 bytes?

If we want to make this RAII we need to store a reference to the SymbolTable& which would require 8 bytes overhead per StatNameStorage instance. Added comment.

mattklein123 · 2018-10-29T18:37:10Z

+uint8_t* StatNameJoiner::alloc(size_t num_bytes) {
+  bytes_.reset(new uint8_t[num_bytes + 2]);
+  uint8_t* p = bytes_.get();
+  *p++ = num_bytes & 0xff;


uint16_t size assign?

factored out length-assigner and commented there why this can't be done via uint16_t.

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

jmarantz added 11 commits October 10, 2018 16:36

Typically tests in Envoy for files named foo_impl.cc are called foo_i…

d3da71d

…mpl_test.cc. This just corrects that for two tests. Signed-off-by: Joshua Marantz <jmarantz@google.com>

Also make tag_extractor and tag_producer consistent.

8b9e031

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

Use uint8_t encoding for symbol arrays, and rely on asserts rather th…

1a5c329

…an RAII to avoid leaks. The RAII concept is nice but it's expensive; need to keep 8-byte SymbolTable reference with each stat-name. Signed-off-by: Joshua Marantz <jmarantz@google.com>

further naming clarification.

b5f3b9a

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

format

40be827

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

test sorting and hashing.

be0a1a9

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

add a test for memory usage.

28ad381

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

Merge branch 'master' into symbol-table-uint8-array

b8ed101

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

clean up comment and remove some extra layers from the API.

2133302

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

more commentary

d32e91e

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

Don't run malloc-stats-dependent comparisons on tsan/asan/mac.

55f50b1

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

Uncomment symbol-table tests I had temporarily disabled during dev, a…

c7d6749

…nd comment out broken SSL test for now. See envoyproxy#4703 Signed-off-by: Joshua Marantz <jmarantz@google.com>

ambuc reviewed Oct 12, 2018

View reviewed changes

Comment thread source/common/stats/symbol_table_impl.h Outdated

Fix unreferenced variable (we needed only the side-effect of its init…

cbebe4b

…ializtion) and clean up stale pure virtual interface. Signed-off-by: Joshua Marantz <jmarantz@google.com>

jmarantz commented Oct 12, 2018

View reviewed changes

Address review comments and add a comment out efficiency in lessThan().

88fddc6

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

jmarantz added 2 commits October 12, 2018 13:45

back out hack to ssl test.

c4bb510

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

Merge branch 'master' into symbol-table-uint8-array

54cb7d3

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

jmarantz added 5 commits October 12, 2018 15:51

Finish comment that evidently I had lost interest in midway through t…

eddfd87

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

Add baseline test.

d5cff26

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

Factor out the enumeration of stats names into a helper library so it…

ab39832

… can be shared with tests for ThreadLocalStore improvements. 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 ~~stats: Use a more compact rep for SymbolTable and add tests for memory usage~~ WIP stats: Use a more compact rep for SymbolTable and add tests for memory usage Oct 13, 2018

jmarantz added 6 commits October 13, 2018 14:26

Merge branch 'stats-mem-tests' into symbol-table-uint8-array

978036d

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

Merge branch 'master' into stats-mem-tests

1a59a29

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

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>

stale Bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 20, 2018

stale Bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 20, 2018

mattklein123 self-assigned this Oct 21, 2018

jmarantz added 4 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 symbol-tab…

eee26cf

…le-uint8-array Signed-off-by: Joshua Marantz <jmarantz@google.com>

Merge branch 'master' into symbol-table-uint8-array

85736b5

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

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

jmarantz added 2 commits October 28, 2018 20:09

Add class to help do joins for StatNames that do not need to lock the…

2964000

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

Merge branch 'master' into symbol-table-uint8-array

6c7bfd9

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

jmarantz changed the title ~~WIP stats: Use a more compact rep for SymbolTable and add tests for memory usage~~ stats: Use a more compact rep for SymbolTable and add tests for memory usage Oct 29, 2018

Don't memcpy an empty vector.

5b43f34

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

mattklein123 requested changes Oct 29, 2018

View reviewed changes

jmarantz added 2 commits October 29, 2018 14:52

clang-tidy cleanup

c37d522

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

address review comments.

940f980

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

mattklein123 approved these changes Oct 29, 2018

View reviewed changes

mattklein123 merged commit 1bdc95a into envoyproxy:master Oct 29, 2018

jmarantz deleted the symbol-table-uint8-array branch October 29, 2018 22:16

Conversation

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

Uh oh!

Uh oh!

jmarantz commented Oct 12, 2018

Uh oh!

Uh oh!

ambuc commented Oct 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ambuc commented Oct 12, 2018

Uh oh!

jmarantz commented Oct 12, 2018

Uh oh!

stale Bot commented Oct 20, 2018

Uh oh!

jmarantz commented Oct 20, 2018 via email

Uh oh!

mattklein123 commented Oct 21, 2018

Uh oh!

jmarantz commented Oct 21, 2018 via email

Uh oh!

jmarantz commented Oct 29, 2018

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

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!

3 participants

jmarantz commented Oct 11, 2018 •

edited

Loading

jmarantz commented Oct 12, 2018 •

edited

Loading