Optimize AsyncClientManager by moving hashing to control plane by DiazAlan · Pull Request #29199 · envoyproxy/envoy

DiazAlan · 2023-08-23T00:13:39Z

Commit Message: Optimize AsyncClientManager by moving hashing to control plane
Additional Description: This change removes the hashing of the config away from the data plane on every request to the control plane when the filter callback is created. This is accomplished by creating a wrapper object for the config which precomputes the hash and also serializes it to string ahead of time so that calls to the AsyncClientManager with that wrapper can avoid unnecessary overhead.
Risk Level: Low
Testing: Changed some ext_authz tests to account for changes
Added a benchmark test that observed the following improvement
Benchmark Time CPU Iterations
testGetOrCreateAsyncClientWithConfig 27199 us 27182 us 1
testGetOrCreateAsyncClientWithHashConfig 15943 us 15943 us 1

Signed-off-by: AlanDiaz <diazalan@google.com>

repokitteh-read-only · 2023-08-23T00:13:43Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #29199 was opened by DiazAlan.

see: more, trace.

Signed-off-by: AlanDiaz <diazalan@google.com>

DiazAlan · 2023-08-23T15:01:18Z

@ggreenway This is a follow up from what we talked about on 28272. This PR wraps the precomputed hash and config and sends that to the async client manager. I haven't changed the other filters yet as I want to get feedback on the initial implementation of this. If this looks good and passes, I'll make a follow up PR for the other filters.

Signed-off-by: AlanDiaz <diazalan@google.com>

pradeepcrao

Thanks so much for working on this Alan!

pradeepcrao · 2023-08-24T15:51:51Z

