Re-apply "matcher: refactor the matcher to avoid unnecessary wrapper and heap allocation (#40249)"

envoy/matcher/matcher.h

@@ -91,22 +91,20 @@ class Action {
   }
 };
 
-using ActionPtr = std::unique_ptr<Action>;
-using ActionFactoryCb = std::function<ActionPtr()>;
+using ActionConstSharedPtr = std::shared_ptr<const Action>;
 
 template <class ActionFactoryContext> class ActionFactory : public Config::TypedFactory {
 public:
-  virtual ActionFactoryCb
-  createActionFactoryCb(const Protobuf::Message& config,
-                        ActionFactoryContext& action_factory_context,
-                        ProtobufMessage::ValidationVisitor& validation_visitor) PURE;
+  virtual ActionConstSharedPtr
+  createAction(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 ActionFactoryCb action_cb_;
+  const ActionConstSharedPtr action_;
   const MatchTreeSharedPtr<DataType> matcher_;
   bool keep_matching_{};
 };
@@ -130,30 +128,39 @@ 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
-//   ActionFactoryCb.)
+//   ActionConstSharedPtr.)
 struct MatchResult {
 public:
-  MatchResult(ActionFactoryCb cb) : result_(std::move(cb)) {}
+  MatchResult(ActionConstSharedPtr 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<ActionFactoryCb>(result_); }
-  ActionFactoryCb actionFactory() const { return absl::get<ActionFactoryCb>(result_); }
-  ActionPtr action() const { return actionFactory()(); }
+  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_));
+  }
 
 private:
   struct InsufficientData {};
   struct NoMatch {};
-  using Result = absl::variant<ActionFactoryCb, NoMatch, InsufficientData>;
+  using Result = absl::variant<ActionConstSharedPtr, NoMatch, InsufficientData>;
   Result result_;
   MatchResult(NoMatch) : result_(NoMatch{}) {}
   MatchResult(InsufficientData) : result_(InsufficientData{}) {}
 };
 
 // Callback to execute against skipped matches' actions.
-using SkippedMatchCb = std::function<void(ActionFactoryCb)>;
+using SkippedMatchCb = std::function<void(const ActionConstSharedPtr&)>;
+
 /**
  * MatchTree provides the interface for performing matches against the data provided by DataType.
  */
@@ -187,19 +194,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.actionFactory());
+          skipped_match_cb(nested_result.action());
         }
         return MatchResult::noMatch();
       }
       return nested_result;
     }
-    if (on_match->action_cb_ && on_match->keep_matching_) {
+    if (on_match->action_ && on_match->keep_matching_) {
       if (skipped_match_cb) {
-        skipped_match_cb(on_match->action_cb_);
+        skipped_match_cb(on_match->action_);
       }
       return MatchResult::noMatch();
     }
-    return MatchResult{on_match->action_cb_};
+    return MatchResult{on_match->action_};
   }
 };
 

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::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); };
+  Matcher::ActionConstSharedPtr createAction(const Protobuf::Message& config,
+                                             FilterChainActionFactoryContext&,
+                                             ProtobufMessage::ValidationVisitor&) override {
+    return std::make_shared<FilterChainNameAction>(
+        dynamic_cast<const ProtobufWkt::StringValue&>(config).value());
   }
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {
     return std::make_unique<ProtobufWkt::StringValue>();
@@ -582,9 +582,8 @@ 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()) {
-    const Matcher::ActionPtr action = match_result.action();
-    return action->getTyped<Configuration::FilterChainBaseAction>().get(filter_chains_by_name_,
-                                                                        info);
+    return match_result.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 = 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};
+      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};
       };
     }
 

source/common/router/config_impl.cc

