Fix issue with Envoy not reference counting across scopes under not-hot restart by ambuc · Pull Request #3249 · envoyproxy/envoy

ambuc · 2018-04-27T21:22:48Z

Signed-off-by: James Buckland jbuckland@google.com

title: Fixes issue with Envoy not reference counting stats across scopes under not-hot restart. Re-opened PR of #3212 due to a revert / DCO conflict.

Description: Simpler solution to issue #2453 than pull #3163, continuing draft work in ambuc#1 and ambuc#2. Summary of changes:

adds an unordered_map named stats_set_ as a member variable of HeapRawStatDataAllocator, and implements reference counting / dedup on allocated stats.

Risk Level: Low.

Testing: Add a test to stats_impl_test. Passes bazel test test/....

Docs Changes: N/A

Release Notes: This is user-facing in that non-hot restart stat allocation now resolves namespace properly, but no effect on user configs.

Fixes: #2453

Signed-off-by: James Buckland <jbuckland@google.com>

…ators Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

…ators Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

…uc/envoy into refcount-stats-in-heap-alloc Signed-off-by: James Buckland <jbuckland@google.com>

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32

Mostly looks good! Left a few nits.

mrice32 · 2018-04-28T02:24:58Z

source/common/stats/stats_impl.h

  void free(RawStatData& data) override;
+
+private:
+  StringRawDataMap stats_set_;


nit: since std::set is a different data structure entirely, can we name this something like stats_map_ or just stats_?

mrice32 · 2018-04-28T02:27:21Z

source/common/stats/stats_impl.h

 class HeapRawStatDataAllocator : public RawStatDataAllocator {
 public:
  // RawStatDataAllocator
+  typedef std::unordered_map<std::string, RawStatData*> StringRawDataMap;


nit: since no other classes need to know about this typedef, do you think it makes sense to make it private?

mrice32 · 2018-04-28T02:42:21Z

source/common/stats/stats_impl.cc

+    return;
+  }
+
+  size_t key_removed = stats_set_.erase(std::string(data.key()));


We expect all stats to call freed before the allocator object disappears, so I would suggest adding a destructor with an ASSERT to check that the map is empty.

mrice32

@ambuc, took a quick look at the TSAN failure. Looks like we may need a lock protecting both methods (like the implementation in the hot restart allocator) because the calls to free(), which come from the destructors of the stat objects, are not protected at the callsite like the alloc() calls coming from the Scope are.

jmarantz · 2018-04-28T12:16:56Z

source/common/stats/stats_impl.cc

+  auto ret = stats_set_.insert(StringRawDataMap::value_type(std::string(key), nullptr));
+  RawStatData*& data = ret.first->second;
+  if (ret.second) {
+    data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));


This looks correct, but it duplicates the required key storage in the map key.

If you make this a set<RawStatData*, RawStatDataHash, RawStatDataCompare> rather than a map, then the hasher and comparator could reference the key stored in the RawStatData, and available as a string_view via RawStatData::key().

Before calling set insertion you'd have to prospectively calloc the ptr and initialize() it, and then free it if turned out to be a dup. That seems better than duplicating the storage. And you'd have to make the trivial functors for hashing and comparison.

Then you could remove also the duplicated length check above, since it would be done in initialize().

I think I +1'd too soon. It seems strange to alloc up an object just to be able to tell if the key exists, and then throwing it away if not. Duplicating the key or doing the truncation of the key before so we can check the set before allocating the object seems better IMO. It seems that now we've added a custom hash function, custom comparitor, and a somewhat complex set addition logic just to remove the duplicate storage of the key. Is there a particular reason that you think the way you suggested is more readable or performant?

RawStatData::initialize() literally just does a memcpy of the bytes, so it's basically the same cost as making the prospective copy of the string you need to do the map lookup.

The syntax for making a custom hash/compare in STL is a little annoying, but I don't think it's that bad. I'm not following you about set-addition logic, I think it should be about equivalent, but it's (IMO) better to do the truncation in one place, and I can't judge exactly the cost of duplicating all stats at scale, but with this option it's zero :)

