eds: decrease computational complexity of updates #11442

pgenera · 2020-06-04T17:04:12Z

Commit Message: Makes BaseDynamicClusterImpl::updateDynamicHostList O(n) rather than O(n^2)
Additional Description: Instead of calling .erase() on list iterators as we find them, we swap with the end of the list and erase after iterating over the list. This shows a ~3x improvement in execution time in the included benchmark test.
Risk Level: Medium. No reordering happens to the endpoint list. Not runtime guarded.
Testing: New benchmark, existing unit tests pass (and cover the affected function).
Docs Changes: N/A
Release Notes: N/A

#2874 #11362

…impl. Signed-off-by: Phil Genera <pgenera@google.com>

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera · 2020-06-04T18:21:12Z

I'm happy to put this behind a runtime guard if it seems prudent.

source/common/upstream/upstream_impl.cc

mattklein123 · 2020-06-05T15:11:24Z

I'm happy to put this behind a runtime guard if it seems prudent.

IMO it's OK to not have a runtime guard for this, but I would raise the regression risk to medium at least. @snowp can you also take a look at this?

Signed-off-by: Phil Genera <pgenera@google.com>

htuch · 2020-06-09T23:27:54Z

This should be rebased on #11505.

Signed-off-by: Phil Genera <pgenera@google.com>

source/common/upstream/upstream_impl.cc

jmarantz

Can you add to the PR description a comparison of your speed test before/after your n^2 fix?

You'd have to patch the speed-test only into a different client.

test/common/common/utility_test.cc

source/common/common/utility.h

Signed-off-by: Phil Genera <pgenera@google.com>

test/common/common/utility_test.cc

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera · 2020-06-17T17:32:10Z

Can you add to the PR description a comparison of your speed test before/after your n^2 fix?

You'd have to patch the speed-test only into a different client.

Done, its still ~3.2x improvement over the baseline. Results are linked from the description; I can be more explicit than that if you'd like.

jmarantz

up to you if you want to think about larger variable names.

This does need a @envoyproxy/senior-maintainers approval.

source/common/upstream/upstream_impl.cc

jmarantz · 2020-06-17T17:42:10Z

and thanks for doing this!

pgenera · 2020-06-17T18:37:45Z

Can you add to the PR description a comparison of your speed test before/after your n^2 fix?
You'd have to patch the speed-test only into a different client.

Done, its still ~3.2x improvement over the baseline. Results are linked from the description; I can be more explicit than that if you'd like.

After a bit of thought I noticed the performance of priorityAndLocalWeighted is about ~30% slower. Notably those tests don't exercise any of the n^2 logic (eg, with one iteration none of those .erase() calls happen), but I'm still surprised that the visitor-predicate-pattern is measurably slower than iterating in situ. Even with this mysterious observation, I think a 320% improvement in what I think is the common case is worth a 30% worse performance on the first iteration.

jmarantz · 2020-06-17T18:40:34Z

Quick check: are you comparing optimized runs? Without optimization, inlining, and collapsing of dead logic I could see this being a significant performance degradation.

jmarantz · 2020-06-17T18:50:19Z

I see in your comment you did use -c opt. It's probably worth an iteration with cachegrind or callgrind focusing on the troublesome use-case to see just what's up. Possibly the lambda context adds a level of indirection through a generated structure that might not be possible to fully optimize away.

If the absolute per-itereration perf penalty is not too great it might be fine to just explain that, maybe as a comment in the code for posterity.

Signed-off-by: Phil Genera <pgenera@google.com>

jmarantz

Great, thanks! Praying to the gods of clang it goes through!

@envoyproxy/senior-maintainers

test/benchmark/main.cc

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera · 2020-06-29T19:52:41Z

Great, thanks! Praying to the gods of clang it goes through!

I do not think we have been smiled upon :D. I'll be out the rest of this week, but will poke my head in late tonight in case there's something easy to do.

test/benchmark/main.cc

jmarantz · 2020-07-02T21:54:53Z

also merge master to hopefully pick up a fix that was made to the http2 integration test for tsan.

jmarantz · 2020-07-02T21:55:06Z

/wait

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera · 2020-07-07T15:57:13Z

also merge master to hopefully pick up a fix that was made to the http2 integration test for tsan.

Done and done. And it appears to have helped!

jmarantz

@envoyproxy/senior-maintainers

htuch

LGTM modulo a nit.
/wait

test/benchmark/main.h

