Temporary revert for release "matcher: refactor the matcher to avoid unnecessary wrapper and heap allocation (envoyproxy#40249)"

This reverts commit c34a837.

Author: wbpcode

envoy/matcher/matcher.h

@@ -91,20 +91,22 @@ class Action {
   }
 };
 
-using ActionConstSharedPtr = std::shared_ptr<const Action>;
+using ActionPtr = std::unique_ptr<Action>;
+using ActionFactoryCb = std::function<ActionPtr()>;
 
 template <class ActionFactoryContext> class ActionFactory : public Config::TypedFactory {
 public:
-  virtual ActionConstSharedPtr
-  createAction(const Protobuf::Message& config, ActionFactoryContext& action_factory_context,
-               ProtobufMessage::ValidationVisitor& validation_visitor) PURE;
+  virtual ActionFactoryCb
+  createActionFactoryCb(const Protobuf::Message& config,
+                        ActionFactoryContext& action_factory_context,
+                        ProtobufMessage::ValidationVisitor& validation_visitor) PURE;
 
   std::string category() const override { return "envoy.matching.action"; }
 };
 
 // On match, we either return the action to perform or another match tree to match against.
 template <class DataType> struct OnMatch {
-  const ActionConstSharedPtr action_;
+  const ActionFactoryCb action_cb_;
   const MatchTreeSharedPtr<DataType> matcher_;
   bool keep_matching_{};
 };
@@ -128,39 +130,30 @@ template <class DataType> class OnMatchFactory {
 // - The match could not be completed due to lack of data (isInsufficientData() will return true.)
 // - The match was completed, no match found (isNoMatch() will return true.)
 // - The match was completed, match found (isMatch() will return true, action() will return the
-//   ActionConstSharedPtr.)
+//   ActionFactoryCb.)
 struct MatchResult {
 public:
-  MatchResult(ActionConstSharedPtr cb) : result_(std::move(cb)) {}
+  MatchResult(ActionFactoryCb cb) : result_(std::move(cb)) {}
   static MatchResult noMatch() { return MatchResult(NoMatch{}); }
   static MatchResult insufficientData() { return MatchResult(InsufficientData{}); }
   bool isInsufficientData() const { return absl::holds_alternative<InsufficientData>(result_); }
   bool isComplete() const { return !isInsufficientData(); }
   bool isNoMatch() const { return absl::holds_alternative<NoMatch>(result_); }
-  bool isMatch() const { return absl::holds_alternative<ActionConstSharedPtr>(result_); }
-  const ActionConstSharedPtr& action() const {
-    ASSERT(isMatch());
-    return absl::get<ActionConstSharedPtr>(result_);
-  }
-  // Returns the action by move. The caller must ensure that the MatchResult is not used after
-  // this call.
-  ActionConstSharedPtr actionByMove() {
-    ASSERT(isMatch());
-    return absl::get<ActionConstSharedPtr>(std::move(result_));
-  }
+  bool isMatch() const { return absl::holds_alternative<ActionFactoryCb>(result_); }
+  ActionFactoryCb actionFactory() const { return absl::get<ActionFactoryCb>(result_); }
+  ActionPtr action() const { return actionFactory()(); }
 
 private:
   struct InsufficientData {};
   struct NoMatch {};
-  using Result = absl::variant<ActionConstSharedPtr, NoMatch, InsufficientData>;
+  using Result = absl::variant<ActionFactoryCb, NoMatch, InsufficientData>;
   Result result_;
   MatchResult(NoMatch) : result_(NoMatch{}) {}
   MatchResult(InsufficientData) : result_(InsufficientData{}) {}
 };
 
 // Callback to execute against skipped matches' actions.
-using SkippedMatchCb = std::function<void(const ActionConstSharedPtr&)>;
-
+using SkippedMatchCb = std::function<void(ActionFactoryCb)>;
 /**
  * MatchTree provides the interface for performing matches against the data provided by DataType.
  */
@@ -194,19 +187,19 @@ template <class DataType> class MatchTree {
       // Parent result's keep_matching skips the nested result.
       if (on_match->keep_matching_ && nested_result.isMatch()) {
         if (skipped_match_cb) {
-          skipped_match_cb(nested_result.action());
+          skipped_match_cb(nested_result.actionFactory());
         }
         return MatchResult::noMatch();
       }
       return nested_result;
     }
-    if (on_match->action_ && on_match->keep_matching_) {
+    if (on_match->action_cb_ && on_match->keep_matching_) {
       if (skipped_match_cb) {
-        skipped_match_cb(on_match->action_);
+        skipped_match_cb(on_match->action_cb_);
       }
       return MatchResult::noMatch();
     }
-    return MatchResult{on_match->action_};
+    return MatchResult{on_match->action_cb_};
   }
 };
 

