Fix issue with Envoy not reference counting across scopes under not-hot restart#3212
Closed
ambuc wants to merge 20 commits intoenvoyproxy:masterfrom
Closed
Fix issue with Envoy not reference counting across scopes under not-hot restart#3212ambuc wants to merge 20 commits intoenvoyproxy:masterfrom
ambuc wants to merge 20 commits intoenvoyproxy:masterfrom
Conversation
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
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>
ASAN does not allow *(nullptr) being converted to a reference. Fixes envoyproxy#3208 Signed-off-by: Greg Greenway <ggreenway@apple.com>
…y#3140)" (envoyproxy#3213) This reverts commit e293ffb. Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
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>
jmarantz
reviewed
Apr 27, 2018
| TEST(RawStatDataTest, HeapAlloc) { | ||
| HeapRawStatDataAllocator alloc; | ||
| RawStatData* stat_1 = alloc.alloc("ref_name"); | ||
| RawStatData* stat_2 = alloc.alloc("ref_name"); |
Contributor
There was a problem hiding this comment.
maybe also do stat_3 with "not_ref_name" and check it's not equal, and that none of these are nullptr.
source/common/stats/stats_impl.cc
Outdated
| key.size(), Stats::RawStatData::maxNameLength()); | ||
| } | ||
|
|
||
| auto ret = stats_set_.insert(std::pair<std::string, RawStatData*>(std::string(key), nullptr)); |
Contributor
There was a problem hiding this comment.
I'd specify a typedef StringRawDataMap for the map type in the header, and then use StringRawDataMap::value_type here rather than spelling out the pair.
| auto ret = stats_set_.insert(std::pair<std::string, RawStatData*>(std::string(key), nullptr)); | ||
| RawStatData*& data = ret.first->second; | ||
| if (ret.second) { | ||
| data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1)); |
test/common/stats/stats_impl_test.cc
Outdated
| RawStatData* stat_1 = alloc.alloc("ref_name"); | ||
| RawStatData* stat_2 = alloc.alloc("ref_name"); | ||
| EXPECT_EQ(stat_1, stat_2); | ||
| ::free(stat_1); |
Contributor
There was a problem hiding this comment.
alloc.free(stat_1);
that's probably the source of the CI failures
jmarantz
reviewed
Apr 27, 2018
Contributor
jmarantz
left a comment
There was a problem hiding this comment.
mostly nits except the test bug.
…uc/envoy into refcount-stats-in-heap-alloc Signed-off-by: James Buckland <jbuckland@google.com>
Signed-off-by: James Buckland <jbuckland@google.com>
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Signed-off-by: James Buckland jbuckland@google.com
title: Fixes issue with Envoy not reference counting stats across scopes under not-hot restart.
Description: Simpler solution to issue #2453 than pull #3163, continuing draft work in ambuc#1 and ambuc#2. Summary of changes:
stats_set_as a member variable ofHeapRawStatDataAllocator, 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