Signed-off-by: Phil Genera <pgenera@google.com>

htuch

LGTM, thanks!

pgenera · 2020-07-08T19:50:09Z

Looking through the CI failures:

coverage: [ FAILED ] IpVersionsClientType/HdsIntegrationTest.SingleEndpointUnhealthyHttp/5, where GetParam() = (4-byte object <00-00 00-00>, 4-byte object <01-00 00-00>, 0): unrelated
windows: //test/extensions/filters/http/router:auto_sni_integration_test FAILED: unrelated

Both of these have high-flakiness warnings when I look at them in azure. They all (of course) pass locally and with RBE.

Signed-off-by: Phil Genera <pgenera@google.com>

jmarantz · 2020-07-09T12:58:24Z

/azp run

azure-pipelines · 2020-07-09T12:58:33Z

Azure Pipelines successfully started running 1 pipeline(s).

antoniovicente · 2020-07-17T21:24:45Z

test/benchmark/main.cc

  }
+
+  skip_expensive_benchmarks = skip_switch.getValue();


Could we add some big nice WARNING when this flag is enabled in order to increase the chances of someone noticing the difference between envoy_cc_benchmarks and tests for those benchmarks?

Done in #12121

Makes BaseDynamicClusterImpl::updateDynamicHostList O(n) rather than O(n^2) Instead of calling .erase() on list iterators as we find them, we swap with the end of the list and erase after iterating over the list. This shows a ~3x improvement in execution time in the included benchmark test. Risk Level: Medium. No reordering happens to the endpoint list. Not runtime guarded. Testing: New benchmark, existing unit tests pass (and cover the affected function). Docs Changes: N/A Release Notes: N/A Relates to envoyproxy#2874 envoyproxy#11362 Signed-off-by: Phil Genera <pgenera@google.com> Signed-off-by: scheler <santosh.cheler@appdynamics.com>

pgenera added 4 commits June 3, 2020 19:40

New eds_speed_tests and temporary complexity annotations in upstream_…

b6393c6

…impl. Signed-off-by: Phil Genera <pgenera@google.com>

Remove N^2 behavior in updateDynamicHostList, write a benchmark for it.

779aa74

Signed-off-by: Phil Genera <pgenera@google.com>

Run pre-push hooks

5005fcf

Signed-off-by: Phil Genera <pgenera@google.com>

Remove a note I missed in the prior pass

de4eeb7

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera marked this pull request as ready for review June 4, 2020 18:21

jmarantz self-assigned this Jun 4, 2020

jmarantz reviewed Jun 5, 2020

View reviewed changes

mattklein123 assigned snowp Jun 5, 2020

Respond to (simple) review comments

46a176e

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera added 3 commits June 11, 2020 13:05

Merge remote-tracking branch 'upstream/master' into eds-nsquared

b81bf9b

Signed-off-by: Phil Genera <pgenera@google.com>

Respond to reivew comments, fix eds_speed_test.

f846f8f

Signed-off-by: Phil Genera <pgenera@google.com>

Merge remote-tracking branch 'upstream/master' into eds-nsquared

dabdeb6

Signed-off-by: Phil Genera <pgenera@google.com>

jmarantz reviewed Jun 16, 2020

View reviewed changes

source/common/upstream/upstream_impl.cc Outdated Show resolved Hide resolved

jmarantz reviewed Jun 16, 2020

View reviewed changes

test/common/common/utility_test.cc Outdated Show resolved Hide resolved

test/common/common/utility_test.cc Outdated Show resolved Hide resolved

jmarantz reviewed Jun 17, 2020

View reviewed changes

source/common/common/utility.h Outdated Show resolved Hide resolved

review comments, fix multiple calls to grpc initializers

6d08d00

Signed-off-by: Phil Genera <pgenera@google.com>

jmarantz reviewed Jun 17, 2020

View reviewed changes

test/common/common/utility_test.cc Outdated Show resolved Hide resolved

jmarantz reviewed Jun 17, 2020

View reviewed changes

test/common/common/utility_test.cc Outdated Show resolved Hide resolved

jmarantz reviewed Jun 17, 2020

View reviewed changes

test/common/common/utility_test.cc Outdated Show resolved Hide resolved

test/common/common/utility_test.cc Outdated Show resolved Hide resolved

pgenera added 2 commits June 17, 2020 14:32

respond to review comments

7cccabb

Signed-off-by: Phil Genera <pgenera@google.com>

Solve the mystery of c++ templates.

