Skip to content

Avoid extra predicate() calls #3869

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
mattklein123 merged 6 commits into from
Jul 18, 2018
Merged

Avoid extra predicate() calls #3869

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17f9169
Avoid extra `predicate()` calls
Jul 16, 2018
5efbba7
Remove unused variable
Jul 16, 2018
4873108
Fix/clarify comment.
Jul 16, 2018
5fd8886
Avoid filter() when we only have 1 locality
Jul 17, 2018
8c98c89
Inline most of Config::Metadata::metadataValue()
Jul 17, 2018
4df5647
Remove unneeded dereferences
Jul 17, 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
48 changes: 40 additions & 8 deletions source/common/upstream/subset_lb.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,12 +275,23 @@ void SubsetLoadBalancer::update(uint32_t priority, const HostVector& hosts_added

bool SubsetLoadBalancer::hostMatches(const SubsetMetadata& kvs, const Host& host) {
const envoy::api::v2::core::Metadata& host_metadata = *host.metadata();
const auto filter_it =
host_metadata.filter_metadata().find(Config::MetadataFilters::get().ENVOY_LB);

if (filter_it == host_metadata.filter_metadata().end()) {
return kvs.size() == 0;
}

const ProtobufWkt::Struct& data_struct = filter_it->second;
const auto& fields = data_struct.fields();

for (const auto& kv : kvs) {
const ProtobufWkt::Value& host_value = Config::Metadata::metadataValue(
host_metadata, Config::MetadataFilters::get().ENVOY_LB, kv.first);
const auto entry_it = fields.find(kv.first);
if (entry_it == fields.end()) {
return false;
}

if (!ValueUtil::equal(host_value, kv.second)) {
if (!ValueUtil::equal(entry_it->second, kv.second)) {
return false;
}
}
Expand Down Expand Up @@ -460,9 +471,12 @@ SubsetLoadBalancer::PrioritySubsetImpl::PrioritySubsetImpl(const SubsetLoadBalan
void SubsetLoadBalancer::HostSubsetImpl::update(const HostVector& hosts_added,
const HostVector& hosts_removed,
std::function<bool(const Host&)> predicate) {
std::unordered_set<HostSharedPtr> predicate_added;

HostVector filtered_added;
for (const auto host : hosts_added) {
if (predicate(*host)) {
predicate_added.insert(host);
filtered_added.emplace_back(host);
}
}
Expand All @@ -477,20 +491,38 @@ void SubsetLoadBalancer::HostSubsetImpl::update(const HostVector& hosts_added,
HostVectorSharedPtr hosts(new HostVector());
HostVectorSharedPtr healthy_hosts(new HostVector());

// It's possible that hosts_added == original_host_set_.hosts(), e.g.: when
// calling refreshSubsets() if only metadata change. If so, we can avoid the
// predicate() call.
Copy link
Member

@zuercher zuercher Jul 16, 2018

Choose a reason for hiding this comment

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

Alternatively, I think you could change refreshSubsets to not pass the host set as hosts added since the underlying LBs don't need use information anyway.

Copy link
Member Author

@rgs1 rgs1 Jul 16, 2018

Choose a reason for hiding this comment

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

I would like to do that — it would improve things a lot — but we'd fail to enter this loop in processSubsets:

https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/subset_lb.cc#L200

... which means we wouldn't create the new subsets, if any.

So we'd have to rework processSubsets() or also call findOrCreateSubset() from somewhere else...

I'll look into it, but we can probably merge this in parallel since it's still better.

for (const auto host : original_host_set_.hosts()) {
if (predicate(*host)) {
bool host_seen = predicate_added.count(host) == 1;
if (host_seen || predicate(*host)) {
hosts->emplace_back(host);
if (host->healthy()) {
healthy_hosts->emplace_back(host);
}
}
}

HostsPerLocalityConstSharedPtr hosts_per_locality =
original_host_set_.hostsPerLocality().filter(predicate);
// Calling predicate() is expensive since it involves metadata lookups; so we
// avoid it in the 2nd call to filter() by using the result from the first call
// to filter() as the starting point.
//
// Also, if we only have one locality we can avoid the first call to filter() by
// just creating a new HostsPerLocality from the list of all hosts.
//
// TODO(rgs1): merge these two filter() calls in one loop.
HostsPerLocalityConstSharedPtr hosts_per_locality;

if (original_host_set_.hostsPerLocality().get().size() == 1) {
hosts_per_locality.reset(
new HostsPerLocalityImpl(*hosts, original_host_set_.hostsPerLocality().hasLocalLocality()));
} else {
hosts_per_locality = original_host_set_.hostsPerLocality().filter(predicate);
}

HostsPerLocalityConstSharedPtr healthy_hosts_per_locality =
original_host_set_.hostsPerLocality().filter(
[&predicate](const Host& host) { return predicate(host) && host.healthy(); });
hosts_per_locality->filter([](const Host& host) { return host.healthy(); });

if (locality_weight_aware_) {
HostSetImpl::updateHosts(hosts, healthy_hosts, hosts_per_locality, healthy_hosts_per_locality,
Expand Down