@@ -1818,14 +1818,13 @@ RouteConstSharedPtr VirtualHostImpl::getRouteFromEntries(const RouteCallback& cb
         Matcher::evaluateMatch<Http::HttpMatchingData>(*matcher_, data);
 
     if (match_result.isMatch()) {
-      const Matcher::ActionPtr result = match_result.action();
+      const auto result = match_result.actionByMove();
       if (result->typeUrl() == RouteMatchAction::staticTypeUrl()) {
-        const RouteMatchAction& route_action = result->getTyped<RouteMatchAction>();
-
-        return getRouteFromRoutes(cb, headers, stream_info, random_value, {route_action.route()});
+        return getRouteFromRoutes(
+            cb, headers, stream_info, random_value,
+            {std::dynamic_pointer_cast<const RouteEntryImplBase>(std::move(result))});
       } 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");
@@ -2133,24 +2132,24 @@ const Envoy::Config::TypedMetadata& NullConfigImpl::typedMetadata() const {
   return DefaultRouteMetadataPack::get().typed_metadata_;
 }
 
-Matcher::ActionFactoryCb RouteMatchActionFactory::createActionFactoryCb(
-    const Protobuf::Message& config, RouteActionContext& context,
-    ProtobufMessage::ValidationVisitor& validation_visitor) {
+Matcher::ActionConstSharedPtr
+RouteMatchActionFactory::createAction(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 std::make_unique<RouteMatchAction>(route); };
+  return route;
 }
 REGISTER_FACTORY(RouteMatchActionFactory, Matcher::ActionFactory<RouteActionContext>);
 
-Matcher::ActionFactoryCb RouteListMatchActionFactory::createActionFactoryCb(
-    const Protobuf::Message& config, RouteActionContext& context,
-    ProtobufMessage::ValidationVisitor& validation_visitor) {
+Matcher::ActionConstSharedPtr
+RouteListMatchActionFactory::createAction(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);
@@ -2162,7 +2161,7 @@ Matcher::ActionFactoryCb RouteListMatchActionFactory::createActionFactoryCb(
                                              validation_visitor, false),
         RouteEntryImplBaseConstSharedPtr));
   }
-  return [routes]() { return std::make_unique<RouteListMatchAction>(routes); };
+  return std::make_shared<RouteListMatchAction>(std::move(routes));
 }
 REGISTER_FACTORY(RouteListMatchActionFactory, Matcher::ActionFactory<RouteActionContext>);
 

source/common/router/config_impl.h

@@ -619,6 +619,7 @@ 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:
@@ -1191,9 +1192,9 @@ class RouteMatchAction : public Matcher::ActionBase<envoy::config::route::v3::Ro
 // Registered factory for RouteMatchAction.
 class RouteMatchActionFactory : public Matcher::ActionFactory<RouteActionContext> {
 public:
-  Matcher::ActionFactoryCb
-  createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context,
-                        ProtobufMessage::ValidationVisitor& validation_visitor) override;
+  Matcher::ActionConstSharedPtr
+  createAction(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>();
@@ -1217,9 +1218,9 @@ class RouteListMatchAction : public Matcher::ActionBase<envoy::config::route::v3
 // Registered factory for RouteListMatchAction.
 class RouteListMatchActionFactory : public Matcher::ActionFactory<RouteActionContext> {
 public:
-  Matcher::ActionFactoryCb
-  createActionFactoryCb(const Protobuf::Message& config, RouteActionContext& context,
-                        ProtobufMessage::ValidationVisitor& validation_visitor) override;
+  Matcher::ActionConstSharedPtr
+  createAction(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::ActionFactoryCb
-ActionFactory::createActionFactoryCb(const Protobuf::Message& config, ActionContext& context,
-                                     ProtobufMessage::ValidationVisitor& validation_visitor) {
+Envoy::Matcher::ActionConstSharedPtr
+ActionFactory::createAction(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::createActionFactoryCb(const Protobuf::Message& config, ActionCont
     context.has_log_ = true;
   }
 
-  return [name, action]() { return std::make_unique<Action>(name, action); };
+  return std::make_shared<Action>(name, action);
 }
 
 REGISTER_FACTORY(ActionFactory, Envoy::Matcher::ActionFactory<ActionContext>);
@@ -138,18 +138,17 @@ bool RoleBasedAccessControlMatcherEngineImpl::handleAction(
       Envoy::Matcher::evaluateMatch<Http::HttpMatchingData>(*matcher_, data);
   ASSERT(result.isComplete());
   if (result.isMatch()) {
-    auto action = result.action()->getTyped<Action>();
+    const 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.
-    auto rbac_action = action.action();
+    const 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::ActionFactoryCb
-  createActionFactoryCb(const Protobuf::Message& config, ActionContext& context,
-                        ProtobufMessage::ValidationVisitor& validation_visitor) override;
+  Envoy::Matcher::ActionConstSharedPtr
+  createAction(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>();