discovery: refactor configuration ingestion. #11638

htuch · 2020-06-18T13:49:14Z

Previously, the gRPC muxes required decoding an opaque resource to
obtain its name, then dispatch to the relevant subscription, which would
again decode the opaque resource. This is pretty horrible efficiency
wise, in particular when upgrading from v2 -> v3.

In this patch, we introduce a DecodedResource wrapper and
OpaqueResourceDecoder. The config ingestion module, e.g. GrpcMuxImpl,
uses the OpaqueResourceDecoder to produce a typed DecodedResource,
performing the decode once. This DecodedResource is then dispatched
to the watching subscription.

This provides > 20% speedup on the v2 -> v3 tax for eds_speed_test,
decreasing from an overhead of 3.2x to 2.5x. It's also likely to unlock
further optimizations as we now have a wrapper resource and simplifies
subscription implementations, as they no longer need to deal with delta
vs. SotW resource decoding in different ways.

Risk level: Medium (configuration ingestion path changes).
Testing: New unit tests for
DecodedResourceImpl/OpaqueResourceDecoderImpl, updated existing unit
tests to work with new interfaces.

Partial solution to #11362

Signed-off-by: Harvey Tuch htuch@google.com

Previously, the gRPC muxes required that decoding an opaque resource to obtain its name, then dispatch to the relevant subscription, which would again decode the opaque resource. This is pretty horrible efficiency wise, in particular when upgrading from v2 -> v3. In this patch, we introduce a DecodedResource wrapper and OpaqueResourceDecoder. The config ingestion module, e.g. GrpcMuxImpl, uses the OpaqueResourceDecoder to produce a typed DecodedResource, performing the decode once. This DecodedResource is then dispatched to the watching subscription. This provides > 20% speedup on the v2 -> v3 tax for eds_speed_test, decreasing from an overhead of 3.2x to 2.5x. It's also likely to unlock further optimizations as we now have a wrapper resource and simplifies subscription implementations, as they no longer need to deal with delta vs. SotW resource decoding in different ways. Risk level: Medium (configuration ingestion path changes). Testing: New unit tests for DecodedResourceImpl/OpaqueResourceDecoderImpl, updated existing unit tests to work with new interfaces. Partial solution to envoyproxy#11362 Signed-off-by: Harvey Tuch <htuch@google.com>

source/common/config/subscription_base.h

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2020-06-19T16:02:30Z

@pgenera FYI, this PR reduces v3 conversion overhead by 20-25%.

Signed-off-by: Harvey Tuch <htuch@google.com>

wgallagher · 2020-06-23T15:26:00Z

include/envoy/config/subscription.h

+/**
+ * Invoked when raw config received from xDS wire.
+ */
+class UntypedConfigUpdateCallbacks {


is there a reason this interface must still be maintained? Is it possible to make all callbacks use 'DecodedResource'

It's used on the WatchMap interface, prior to any resource decoding.

wgallagher · 2020-06-23T15:28:27Z

include/envoy/config/subscription_factory.h

 * @return SubscriptionPtr subscription object corresponding for config and type_url.
 */
 virtual SubscriptionPtr
 subscriptionFromConfigSource(const envoy::config::core::v3::ConfigSource& config,
 absl::string_view type_url, Stats::Scope& scope,
- SubscriptionCallbacks& callbacks) PURE;
+ SubscriptionCallbacks& callbacks,
+ OpaqueResourceDecoder& resource_decoder) PURE;


Are different decoders used for different subscriptions? If not, would it be better for the SubscriptionFactory implementation to be constructed with the decoder?

Yeah, each resource decoder effectively capture the Any conversion for the subscription type, so it has to be part of the args here.

wgallagher · 2020-06-23T15:44:02Z

source/common/config/grpc_mux_impl.cc

