Avoid extra `predicate()` calls #3869

rgs1 · 2018-07-16T19:41:16Z

In SubsetLoadBalancer::HostSubsetImpl::update() we have repeated calls
to predicate(). These calls are expensive mostly because of
Envoy::Config::Metadata::metadataValue() calls. Furthermore, metadata
is guarded by a lock since #3814 — so there is some contention too.

This change improves things by doing two things:

avoid use of predicate() to derive healthy_hosts_per_locality
cache predicate() calls for the special case of only metadata
changing (current hosts == hosts added)

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

In `SubsetLoadBalancer::HostSubsetImpl::update()` we have repeated calls to `predicate()`. These calls are expensive mostly because of `Envoy::Config::Metadata::metadataValue()` calls. Furthermore, metadata is guarded by a lock since envoyproxy#3814 — so there is some contention too. This change improves things by doing two things: * avoid use of `predicate()` to derive `healthy_hosts_per_locality` * cache `predicate()` calls for the special case of only metadata changing (current hosts == hosts added) Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2018-07-16T19:48:17Z

A bit more context here, in SubsetLoadBalancer::HostSubsetImpl::update() (https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/subset_lb.cc) the following is going on:

* go over added hosts (when only metadata change, this is O(n) where n is total hosts)
* go over removed hosts (not a big deal, usually)
* go over original hosts (O(n) where n is total hosts)
* go over hosts per locality (O(n) with n == total hosts) 
* go over hosts per locality and extra healthy ones (again, O(n)

Things get pretty expensive because on every iteration we have to call hostMatches() (https://github.com/envoyproxy/envoy/blob/master/source/common/upstream/subset_lb.cc#L276), which requires a lock and (the very expensive part) metadata lookups and metadata value extraction. From our perf samples, the Envoy::Config::Metadata::metadataValue() calls dominate.

So ideally, we can:

collapse the iterations as much as possible
cache predicate() calls when possible

This diff starts in that way, but there's more I think.

Thoughts?

cc: @mattklein123 @zuercher

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

zuercher

Looks good. One comment about a potential simplification. I'll leave it up to you if you want to do that or not.

zuercher · 2018-07-16T20:33:50Z

source/common/upstream/subset_lb.cc

@@ -477,20 +480,26 @@ 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.


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.

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.

mattklein123 · 2018-07-16T22:21:41Z

source/common/upstream/subset_lb.cc

      hosts->emplace_back(host);
      if (host->healthy()) {
        healthy_hosts->emplace_back(host);
      }
    }
  }

+  // Calling predicate() is expensive since it involves metadata lookups; so we
+  // can take a shortcut and just filter healthy hosts in a follow-up call.


Sorry just for my understanding, what follow-up call? How come we don't have to do this predicate call here? Maybe make the comment a little more detailed?

yes, yes — the follow-up call is the 2nd call to filter(). I'll comment in more detail, and there's actually one more (significant) perf improvement we can have....

Ah, I see it now. Sorry did not read it closely. I think maybe you meant "in the follow-up call" ?

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

This is one more optimization to avoid filter() calls using hostMatches() as a predicate when possible. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2018-07-17T04:49:45Z

@zuercher @mattklein123 lets actually not merge this yet. I ran the patch through perf today and although it's nice to see the metadataValue() calls go away, CPU does go up a bit because of the ordered_set calls. So I'd like to play with this a bit more before we merge it.

On a related note, I got into this patch while debugging a case in which a given stream of EDS updates will cause the subset lb to push envoy into 100% CPU usage. I'll try to get a repro tomorrow.

Config::Metadata::metadataValue() is pretty general, which means we end up calling host_metadata.filter_metadata().find() from within the main loop of hostMatches(). This should get rid of some of hostMatches()'s overhead. Tests: running this through perf now to measure. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2018-07-17T06:47:54Z

One last thought: I think most improvements are good to go except for maybe using the ordered_set to cache the first set of hostMatches() calls in SubsetLoadBalancer::HostSubsetImpl::update(). I'll do extensive perf runs tomorrow to decide if we want to keep that optimization or not.

Leftovers from inline metadataValue(). Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2018-07-17T16:36:33Z

@mattklein123 @zuercher on a 2nd thought, if you like it as is we can actually merge it and I can run out of master for a few days and see how it goes. Worst case if it isn't great or that much better, we revert that parts that we don't like.

rgs1 · 2018-07-17T20:52:37Z

FWIW, the CPU increase that I saw yesterday was not from this change. I am bisecting now to see who's driving that increase, so I think we are clear here. Perf issue follow-up #3878.

Remove unused variable

5efbba7

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

zuercher reviewed Jul 16, 2018

View reviewed changes

zuercher previously approved these changes Jul 16, 2018

View reviewed changes

mattklein123 reviewed Jul 16, 2018

View reviewed changes

Fix/clarify comment.

4873108

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 dismissed zuercher’s stale review via 4873108 July 16, 2018 23:08

Avoid filter() when we only have 1 locality

5fd8886

This is one more optimization to avoid filter() calls using hostMatches() as a predicate when possible. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

Remove unneeded dereferences

4df5647

Leftovers from inline metadataValue(). Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

junr03 assigned zuercher and mattklein123 Jul 17, 2018

zuercher approved these changes Jul 17, 2018

View reviewed changes

mattklein123 approved these changes Jul 18, 2018

View reviewed changes

mattklein123 merged commit 17efc83 into envoyproxy:master Jul 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid extra `predicate()` calls #3869

Avoid extra `predicate()` calls #3869

rgs1 commented Jul 16, 2018

rgs1 commented Jul 16, 2018

zuercher left a comment

zuercher Jul 16, 2018

rgs1 Jul 16, 2018

mattklein123 Jul 16, 2018

rgs1 Jul 16, 2018

mattklein123 Jul 16, 2018

rgs1 commented Jul 17, 2018

rgs1 commented Jul 17, 2018

rgs1 commented Jul 17, 2018

rgs1 commented Jul 17, 2018

Avoid extra predicate() calls #3869

Avoid extra predicate() calls #3869

Conversation

rgs1 commented Jul 16, 2018

rgs1 commented Jul 16, 2018

zuercher left a comment

Choose a reason for hiding this comment

zuercher Jul 16, 2018

Choose a reason for hiding this comment

rgs1 Jul 16, 2018

Choose a reason for hiding this comment

mattklein123 Jul 16, 2018

Choose a reason for hiding this comment

rgs1 Jul 16, 2018

Choose a reason for hiding this comment

mattklein123 Jul 16, 2018

Choose a reason for hiding this comment

rgs1 commented Jul 17, 2018

rgs1 commented Jul 17, 2018

rgs1 commented Jul 17, 2018

rgs1 commented Jul 17, 2018

Avoid extra `predicate()` calls #3869

Avoid extra `predicate()` calls #3869