SGTM. Not a huge fan of the additional complexity around the additional allocation logic and stl munging, but your perf point is reasonable. I'm don't think getting rid of these additional allocations on successful lookups would be worth all of the wasted memory in the normal map case. And that seems to be the only choices here.

Just a side note: we don't do the truncation in one place - we do it in two. key() truncates when the key is extracted. In the hot restart allocator, we do it three times: at the callsite, in initialize, and when the key is extracted. We should probably fix this in a later PR.

jmarantz · 2018-04-28T12:18:03Z

test/common/stats/stats_impl_test.cc

+  RawStatData* stat_3 = alloc.alloc("not_ref_name");
+  EXPECT_EQ(stat_1, stat_2);
+  EXPECT_NE(stat_1, stat_3);
+  EXPECT_NE(stat_2, stat_3);


let's just expect stat_3 is not nullptr too, though it looks like that would segv below anyway.

jmarantz · 2018-04-28T12:45:19Z

source/common/stats/stats_impl.cc

+        key.size(), Stats::RawStatData::maxNameLength());
+  }
+
+  auto ret = stats_set_.insert(StringRawDataMap::value_type(std::string(key), nullptr));


I think stats_set_ needs a mutex.

Signed-off-by: James Buckland <jbuckland@google.com>

jmarantz · 2018-05-01T16:52:10Z

source/common/stats/stats_impl.cc

-  data->initialize(name);
-  return data;
+  data->initialize(key);
+  auto ret = stats_.insert(data);


you can take the lock after the call to initialize(), to minimize the time spent holding the lock. Actually I think you can also let it go immediately after the call to insert as well as ref_count_ is atomic.

jmarantz · 2018-05-01T16:52:46Z

source/common/stats/stats_impl.cc

  RawStatData* data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));
-  data->initialize(name);
-  return data;
+  data->initialize(key);


you can just pass 'name' in here, no need for the temp key.

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 · 2018-05-01T17:23:42Z

source/common/stats/stats_impl.cc

+  auto ret = stats_set_.insert(StringRawDataMap::value_type(std::string(key), nullptr));
+  RawStatData*& data = ret.first->second;
+  if (ret.second) {
+    data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));


SGTM. Not a huge fan of the additional complexity around the additional allocation logic and stl munging, but your perf point is reasonable. I'm don't think getting rid of these additional allocations on successful lookups would be worth all of the wasted memory in the normal map case. And that seems to be the only choices here.

Just a side note: we don't do the truncation in one place - we do it in two. key() truncates when the key is extracted. In the hot restart allocator, we do it three times: at the callsite, in initialize, and when the key is extracted. We should probably fix this in a later PR.

mrice32 · 2018-05-01T17:29:16Z

source/common/stats/stats_impl.cc

-  // This must be zero-initialized
+  std::unique_lock<std::mutex> lock(mutex_);
+
+  absl::string_view key = name;


nit: I don't think you need this line anymore since you can just pass name to initialize directly (string_view will implicitly be constructed from a string IIUC).

mrice32 · 2018-05-01T17:37:59Z

source/common/stats/stats_impl.cc

+
+  std::unique_lock<std::mutex> lock(mutex_);
+  auto ret = stats_.insert(data);
+  lock.unlock();


There's a subtle problem here. The iterator you were returned can be invalidated if another element is inserted into the set. You need to grab the raw pointer from the iterator while locked, and never use the iterator again after unlocking.

Oh very good point Matt. Just leave it locked till the end of the function then. I can't think of why the hash implementation would need to invalidate the iterator but if the standard doesn't say it is safe then there is no point risking it.

I think the lock tightening is a good idea and will probably save us some cycles. We just need to extract the raw pointer from the iterator while locked and only use the pointer temporary after we unlock.

As for how this interacts with a custom hash, my basic understanding is that as the set grows, it will at some point decide to rehash (using the same hash function) the entire set onto a larger set of buckets. This is the only process that causes iterators to be invalidated for std::unordered_set.

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 · 2018-05-01T21:13:29Z