- SubscriptionCallbacks& callbacks) {
- auto watch = std::make_unique<GrpcMuxWatchImpl>(resources, callbacks, type_url, *this);
+ SubscriptionCallbacks& callbacks,
+ OpaqueResourceDecoder& resource_decoder) {


if the grpc_mux_impl uses the same resource_decoder to decode all resources, I think it makes sense to make it a member of the class

As above, since this has the type-specific Any conversion internally, we need to supply the resource decoder on each watch when muxing across types, i.e. ADS.

source/common/config/http_subscription_impl.cc

source/common/config/watch_map.cc

source/common/config/filesystem_subscription_impl.cc

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2020-06-25T18:23:39Z

@wgallagher could you take another look, review feedback should be addressed, thanks.

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch · 2020-06-26T17:53:07Z

Also adding @mattklein123 as reviewer for maintainer approval.

wgallagher · 2020-06-26T18:16:11Z

+1 lgtm

mattklein123

Nice, very cool. Some optional comments from my skim through.

mattklein123 · 2020-06-30T16:02:16Z

include/envoy/config/subscription.h

+ /**
+ * @return const Protobuf::Message& resource message reference. If hasResource() is false, this
+ * will be the empty message.
+ */
+ virtual const Protobuf::Message& resource() const PURE;
+
+ /**
+ * @return bool does the xDS discovery response have a set resource payload?
+ */
+ virtual bool hasResource() const PURE;


From an interface perspective you might consider just returning an optional reference from resource() and then I think you can do away with hasResource()?

Yeah, I tried that initially, but it's cleaner to do it this way, since some sites still want a resource (the empty resource) even if hasResource() is false.

source/common/config/grpc_mux_impl.cc

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch assigned wgallagher Jun 18, 2020

htuch requested a review from wgallagher June 18, 2020 13:49

wgallagher reviewed Jun 18, 2020

View reviewed changes

source/common/config/subscription_base.h Outdated Show resolved Hide resolved

htuch added 2 commits June 19, 2020 06:54

Merge remote-tracking branch 'upstream/master' into resource-name-opt

6a8f606

Review feedback.

b532c57

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch added 2 commits June 21, 2020 18:36

Merge remote-tracking branch 'upstream/master' into resource-name-opt

e88422b

Some clang-tidy fixes.

a623257

Signed-off-by: Harvey Tuch <htuch@google.com>

wgallagher reviewed Jun 23, 2020

View reviewed changes

htuch added 3 commits June 23, 2020 18:27

Merge remote-tracking branch 'upstream/master' into resource-name-opt

edb2842

Factor out decoded resources owned/ref vector creation.

49b15c1

Signed-off-by: Harvey Tuch <htuch@google.com>

Merge remote-tracking branch 'upstream/master' into resource-name-opt

41558ad

Fix Windows build hopefully.

5839952

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch requested a review from mattklein123 June 26, 2020 17:52

htuch assigned mattklein123 Jun 26, 2020

Merge remote-tracking branch 'upstream/master' into resource-name-opt

f9c0847

mattklein123 previously approved these changes Jun 30, 2020

View reviewed changes

htuch added 2 commits June 30, 2020 12:49

Merge remote-tracking branch 'upstream/master' into resource-name-opt

0afc3fc

Switch to btree_map in grpc_mux_impl.cc.

f5ea44e

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch dismissed mattklein123’s stale review via f5ea44e June 30, 2020 17:24

mattklein123 approved these changes Jun 30, 2020

View reviewed changes

htuch added 3 commits June 30, 2020 17:16

Kick CI

eaf2516

Signed-off-by: Harvey Tuch <htuch@google.com>

Kick CI

9ff5c17

Signed-off-by: Harvey Tuch <htuch@google.com>

Kick CI

dd0784b

Signed-off-by: Harvey Tuch <htuch@google.com>

htuch merged commit 9b86ed6 into envoyproxy:master Jul 1, 2020

htuch deleted the resource-name-opt branch July 1, 2020 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

discovery: refactor configuration ingestion. #11638

discovery: refactor configuration ingestion. #11638

htuch commented Jun 18, 2020 •

edited

Loading

htuch commented Jun 19, 2020

wgallagher Jun 23, 2020

htuch Jun 23, 2020 •

edited

Loading

wgallagher Jun 23, 2020

htuch Jun 23, 2020

wgallagher Jun 26, 2020

wgallagher Jun 23, 2020

htuch Jun 23, 2020

wgallagher Jun 26, 2020

htuch commented Jun 25, 2020

htuch commented Jun 26, 2020 •

edited

Loading

wgallagher commented Jun 26, 2020

mattklein123 left a comment

mattklein123 Jun 30, 2020

htuch Jun 30, 2020

discovery: refactor configuration ingestion. #11638

discovery: refactor configuration ingestion. #11638

Conversation

htuch commented Jun 18, 2020 • edited Loading

htuch commented Jun 19, 2020

Choose a reason for hiding this comment

htuch Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Jun 25, 2020

htuch commented Jun 26, 2020 • edited Loading

wgallagher commented Jun 26, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Jun 18, 2020 •

edited

Loading

htuch Jun 23, 2020 •

edited

Loading

htuch commented Jun 26, 2020 •

edited

Loading