source/common/grpc/async_client_manager_impl.cc

    const envoy::config::core::v3::GrpcService& config, Stats::Scope& scope,
    bool skip_cluster_check) {
-  RawAsyncClientSharedPtr client = raw_async_client_cache_->getCache(config);
+  const GrpcServiceHashKeyWrapper& key_wrapper = GrpcServiceHashKeyWrapper(config);


There is a reference to a temporary object here.

Also, what do you think about replacing key_wrapper with config_with_hash_key?

Done. Also changed GrpcServiceHashKeyWrapper to GrpcServiceConfigWithHashKey.

pradeepcrao · 2023-08-24T15:58:35Z

source/common/grpc/async_client_manager_impl.h

  private:
    void evictEntriesAndResetEvictionTimer();
    struct CacheEntry {
      CacheEntry(const envoy::config::core::v3::GrpcService& config,


Why not just have CacheEntry accept a GrpcServiceHashKeyWrapper instead of a GrpcService?

pradeepcrao · 2023-08-24T15:59:50Z

source/common/grpc/async_client_manager_impl.h

-                        MessageUtil>
-        lru_map_;
    LruList lru_list_;
+    absl::flat_hash_map<GrpcServiceHashKeyWrapper, LruList::iterator> lru_map_wrapper_;


nit: maybe keep the old name (lru_map_)? The new name made more sense when you had 2 maps in your old implementation.

pradeepcrao · 2023-08-24T16:02:54Z

envoy/grpc/async_client_manager.h

                            Stats::Scope& scope, bool skip_cluster_check) PURE;

+  virtual RawAsyncClientSharedPtr
+  getOrCreateRawAsyncClientWithWrapper(const GrpcServiceHashKeyWrapper& grpc_service,


Once you've made the change to all filters, it'll be good to deprecate getOrCreateRawAsyncClient without the wrapper, and change the name of this method to getOrCreateRawAsyncClient. Maybe add a comment to that effect?

I also just added a comment to the new getOrCreateRawAsyncClient that better describes it's use.

pradeepcrao · 2023-08-24T16:06:15Z

envoy/grpc/async_client_manager.h

+
+  friend bool operator==(const GrpcServiceHashKeyWrapper& lhs,
+                         const GrpcServiceHashKeyWrapper& rhs) {
+    return Protobuf::util::MessageDifferencer::Equivalent(lhs.config_, rhs.config_);


There is scope for optimisation here. Before the call Protobuf::util::MessageDifferencer::Equivalent, short circuit and return false if pre_computed_hash_ values are different.

Signed-off-by: AlanDiaz <diazalan@google.com>

RyanTheOptimist · 2023-08-25T13:56:45Z

/assign @pradeepcrao

DiazAlan · 2023-08-25T14:36:27Z

/retest

pradeepcrao

Looks great Alan!

Could you please add a unit test for GrpcServiceConfigWithHashKey? Also, I feel it might be easier (and better) to write a performance benchmark that compares getOrCreateRawAsyncClientWithHashKey with getOrCreateRawAsyncClient than try to load test this.

pradeepcrao · 2023-08-25T15:21:15Z

envoy/grpc/async_client_manager.h


+class GrpcServiceConfigWithHashKey {
+public:
+  GrpcServiceConfigWithHashKey(const envoy::config::core::v3::GrpcService& config)


nit: Please mark constructor as explicit.

phlax · 2023-08-29T10:08:41Z

@DiazAlan i think this may be waiting for further iteration

/wait

Signed-off-by: AlanDiaz <diazalan@google.com>

phlax · 2023-09-01T07:33:30Z

/retest

ggreenway

This looks great! Thanks!

Signed-off-by: AlanDiaz <diazalan@google.com>

DiazAlan · 2023-09-05T15:46:13Z

Made a benchmark test to see the difference. Each test using getOrCreateRawAsyncClient 1000 times for each implementation. Here were the results.
Benchmark Time CPU Iterations
testGetOrCreateAsyncClientWithConfig 27199 us 27182 us 1
testGetOrCreateAsyncClientWithHashConfig 15943 us 15943 us 1

Signed-off-by: AlanDiaz <diazalan@google.com>

DiazAlan · 2023-09-05T20:46:33Z

/retest

DiazAlan · 2023-09-06T13:49:14Z

/retest

Change API call on ext_proc to more efficient new implementation. Following the PR #29199, we are replacing the API call in ext_proc so it's not hashing the whole config proto on each request. Signed-off-by: AlanDiaz <diazalan@google.com>

Change API call on ext_proc to more efficient new implementation. Following the PR envoyproxy#29199, we are replacing the API call in ext_proc so it's not hashing the whole config proto on each request. Signed-off-by: AlanDiaz <diazalan@google.com>

…proxy#29199) This change removes the hashing of the config away from the data plane on every request to the control plane when the filter callback is created. This is accomplished by creating a wrapper object for the config which precomputes the hash and also serializes it to string ahead of time so that calls to the AsyncClientManager with that wrapper can avoid unnecessary overhead. Signed-off-by: AlanDiaz <diazalan@google.com>

Change API call on ext_proc to more efficient new implementation. Following the PR envoyproxy#29199, we are replacing the API call in ext_proc so it's not hashing the whole config proto on each request. Signed-off-by: AlanDiaz <diazalan@google.com>

Optimize AsyncClientManager by moving hashing to control plane

d14aa60

Signed-off-by: AlanDiaz <diazalan@google.com>

Change SerializeAsString in wrapper

67d29f6

Signed-off-by: AlanDiaz <diazalan@google.com>

Fix error in build and moved implementation for constructor.

0cc13b8

Signed-off-by: AlanDiaz <diazalan@google.com>

pradeepcrao reviewed Aug 24, 2023

View reviewed changes

AlanDiaz added 2 commits August 24, 2023 15:39

Addressed review comments and did some minor refactoring

13e1081

Signed-off-by: AlanDiaz <diazalan@google.com>

Add comment to new getOrCreateRawAsyncClient function

81c7cc8

Signed-off-by: AlanDiaz <diazalan@google.com>

DiazAlan marked this pull request as ready for review August 24, 2023 19:46

DiazAlan requested a review from ggreenway as a code owner August 24, 2023 19:46

repokitteh-read-only bot assigned pradeepcrao Aug 25, 2023

pradeepcrao reviewed Aug 25, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 29, 2023

Addressed review comments

b208980

Signed-off-by: AlanDiaz <diazalan@google.com>

repokitteh-read-only bot removed the waiting label Aug 31, 2023

Format fix

850a865

Signed-off-by: AlanDiaz <diazalan@google.com>

phlax assigned ggreenway Sep 1, 2023

ggreenway previously approved these changes Sep 5, 2023

View reviewed changes

Add benchmark test

8a9dc73

Signed-off-by: AlanDiaz <diazalan@google.com>

DiazAlan dismissed ggreenway’s stale review via 8a9dc73 September 5, 2023 15:42

format fix

1e03215

Signed-off-by: AlanDiaz <diazalan@google.com>

ggreenway approved these changes Sep 5, 2023

View reviewed changes

ggreenway enabled auto-merge (squash) September 5, 2023 17:18

DiazAlan mentioned this pull request Sep 6, 2023

Add GrpcClient to filter callback to unnecessary hashing in dataplane #28272

Closed

ggreenway merged commit 20676d7 into envoyproxy:main Sep 6, 2023

DiazAlan deleted the AsyncClientManagerOptimization branch September 7, 2023 19:08

DiazAlan mentioned this pull request Sep 8, 2023

Ext proc optimization #29527

Merged

DiazAlan mentioned this pull request Sep 19, 2023

Rate limit async change #29705

Merged

DiazAlan mentioned this pull request Oct 13, 2023

Possible optimization in AsyncClientManager with message comparison #30196

Closed

ashishb-90 mentioned this pull request Nov 30, 2023

extract metadata changes from #29069 solo-io/envoy-fork#12

Merged

Conversation

DiazAlan commented Aug 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 23, 2023

Uh oh!

DiazAlan commented Aug 23, 2023

Uh oh!

pradeepcrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pradeepcrao Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyanTheOptimist commented Aug 25, 2023

Uh oh!

DiazAlan commented Aug 25, 2023

Uh oh!

pradeepcrao left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phlax commented Aug 29, 2023

Uh oh!

phlax commented Sep 1, 2023

Uh oh!

ggreenway left a comment

Choose a reason for hiding this comment

Uh oh!

DiazAlan commented Sep 5, 2023

Uh oh!

DiazAlan commented Sep 5, 2023

Uh oh!

DiazAlan commented Sep 6, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DiazAlan commented Aug 23, 2023 •

edited

Loading

pradeepcrao Aug 24, 2023 •

edited

Loading