source/common/stats/stats_impl.cc

+
+  std::unique_lock<std::mutex> lock(mutex_);
+  auto ret = stats_.insert(data);
+  RawStatData* existingData = *ret.first;


nit: existing_data

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32

LGTM - thanks!

mattklein123

LGTM, just small nit/Q

mattklein123 · 2018-05-02T07:46:53Z

source/common/stats/stats_impl.cc

+  size_t key_removed = stats_.erase(&data);
+  lock.unlock();
+
+  ASSERT(key_removed >= 1);


nit: Shouldn't this be == ?

Oops, good catch. Fixed in 87318c0.

Signed-off-by: James Buckland <jbuckland@google.com>

ggreenway · 2018-05-02T16:20:02Z

Mac test failure. I can't tell if it's related to this change or not.

[ RUN      ] TestParameters/UdsUpstreamIntegrationTest.RouterDownstreamDisconnectBeforeResponseComplete/0
[2018-05-02 16:10:36.593][155368][critical][assert] source/common/stats/thread_local_store.cc:103] assert failure: !merge_in_progress_
[2018-05-02 16:10:36.593][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:114] Caught Abort trap: 6, suspect faulting address 0x7fff759f7e3e
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:87] Backtrace obj<uds_integration_test> thr<123145453625344>:
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:99] thr<123145453625344> obj<uds_integration_test                0x000000010e6ff1b6 _ZN8backward7details6unwindINS_14StackTraceImplINS_10system_tag10darwin_tagEE8callbackEEEmT_m>
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:105] thr<123145453625344> #0 0x10e6ff1b6: 
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:99] thr<123145453625344> obj<uds_integration_test>
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:105] thr<123145453625344> #1 0x10e6fed25: backward::StackTraceImpl<backward::system_tag::darwin_tag>::load_here(unsigned long) + 101
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:105] thr<123145453625344> #2 0x10e6feb21: backward::StackTraceImpl<backward::system_tag::darwin_tag>::load_from(void*, unsigned long) + 49
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:105] thr<123145453625344> #3 0x10e6fd07e: Envoy::BackwardsTrace::captureFrom(void*) + 46
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:105] thr<123145453625344> #4 0x10e6fcf3f: Envoy::SignalAction::sigHandler(int, __siginfo*, void*) + 143
[2018-05-02 16:10:36.605][155368][critical][backtrace] bazel-out/darwin-fastbuild/bin/source/server/_virtual_includes/backtrace_lib/server/backtrace.h:110] end backtrace thread 123145453625344

Signed-off-by: James Buckland <jbuckland@google.com>

htuch

Thanks for adding this, surprising how simple it ended up. A few questions.

htuch · 2018-05-03T02:16:41Z

source/common/stats/stats_impl.h

+    }
+  };
+  typedef std::unordered_set<RawStatData*, RawStatDataHash_, RawStatDataCompare_> StringRawDataSet;
+  StringRawDataSet stats_;


Please add comments explaining what this is/does.

htuch · 2018-05-03T02:17:06Z

source/common/stats/stats_impl.h

+  };
+  typedef std::unordered_set<RawStatData*, RawStatDataHash_, RawStatDataCompare_> StringRawDataSet;
+  StringRawDataSet stats_;
+  std::mutex mutex_;


Please add comments explaining what htis protects. Ideally we use GUARDED_BY etc. macros.

htuch · 2018-05-03T02:20:24Z

source/common/stats/stats_impl.cc

+    return;
+  }
+
+  std::unique_lock<std::mutex> lock(mutex_);


Why do we need locking at all if the old comment about "This allocator does not ever have concurrent access to the raw data" hold true?

The problem isn't the stat, it's the set.

+1. Also, side note: that comment is no longer valid since the same stat can be freed/allocated multiple times, meaning that there may be cases where the allocator is operating on the same raw stat from two different threads.

AFAICT we only ever do these allocations under existing locking, e.g.

envoy/source/common/stats/thread_local_store.cc

Line 192 in 872728d

