Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
htuch merged 34 commits into from
May 4, 2018
Merged

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

Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
3d5849d
First working draft of simpler reference-counted heap allocator
ambuc Apr 25, 2018
b6aa029
Fixed reference to pointer to RawStatData
ambuc Apr 25, 2018
d615eef
Add HeapAlloc test for HeapRawStatDataAllocator
ambuc Apr 25, 2018
cdd8418
Initialized data based on truncated name
ambuc Apr 25, 2018
101c7aa
Remove leftover comments
ambuc Apr 25, 2018
282c56b
Initialized data based on non-truncated name
ambuc Apr 25, 2018
830c718
Initialized data based on truncated name, and also warn in both alloc…
ambuc Apr 25, 2018
4de411a
Remove leftover couts
ambuc Apr 25, 2018
11a8cdb
First working draft of simpler reference-counted heap allocator
ambuc Apr 25, 2018
ecd7714
Fixed reference to pointer to RawStatData
ambuc Apr 25, 2018
8c00102
Add HeapAlloc test for HeapRawStatDataAllocator
ambuc Apr 25, 2018
ddcb0f9
Initialized data based on truncated name
ambuc Apr 25, 2018
7031caa
Remove leftover comments
ambuc Apr 25, 2018
93eb3d7
Initialized data based on non-truncated name
ambuc Apr 25, 2018
b4b1b39
Initialized data based on truncated name, and also warn in both alloc…
ambuc Apr 25, 2018
7e2b8d6
Remove leftover couts
ambuc Apr 25, 2018
96582ad
Merge branch 'refcount-stats-in-heap-alloc' of https://github.com/amb…
ambuc Apr 27, 2018
bec4fac
Fix free stat issue, typedef datamap, assorted
ambuc Apr 27, 2018
ea27f46
Add mutex to HeapRawStatDataAllocator
ambuc Apr 30, 2018
a4eb61e
Rename mutex, make non-mutable
ambuc Apr 30, 2018
b388492
Use unordered_set<> for HeapRawStatDataAllocator's stats
ambuc May 1, 2018
3b9d839
Tighten mutex; remove temp key
ambuc May 1, 2018
b10f663
Minimize time spent holding lock
ambuc May 1, 2018
759e6d9
Fix camelCase style issue
ambuc May 1, 2018
8a8a39d
Add more robust null checking around RawDataTest/Truncate
ambuc May 1, 2018
f0e19ae
Change EXPECTs to ASSERTs in RawStatDataTest/Truncate
ambuc May 1, 2018
76ebd08
Tighten mutex around HeapRawStatDataAllocator::free()
ambuc May 1, 2018
87318c0
More specific asserts around unordered_map::erase behavior
ambuc May 2, 2018
84913f9
Merge branch 'master' into refcount-stats-in-heap-alloc
ambuc May 2, 2018
5194ec2
Add GUARDED_BY to mutex_, documentation around StringRawDataSet
ambuc May 3, 2018
ce99186
Merge branch 'master' into refcount-stats-in-heap-alloc
ambuc May 3, 2018
5429301
Formatting updates
ambuc May 3, 2018
40ae83b
Add documentation for HeapRawStatDataAllocator
ambuc May 4, 2018
20526e7
Edits for documentation for HeapRawStatDataAllocator
ambuc May 4, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 28 additions & 7 deletions source/common/stats/stats_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,9 +150,25 @@ bool TagExtractorImpl::extractTag(const std::string& stat_name, std::vector<Tag>
}

RawStatData* HeapRawStatDataAllocator::alloc(const std::string& name) {
// This must be zero-initialized
RawStatData* data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));
data->initialize(name);
absl::string_view key = name;

@mrice32 mrice32 May 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


if (key.size() > Stats::RawStatData::maxNameLength()) {
key.remove_suffix(key.size() - Stats::RawStatData::maxNameLength());
ENVOY_LOG_MISC(
warn,
"Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key,
key.size(), Stats::RawStatData::maxNameLength());
}

auto ret = stats_set_.insert(StringRawDataMap::value_type(std::string(key), nullptr));

@jmarantz jmarantz Apr 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think stats_set_ needs a mutex.

RawStatData*& data = ret.first->second;
if (ret.second) {
data = static_cast<RawStatData*>(::calloc(RawStatData::size(), 1));

@jmarantz jmarantz Apr 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

@mrice32 mrice32 Apr 30, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1.

@mrice32 mrice32 May 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@jmarantz jmarantz May 1, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

@mrice32 mrice32 May 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

RELEASE_ASSERT(data);
data->initialize(key);
} else {
++data->ref_count_;
}
return data;
}

Expand Down Expand Up @@ -256,18 +272,23 @@ TagProducerImpl::addDefaultExtractors(const envoy::config::metrics::v2::StatsCon
}

void HeapRawStatDataAllocator::free(RawStatData& data) {
// This allocator does not ever have concurrent access to the raw data.
ASSERT(data.ref_count_ == 1);
ASSERT(data.ref_count_ > 0);
if (--data.ref_count_ > 0) {
return;
}

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

@mrice32 mrice32 Apr 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ASSERT(key_removed >= 1);

@mattklein123 mattklein123 May 2, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Shouldn't this be == ?

@ambuc ambuc May 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, good catch. Fixed in 87318c0.

::free(&data);
}

void RawStatData::initialize(absl::string_view key) {
ASSERT(!initialized());
if (key.size() > maxNameLength()) {
if (key.size() > Stats::RawStatData::maxNameLength()) {
ENVOY_LOG_MISC(
warn,
"Statistic '{}' is too long with {} characters, it will be truncated to {} characters", key,
key.size(), maxNameLength());
key.size(), Stats::RawStatData::maxNameLength());
}
ref_count_ = 1;

Expand Down
4 changes: 4 additions & 0 deletions source/common/stats/stats_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -412,8 +412,12 @@ class HistogramImpl : public Histogram, public MetricImpl {
class HeapRawStatDataAllocator : public RawStatDataAllocator {
public:
// RawStatDataAllocator
typedef std::unordered_map<std::string, RawStatData*> StringRawDataMap;

@mrice32 mrice32 Apr 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

RawStatData* alloc(const std::string& name) override;
void free(RawStatData& data) override;

private:
StringRawDataMap stats_set_;

@mrice32 mrice32 Apr 28, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

};

/**
Expand Down
13 changes: 13 additions & 0 deletions test/common/stats/stats_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -478,5 +478,18 @@ TEST(RawStatDataTest, Truncate) {
alloc.free(*stat);
}

TEST(RawStatDataTest, HeapAlloc) {
HeapRawStatDataAllocator alloc;
RawStatData* stat_1 = alloc.alloc("ref_name");
RawStatData* stat_2 = alloc.alloc("ref_name");
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);

@jmarantz jmarantz Apr 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

alloc.free(*stat_1);
alloc.free(*stat_2);
alloc.free(*stat_3);
}

} // namespace Stats
} // namespace Envoy