source/common/listener_manager/filter_chain_manager_impl.cc

@@ -49,11 +49,11 @@ class FilterChainNameActionFactory : public Matcher::ActionFactory<FilterChainAc
                                      Logger::Loggable<Logger::Id::config> {
 public:
   std::string name() const override { return "filter-chain-name"; }
-  Matcher::ActionConstSharedPtr createAction(const Protobuf::Message& config,
-                                             FilterChainActionFactoryContext&,
-                                             ProtobufMessage::ValidationVisitor&) override {
-    return std::make_shared<FilterChainNameAction>(
-        dynamic_cast<const ProtobufWkt::StringValue&>(config).value());
+  Matcher::ActionFactoryCb createActionFactoryCb(const Protobuf::Message& config,
+                                                 FilterChainActionFactoryContext&,
+                                                 ProtobufMessage::ValidationVisitor&) override {
+    const auto& name = dynamic_cast<const ProtobufWkt::StringValue&>(config);
+    return [value = name.value()]() { return std::make_unique<FilterChainNameAction>(value); };
   }
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {
     return std::make_unique<ProtobufWkt::StringValue>();
@@ -573,8 +573,9 @@ FilterChainManagerImpl::findFilterChainUsingMatcher(const Network::ConnectionSoc
       Matcher::evaluateMatch<Network::MatchingData>(*matcher_, data);
   ASSERT(match_result.isComplete(), "Matching must complete for network streams.");
   if (match_result.isMatch()) {
-    return match_result.action()->getTyped<Configuration::FilterChainBaseAction>().get(
-        filter_chains_by_name_, info);
+    const Matcher::ActionPtr action = match_result.action();
+    return action->getTyped<Configuration::FilterChainBaseAction>().get(filter_chains_by_name_,
+                                                                        info);
   }
   return default_filter_chain_.get();
 }

source/common/matcher/matcher.h

@@ -324,10 +324,10 @@ class MatchTreeFactory : public OnMatchFactory<DataType> {
           on_match.action().typed_config(), server_factory_context_.messageValidationVisitor(),
           factory);
 
-      auto action = factory.createAction(*message, action_factory_context_,
-                                         server_factory_context_.messageValidationVisitor());
-      return [action, keep_matching = on_match.keep_matching()] {
-        return OnMatch<DataType>{action, {}, keep_matching};
+      auto action_factory = factory.createActionFactoryCb(
+          *message, action_factory_context_, server_factory_context_.messageValidationVisitor());
+      return [action_factory, keep_matching = on_match.keep_matching()] {
+        return OnMatch<DataType>{action_factory, {}, keep_matching};
       };
     }
 

source/common/router/config_impl.cc

@@ -1825,13 +1825,14 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb
         Matcher::evaluateMatch<Http::HttpMatchingData>(*matcher_, data);
 
     if (match_result.isMatch()) {
-      const auto result = match_result.actionByMove();
+      const Matcher::ActionPtr result = match_result.action();
       if (result->typeUrl() == RouteMatchAction::staticTypeUrl()) {
-        return getRouteFromRoutes(
-            cb, headers, stream_info, random_value,
-            {std::dynamic_pointer_cast<const RouteEntryImplBase>(std::move(result))});
+        const RouteMatchAction& route_action = result->getTyped<RouteMatchAction>();
+
+        return getRouteFromRoutes(cb, headers, stream_info, random_value, {route_action.route()});
       } else if (result->typeUrl() == RouteListMatchAction::staticTypeUrl()) {
         const RouteListMatchAction& action = result->getTyped<RouteListMatchAction>();
+
         return getRouteFromRoutes(cb, headers, stream_info, random_value, action.routes());
       }
       PANIC("Action in router matcher should be Route or RouteList");
@@ -2139,24 +2140,24 @@ const Envoy::Config::TypedMetadata& NullConfigImpl::typedMetadata() const {
   return DefaultRouteMetadataPack::get().typed_metadata_;
 }
 
-Matcher::ActionConstSharedPtr
-RouteMatchActionFactory::createAction(const Protobuf::Message& config, RouteActionContext& context,
-                                      ProtobufMessage::ValidationVisitor& validation_visitor) {
+Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb(
+    const Protobuf::Message& config, RouteActionContext& context,
+    ProtobufMessage::ValidationVisitor& validation_visitor) {
   const auto& route_config =
       MessageUtil::downcastAndValidate<const envoy::config::route::v3::Route&>(config,
                                                                                validation_visitor);
   auto route = THROW_OR_RETURN_VALUE(
       RouteCreator::createAndValidateRoute(route_config, context.vhost, context.factory_context,
                                            validation_visitor, false),
       RouteEntryImplBaseConstSharedPtr);
-  return route;
+
+  return [route]() { return std::make_unique<RouteMatchAction>(route); };
 }
 REGISTER_FACTORY(RouteMatchActionFactory, Matcher::ActionFactory<RouteActionContext>);
 
-Matcher::ActionConstSharedPtr
-RouteListMatchActionFactory::createAction(const Protobuf::Message& config,
-                                          RouteActionContext& context,
-                                          ProtobufMessage::ValidationVisitor& validation_visitor) {
+Matcher::ActionFactoryCb RouteListMatchActionFactory::createActionFactoryCb(
+    const Protobuf::Message& config, RouteActionContext& context,
+    ProtobufMessage::ValidationVisitor& validation_visitor) {
   const auto& route_config =
       MessageUtil::downcastAndValidate<const envoy::config::route::v3::RouteList&>(
           config, validation_visitor);
@@ -2168,7 +2169,7 @@ RouteListMatchActionFactory::createAction(const Protobuf::Message& config,
                                              validation_visitor, false),
         RouteEntryImplBaseConstSharedPtr));
   }
-  return std::make_shared<RouteListMatchAction>(std::move(routes));
+  return [routes]() { return std::make_unique<RouteListMatchAction>(routes); };
 }
 REGISTER_FACTORY(RouteListMatchActionFactory, Matcher::ActionFactory<RouteActionContext>);
 

source/common/router/config_impl.h

@@ -619,7 +619,6 @@ class RouteEntryImplBase : public RouteEntryAndRoute,
                            public Matchable,
                            public DirectResponseEntry,
                            public PathMatchCriterion,
-                           public Matcher::ActionBase<envoy::config::route::v3::Route>,
                            public std::enable_shared_from_this<RouteEntryImplBase>,
                            Logger::Loggable<Logger::Id::router> {
 protected:
@@ -1192,9 +1191,9 @@ class RouteMatchAction : public Matcher::ActionBase<envoy::config::route::v3::Ro
 // Registered factory for RouteMatchAction.
 class RouteMatchActionFactory : public Matcher::ActionFactory<RouteActionContext> {
 public:
-  Matcher::ActionConstSharedPtr
-  createAction(const Protobuf::Message& config, RouteActionContext& context,
-               ProtobufMessage::ValidationVisitor& validation_visitor) override;
+  Matcher::ActionFactoryCb
+  createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context,
+                        ProtobufMessage::ValidationVisitor& validation_visitor) override;
   std::string name() const override { return "route"; }
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {
     return std::make_unique<envoy::config::route::v3::Route>();
@@ -1218,9 +1217,9 @@ class RouteListMatchAction : public Matcher::ActionBase<envoy::config::route::v3
 // Registered factory for RouteListMatchAction.
 class RouteListMatchActionFactory : public Matcher::ActionFactory<RouteActionContext> {
 public:
-  Matcher::ActionConstSharedPtr
-  createAction(const Protobuf::Message& config, RouteActionContext& context,
-               ProtobufMessage::ValidationVisitor& validation_visitor) override;
+  Matcher::ActionFactoryCb
+  createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context,
+                        ProtobufMessage::ValidationVisitor& validation_visitor) override;
   std::string name() const override { return "route_match_action"; }
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {
     return std::make_unique<envoy::config::route::v3::RouteList>();

source/extensions/filters/common/rbac/engine_impl.cc

@@ -11,9 +11,9 @@ namespace Filters {
 namespace Common {
 namespace RBAC {
 
-Envoy::Matcher::ActionConstSharedPtr
-ActionFactory::createAction(const Protobuf::Message& config, ActionContext& context,
-                            ProtobufMessage::ValidationVisitor& validation_visitor) {
+Envoy::Matcher::ActionFactoryCb
+ActionFactory::createActionFactoryCb(const Protobuf::Message& config, ActionContext& context,
+                                     ProtobufMessage::ValidationVisitor& validation_visitor) {
   const auto& action_config =
       MessageUtil::downcastAndValidate<const envoy::config::rbac::v3::Action&>(config,
                                                                                validation_visitor);
@@ -25,7 +25,7 @@ ActionFactory::createAction(const Protobuf::Message& config, ActionContext& cont
     context.has_log_ = true;
   }
 
-  return std::make_shared<Action>(name, action);
+  return [name, action]() { return std::make_unique<Action>(name, action); };
 }
 
 REGISTER_FACTORY(ActionFactory, Envoy::Matcher::ActionFactory<ActionContext>);
@@ -138,17 +138,18 @@ bool RoleBasedAccessControlMatcherEngineImpl::handleAction(
       Envoy::Matcher::evaluateMatch<Http::HttpMatchingData>(*matcher_, data);
   ASSERT(result.isComplete());
   if (result.isMatch()) {
-    const auto& action = result.action()->getTyped<Action>();
+    auto action = result.action()->getTyped<Action>();
     if (effective_policy_id != nullptr) {
       *effective_policy_id = action.name();
     }
 
     // If there is at least an LOG action in matchers, we have to turn on and off for shared log
     // metadata every time when there is a connection or request.
-    const auto rbac_action = action.action();
+    auto rbac_action = action.action();
     if (has_log_) {
       generateLog(info, mode_, rbac_action == envoy::config::rbac::v3::RBAC::LOG);
     }
+
     switch (rbac_action) {
       PANIC_ON_PROTO_ENUM_SENTINEL_VALUES;
     case envoy::config::rbac::v3::RBAC::ALLOW:

source/extensions/filters/common/rbac/engine_impl.h

@@ -50,9 +50,9 @@ class Action : public Envoy::Matcher::ActionBase<envoy::config::rbac::v3::Action
 
 class ActionFactory : public Envoy::Matcher::ActionFactory<ActionContext> {
 public:
-  Envoy::Matcher::ActionConstSharedPtr
-  createAction(const Protobuf::Message& config, ActionContext& context,
-               ProtobufMessage::ValidationVisitor& validation_visitor) override;
+  Envoy::Matcher::ActionFactoryCb
+  createActionFactoryCb(const Protobuf::Message& config, ActionContext& context,
+                        ProtobufMessage::ValidationVisitor& validation_visitor) override;
   std::string name() const override { return "envoy.filters.rbac.action"; }
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {
     return std::make_unique<envoy::config::rbac::v3::Action>();