SafeAllocData alloc = parent_.safeAlloc(final_name);

. Do we need to be double locking here?

I might be wrong in my assessment, please point out if not (and add a comment to the code!).

Yes, good point, there probably need to be comments around this. The alloc() calls are protected, but the free() calls are made from the destructors of the individual stat objects. See https://github.com/envoyproxy/envoy/blob/master/source/common/stats/stats_impl.h#L310 for an example.

htuch · 2018-05-03T02:21:45Z

source/common/stats/stats_impl.cc

  data->initialize(name);
-  return data;
+
+  std::unique_lock<std::mutex> lock(mutex_);


Why are we allocating and then freeing on the case where we have an existing stat?

Good question. Fundamentally it doesn't have to be this way, but this is an artifact of the way STL sets & maps work. STL set lookups require construction of an object. If you make this an map<string, RawStatData*> you have just pushed the problem around a little, as you'd need to copy the string to potentially truncate it, which is the same work, basically, as is being done here, and then you have to duplicate the truncation logic instead of just having it in RawStatData::initialize. Worse, you'd wind up permanently duplicating all the name storage. I argued that I don't know really how impactful that would be across different ways you might scale the system, but the current solution has zero overhead from duplication and is really no more complex from a programming perspective.

One question to ask is whether RawStatData::initialize is doing anything extra that's not required for the set lookup. It is, but it's pretty minimal and IMO not worth optimizing around.

An ideal solution would allow the set lookup against a string_view, without actually constructing the templated type. BlockMemoryHashSet::insert has that signature, so in the hot-restart case you don't need to do the prospective allocation.

Yeah, fair enough. Maybe add a comment to the code capturing this design history. Thanks!

htuch

Clearing approved status for now.

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 · 2018-05-04T14:39:06Z

source/common/stats/stats_impl.cc

+  // storing the name twice. Performing a lookup on the set is similarly
+  // expensive to performing a map lookup, since both require allocating a
+  // RawStatData object and a writing a string.
+


nit: extra line.

Signed-off-by: James Buckland <jbuckland@google.com>

htuch

Thanks!

ambuc added 18 commits April 25, 2018 16:40

First working draft of simpler reference-counted heap allocator

3d5849d

Signed-off-by: James Buckland <jbuckland@google.com>

Fixed reference to pointer to RawStatData

b6aa029

Signed-off-by: James Buckland <jbuckland@google.com>

Add HeapAlloc test for HeapRawStatDataAllocator

d615eef

Signed-off-by: James Buckland <jbuckland@google.com>

Initialized data based on truncated name

cdd8418

Signed-off-by: James Buckland <jbuckland@google.com>

Remove leftover comments

101c7aa

Signed-off-by: James Buckland <jbuckland@google.com>

Initialized data based on non-truncated name

282c56b

Signed-off-by: James Buckland <jbuckland@google.com>

Initialized data based on truncated name, and also warn in both alloc…

830c718

…ators Signed-off-by: James Buckland <jbuckland@google.com>

Remove leftover couts

4de411a

Signed-off-by: James Buckland <jbuckland@google.com>

First working draft of simpler reference-counted heap allocator

11a8cdb

Signed-off-by: James Buckland <jbuckland@google.com>

Fixed reference to pointer to RawStatData

ecd7714

Signed-off-by: James Buckland <jbuckland@google.com>

Add HeapAlloc test for HeapRawStatDataAllocator

8c00102

Signed-off-by: James Buckland <jbuckland@google.com>

Initialized data based on truncated name

ddcb0f9

Signed-off-by: James Buckland <jbuckland@google.com>

Remove leftover comments

7031caa

Signed-off-by: James Buckland <jbuckland@google.com>

Initialized data based on non-truncated name

93eb3d7

Signed-off-by: James Buckland <jbuckland@google.com>

Initialized data based on truncated name, and also warn in both alloc…

b4b1b39

…ators Signed-off-by: James Buckland <jbuckland@google.com>

Remove leftover couts

7e2b8d6

