From e081abb9e691014f6e7423802a7f3f7df9d58c85 Mon Sep 17 00:00:00 2001
From: Thales Macedo Garitezi <thalesmg@gmail.com>
Date: Tue, 12 Sep 2023 16:29:43 -0300
Subject: [PATCH] perf(ps_router): only check if there are any routes when
 deciding whether to persist message

---
 apps/emqx/src/emqx_persistent_session_ds.erl       | 10 ++++------
 .../emqx/src/emqx_persistent_session_ds_router.erl | 14 ++++++++++++++
 apps/emqx/src/emqx_topic_index.erl                 |  1 +
 .../emqx_persistent_session_ds_router_SUITE.erl    | 10 ++++++++++
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/apps/emqx/src/emqx_persistent_session_ds.erl b/apps/emqx/src/emqx_persistent_session_ds.erl
index e2984e30b..68ae15c6c 100644
--- a/apps/emqx/src/emqx_persistent_session_ds.erl
+++ b/apps/emqx/src/emqx_persistent_session_ds.erl
@@ -71,11 +71,9 @@ init() ->
     ok | {skipped, _Reason} | {error, _TODO}.
 persist_message(Msg) ->
     ?WHEN_ENABLED(
-        case needs_persistence(Msg) andalso find_subscribers(Msg) of
-            [_ | _] ->
+        case needs_persistence(Msg) andalso has_subscribers(Msg) of
+            true ->
                 store_message(Msg);
-            [] ->
-                {skipped, no_subscribers};
             false ->
                 {skipped, needs_no_persistence}
         end
@@ -90,8 +88,8 @@ store_message(Msg) ->
     Topic = emqx_topic:words(emqx_message:topic(Msg)),
     emqx_ds_storage_layer:store(?DS_SHARD, ID, Timestamp, Topic, serialize_message(Msg)).
 
-find_subscribers(#message{topic = Topic}) ->
-    emqx_persistent_session_ds_router:match_routes(Topic).
+has_subscribers(#message{topic = Topic}) ->
+    emqx_persistent_session_ds_router:has_any_route(Topic).
 
 open_session(ClientID) ->
     ?WHEN_ENABLED(emqx_ds:session_open(ClientID)).
diff --git a/apps/emqx/src/emqx_persistent_session_ds_router.erl b/apps/emqx/src/emqx_persistent_session_ds_router.erl
index cbb5175bc..324db991a 100644
--- a/apps/emqx/src/emqx_persistent_session_ds_router.erl
+++ b/apps/emqx/src/emqx_persistent_session_ds_router.erl
@@ -25,6 +25,7 @@
 -export([
     do_add_route/2,
     do_delete_route/2,
+    has_any_route/1,
     match_routes/1,
     lookup_routes/1,
     foldr_routes/2,
@@ -101,6 +102,19 @@ do_delete_route(Topic, Dest) ->
             mria_route_tab_delete(#ps_route{topic = Topic, dest = Dest})
     end.
 
+%% @doc Takes a real topic (not filter) as input, and returns whether there is any
+%% matching filters.
+-spec has_any_route(emqx_types:topic()) -> boolean().
+has_any_route(Topic) ->
+    DirectTopicMatch = lookup_route_tab(Topic),
+    WildcardMatch = emqx_topic_index:match(Topic, ?PS_FILTERS_TAB),
+    case {DirectTopicMatch, WildcardMatch} of
+        {[], false} ->
+            false;
+        {_, _} ->
+            true
+    end.
+
 %% @doc Take a real topic (not filter) as input, return the matching topics and topic
 %% filters associated with route destination.
 -spec match_routes(emqx_types:topic()) -> [emqx_types:route()].
diff --git a/apps/emqx/src/emqx_topic_index.erl b/apps/emqx/src/emqx_topic_index.erl
index 59dfdfeab..9b1982cc6 100644
--- a/apps/emqx/src/emqx_topic_index.erl
+++ b/apps/emqx/src/emqx_topic_index.erl
@@ -66,6 +66,7 @@ match(Topic, Tab) ->
 
 %% @doc Match given topic against the index and return _all_ matches.
 %% If `unique` option is given, return only unique matches by record ID.
+-spec matches(emqx_types:topic(), ets:table(), emqx_trie_search:opts()) -> [key(_ID)].
 matches(Topic, Tab, Opts) ->
     emqx_trie_search:matches(Topic, make_nextf(Tab), Opts).
 
diff --git a/apps/emqx/test/emqx_persistent_session_ds_router_SUITE.erl b/apps/emqx/test/emqx_persistent_session_ds_router_SUITE.erl
index 742dc5b41..3e48173c3 100644
--- a/apps/emqx/test/emqx_persistent_session_ds_router_SUITE.erl
+++ b/apps/emqx/test/emqx_persistent_session_ds_router_SUITE.erl
@@ -80,12 +80,21 @@ delete_route(TopicFilter) ->
 %     error('TODO').
 
 t_add_delete(_) ->
+    ?assertNot(?R:has_any_route(<<"a/b/c">>)),
     add_route(<<"a/b/c">>),
+    ?assert(?R:has_any_route(<<"a/b/c">>)),
     add_route(<<"a/b/c">>),
+    ?assert(?R:has_any_route(<<"a/b/c">>)),
     add_route(<<"a/+/b">>),
+    ?assert(?R:has_any_route(<<"a/b/c">>)),
+    ?assert(?R:has_any_route(<<"a/c/b">>)),
     ?assertEqual([<<"a/+/b">>, <<"a/b/c">>], lists:sort(?R:topics())),
     delete_route(<<"a/b/c">>),
+    ?assertNot(?R:has_any_route(<<"a/b/c">>)),
+    ?assert(?R:has_any_route(<<"a/c/b">>)),
     delete_route(<<"a/+/b">>),
+    ?assertNot(?R:has_any_route(<<"a/b/c">>)),
+    ?assertNot(?R:has_any_route(<<"a/c/b">>)),
     ?assertEqual([], ?R:topics()).
 
 t_add_delete_incremental(_) ->
@@ -94,6 +103,7 @@ t_add_delete_incremental(_) ->
     add_route(<<"a/+/+">>),
     add_route(<<"a/b/#">>),
     add_route(<<"#">>),
+    ?assert(?R:has_any_route(<<"any/topic">>)),
     ?assertEqual(
         [
             #ps_route{topic = <<"#">>, dest = ?DEF_DS_SESSION_ID},