4d1acad

Signed-off-by: Phil Genera <pgenera@google.com>

jmarantz previously approved these changes Jun 17, 2020

View reviewed changes

source/common/upstream/upstream_impl.cc Outdated Show resolved Hide resolved

response to review comments

3c37bc6

Signed-off-by: Phil Genera <pgenera@google.com>

jmarantz previously approved these changes Jun 29, 2020

View reviewed changes

test/benchmark/main.cc Outdated Show resolved Hide resolved

respond to comments

e99517d

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera dismissed jmarantz’s stale review via e99517d June 29, 2020 16:35

jmarantz reviewed Jul 2, 2020

View reviewed changes

test/benchmark/main.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 2, 2020

pgenera added 2 commits July 6, 2020 13:00

respond to review comments

60e769f

Signed-off-by: Phil Genera <pgenera@google.com>

Merge remote-tracking branch 'upstream/master' into eds-nsquared

3644413

Signed-off-by: Phil Genera <pgenera@google.com>

repokitteh-read-only bot removed the waiting label Jul 6, 2020

jmarantz previously approved these changes Jul 8, 2020

View reviewed changes

htuch suggested changes Jul 8, 2020

View reviewed changes

test/benchmark/main.h Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Jul 8, 2020

respond to review comments

558423a

Signed-off-by: Phil Genera <pgenera@google.com>

pgenera dismissed jmarantz’s stale review via 558423a July 8, 2020 18:32

repokitteh-read-only bot removed the waiting label Jul 8, 2020

htuch approved these changes Jul 8, 2020

View reviewed changes

Kick CI

2b689d1

Signed-off-by: Phil Genera <pgenera@google.com>

htuch merged commit b1e62a3 into envoyproxy:master Jul 9, 2020

pgenera deleted the eds-nsquared branch July 13, 2020 16:07

antoniovicente reviewed Jul 17, 2020

View reviewed changes

antoniovicente mentioned this pull request Jul 17, 2020

docs: add some verbiage for benchmark test rules #12121

Merged

htuch mentioned this pull request Jul 20, 2020

eds: improve performance of updates #11362

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

eds: decrease computational complexity of updates #11442

eds: decrease computational complexity of updates #11442

pgenera commented Jun 4, 2020 •

edited

Loading

pgenera commented Jun 4, 2020

mattklein123 commented Jun 5, 2020

htuch commented Jun 9, 2020

jmarantz left a comment

pgenera commented Jun 17, 2020

jmarantz left a comment

jmarantz commented Jun 17, 2020

pgenera commented Jun 17, 2020

jmarantz commented Jun 17, 2020

jmarantz commented Jun 17, 2020

jmarantz left a comment

pgenera commented Jun 29, 2020

jmarantz commented Jul 2, 2020

jmarantz commented Jul 2, 2020

pgenera commented Jul 7, 2020

jmarantz left a comment

htuch left a comment

htuch left a comment

pgenera commented Jul 8, 2020 •

edited

Loading

jmarantz commented Jul 9, 2020

azure-pipelines bot commented Jul 9, 2020

antoniovicente Jul 17, 2020

pgenera Jul 21, 2020

eds: decrease computational complexity of updates #11442

eds: decrease computational complexity of updates #11442

Conversation

pgenera commented Jun 4, 2020 • edited Loading

pgenera commented Jun 4, 2020

mattklein123 commented Jun 5, 2020

htuch commented Jun 9, 2020

jmarantz left a comment

Choose a reason for hiding this comment

pgenera commented Jun 17, 2020

jmarantz left a comment

Choose a reason for hiding this comment

jmarantz commented Jun 17, 2020

pgenera commented Jun 17, 2020

jmarantz commented Jun 17, 2020

jmarantz commented Jun 17, 2020

jmarantz left a comment

Choose a reason for hiding this comment

pgenera commented Jun 29, 2020

jmarantz commented Jul 2, 2020

jmarantz commented Jul 2, 2020

pgenera commented Jul 7, 2020

jmarantz left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

pgenera commented Jul 8, 2020 • edited Loading

jmarantz commented Jul 9, 2020

azure-pipelines bot commented Jul 9, 2020

antoniovicente Jul 17, 2020

Choose a reason for hiding this comment

pgenera Jul 21, 2020

Choose a reason for hiding this comment

pgenera commented Jun 4, 2020 •

edited

Loading

pgenera commented Jul 8, 2020 •

edited

Loading