Signed-off-by: James Buckland <jbuckland@google.com>

Merge branch 'refcount-stats-in-heap-alloc' of https://github.com/amb…

96582ad

…uc/envoy into refcount-stats-in-heap-alloc Signed-off-by: James Buckland <jbuckland@google.com>

Fix free stat issue, typedef datamap, assorted

bec4fac

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc mentioned this pull request Apr 27, 2018

Fix issue with Envoy not reference counting across scopes under not-hot restart #3212

Closed

mrice32 reviewed Apr 28, 2018

View reviewed changes

mattklein123 assigned mrice32 and jmarantz Apr 28, 2018

jmarantz reviewed Apr 28, 2018

View reviewed changes

ambuc added 3 commits April 30, 2018 15:22

Add mutex to HeapRawStatDataAllocator

ea27f46

Signed-off-by: James Buckland <jbuckland@google.com>

Rename mutex, make non-mutable

a4eb61e

Signed-off-by: James Buckland <jbuckland@google.com>

Use unordered_set<> for HeapRawStatDataAllocator's stats

b388492

Signed-off-by: James Buckland <jbuckland@google.com>

jmarantz approved these changes May 1, 2018

View reviewed changes

Tighten mutex; remove temp key

3b9d839

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 reviewed May 1, 2018

View reviewed changes

Minimize time spent holding lock

b10f663

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 reviewed May 1, 2018

View reviewed changes

ambuc added 4 commits May 1, 2018 17:15

Fix camelCase style issue

759e6d9

Signed-off-by: James Buckland <jbuckland@google.com>

Add more robust null checking around RawDataTest/Truncate

8a8a39d

Signed-off-by: James Buckland <jbuckland@google.com>

Change EXPECTs to ASSERTs in RawStatDataTest/Truncate

f0e19ae

Signed-off-by: James Buckland <jbuckland@google.com>

Tighten mutex around HeapRawStatDataAllocator::free()

76ebd08

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 approved these changes May 1, 2018

View reviewed changes

mattklein123 reviewed May 2, 2018

View reviewed changes

More specific asserts around unordered_map::erase behavior

87318c0

Signed-off-by: James Buckland <jbuckland@google.com>

mattklein123 previously approved these changes May 2, 2018

View reviewed changes

Merge branch 'master' into refcount-stats-in-heap-alloc

84913f9

Signed-off-by: James Buckland <jbuckland@google.com>

htuch reviewed May 3, 2018

View reviewed changes

htuch suggested changes May 3, 2018

View reviewed changes

Add GUARDED_BY to mutex_, documentation around StringRawDataSet

5194ec2

Signed-off-by: James Buckland <jbuckland@google.com>

ambuc dismissed mattklein123’s stale review via 5194ec2 May 3, 2018 14:44

ambuc and others added 3 commits May 3, 2018 10:48

Merge branch 'master' into refcount-stats-in-heap-alloc

ce99186

Formatting updates

5429301

Signed-off-by: James Buckland <jbuckland@google.com>

Add documentation for HeapRawStatDataAllocator

40ae83b

Signed-off-by: James Buckland <jbuckland@google.com>

mrice32 reviewed May 4, 2018

View reviewed changes

Edits for documentation for HeapRawStatDataAllocator

20526e7

Signed-off-by: James Buckland <jbuckland@google.com>

jmarantz approved these changes May 4, 2018

View reviewed changes

htuch approved these changes May 4, 2018

View reviewed changes

htuch merged commit 795848b into envoyproxy:master May 4, 2018

ambuc deleted the refcount-stats-in-heap-alloc branch May 7, 2018 12:24

Conversation

ambuc commented Apr 27, 2018

Uh oh!

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

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

mrice32 May 1, 2018 • 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!

jmarantz May 1, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

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

ggreenway commented May 2, 2018

Uh oh!

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

mrice32 May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mrice32 left a comment •

edited

Loading

mrice32 May 1, 2018 •

edited

Loading

jmarantz May 1, 2018 •

edited

Loading

mrice32 May 3, 2018 •

edited

Loading