From 8d6744e5dd36f16a8f3e1cacbd4673c5294cf796 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Thu, 12 Oct 2023 10:56:55 +0200 Subject: [PATCH 1/3] Extract common ets heir helper under ejabberd_sup --- src/ejabberd_sup.erl | 28 +++++++++++++++++++ .../mod_global_distrib_utils.erl | 9 +----- src/mod_keystore.erl | 28 +------------------ src/wpool/mongoose_wpool.erl | 17 ++++------- src/wpool/mongoose_wpool_http.erl | 13 +-------- src/wpool/mongoose_wpool_rdbms.erl | 14 ++-------- 6 files changed, 39 insertions(+), 70 deletions(-) diff --git a/src/ejabberd_sup.erl b/src/ejabberd_sup.erl index b0b12c71d2c..0ccc21f47b2 100644 --- a/src/ejabberd_sup.erl +++ b/src/ejabberd_sup.erl @@ -30,6 +30,7 @@ -export([start_link/0, init/1]). -export([start_child/1, start_child/2, stop_child/1]). +-export([create_ets_table/2]). -include("mongoose_logger.hrl"). @@ -119,3 +120,30 @@ worker_spec(Mod) -> worker_spec(Mod, Args) -> {Mod, {Mod, start_link, Args}, permanent, timer:seconds(5), worker, [Mod]}. + +-spec create_ets_table(atom(), list()) -> ok. +create_ets_table(TableName, TableOpts) -> + case does_table_exist(TableName) of + true -> ok; + false -> + Opts = maybe_add_heir(whereis(?MODULE), self(), TableOpts), + ets:new(TableName, Opts), + ok + end. + +does_table_exist(TableName) -> + undefined =/= ets:info(TableName, name). + +%% In tests or when module is started in run-time, we need to set heir to the +%% ETS table, otherwise it will be destroyed when the creator's process finishes. +%% When started normally during node start up, self() =:= EjdSupPid and there +%% is no need for setting heir +maybe_add_heir(EjdSupPid, EjdSupPid, BaseOpts) when is_pid(EjdSupPid) -> + BaseOpts; +maybe_add_heir(EjdSupPid, _Self, BaseOpts) when is_pid(EjdSupPid) -> + case lists:keymember(heir, 1, BaseOpts) of + true -> BaseOpts; + false -> [{heir, EjdSupPid, testing} | BaseOpts] + end; +maybe_add_heir(_, _, BaseOpts) -> + BaseOpts. diff --git a/src/global_distrib/mod_global_distrib_utils.erl b/src/global_distrib/mod_global_distrib_utils.erl index 7c92782c978..75f89e42011 100644 --- a/src/global_distrib/mod_global_distrib_utils.erl +++ b/src/global_distrib/mod_global_distrib_utils.erl @@ -111,14 +111,7 @@ create_ets(Names) -> create_ets(Names, Type) when is_list(Names) -> [create_ets(Name, Type) || Name <- Names]; create_ets(Name, Type) -> - Self = self(), - Heir = case whereis(ejabberd_sup) of - undefined -> {heir, none}; - Self -> {heir, none}; - Pid -> {heir, Pid, testing} - end, - - ets:new(Name, [named_table, public, Type, {read_concurrency, true}, Heir]). + ejabberd_sup:create_ets_table(Name, [named_table, public, Type, {read_concurrency, true}]). -spec resolve_endpoints([{inet:ip_address() | string(), inet:port_number()}]) -> [endpoint()]. resolve_endpoints(Endpoints) when is_list(Endpoints) -> diff --git a/src/mod_keystore.erl b/src/mod_keystore.erl index 6dec2728a77..d1ca38fbb63 100644 --- a/src/mod_keystore.erl +++ b/src/mod_keystore.erl @@ -49,7 +49,7 @@ -spec start(mongooseim:host_type(), gen_mod:module_opts()) -> ok. start(HostType, Opts) -> - create_keystore_ets(), + ejabberd_sup:create_ets_table(keystore, [named_table, public, {read_concurrency, true}]), mod_keystore_backend:init(HostType, Opts), init_keys(HostType, Opts), ok. @@ -129,36 +129,10 @@ get_key(HandlerAcc, #{key_id := KeyID}, _) -> %% %% Internal functions %% - -create_keystore_ets() -> - case does_table_exist(keystore) of - true -> ok; - false -> - BaseOpts = [named_table, public, - {read_concurrency, true}], - Opts = maybe_add_heir(whereis(ejabberd_sup), self(), BaseOpts), - ets:new(keystore, Opts), - ok - end. - -%% In tests or when module is started in run-time, we need to set heir to the -%% ETS table, otherwise it will be destroy when the creator's process finishes. -%% When started normally during node start up, self() =:= EjdSupPid and there -%% is no need for setting heir -maybe_add_heir(EjdSupPid, EjdSupPid, BaseOpts) when is_pid(EjdSupPid) -> - BaseOpts; -maybe_add_heir(EjdSupPid, _Self, BaseOpts) when is_pid(EjdSupPid) -> - [{heir, EjdSupPid, testing} | BaseOpts]; -maybe_add_heir(_, _, BaseOpts) -> - BaseOpts. - clear_keystore_ets(HostType) -> Pattern = {{'_', HostType}, '$1'}, ets:match_delete(keystore, Pattern). -does_table_exist(NameOrTID) -> - ets:info(NameOrTID, name) /= undefined. - init_keys(HostType, Opts = #{keys := Keys}) -> maps:map(fun(KeyName, KeyType) -> init_key({KeyName, KeyType}, HostType, Opts) end, Keys). diff --git a/src/wpool/mongoose_wpool.erl b/src/wpool/mongoose_wpool.erl index 4d9b0a69e6a..52f5acf26b6 100644 --- a/src/wpool/mongoose_wpool.erl +++ b/src/wpool/mongoose_wpool.erl @@ -106,17 +106,12 @@ ensure_started() -> _ -> ok end, - - case ets:info(?MODULE) of - undefined -> - % we set heir here because the whole thing may be started by an ephemeral process - ets:new(?MODULE, [named_table, public, - {read_concurrency, true}, - {keypos, #mongoose_wpool.name}, - {heir, whereis(mongoose_wpool_sup), undefined}]); - _ -> - ok - end. + ejabberd_sup:create_ets_table( + ?MODULE, + [named_table, public, + {read_concurrency, true}, + {keypos, #mongoose_wpool.name}, + {heir, whereis(mongoose_wpool_sup), undefined}]). start_configured_pools() -> Pools = mongoose_config:get_opt(outgoing_pools), diff --git a/src/wpool/mongoose_wpool_http.erl b/src/wpool/mongoose_wpool_http.erl index fc848a34497..d1298c67e43 100644 --- a/src/wpool/mongoose_wpool_http.erl +++ b/src/wpool/mongoose_wpool_http.erl @@ -20,18 +20,7 @@ %% mongoose_wpool callbacks -spec init() -> ok. init() -> - case ets:info(?MODULE) of - undefined -> - Heir = case whereis(ejabberd_sup) of - undefined -> []; - Pid -> [{heir, Pid, undefined}] - end, - ets:new(?MODULE, - [named_table, public, {read_concurrency, true} | Heir]), - ok; - _ -> - ok - end. + ejabberd_sup:create_ets_table(?MODULE, [named_table, public, {read_concurrency, true}]). -spec start(mongooseim:host_type_or_global(), mongoose_wpool:tag(), mongoose_wpool:pool_opts(), mongoose_wpool:conn_opts()) -> {ok, pid()} | {error, any()}. diff --git a/src/wpool/mongoose_wpool_rdbms.erl b/src/wpool/mongoose_wpool_rdbms.erl index c7f02b3b99e..6fa953c7c3a 100644 --- a/src/wpool/mongoose_wpool_rdbms.erl +++ b/src/wpool/mongoose_wpool_rdbms.erl @@ -9,18 +9,8 @@ %% mongoose_wpool callbacks -spec init() -> ok. init() -> - case ets:info(prepared_statements) of - undefined -> - Heir = case whereis(ejabberd_sup) of - undefined -> []; - Pid -> [{heir, Pid, undefined}] - end, - ets:new(prepared_statements, - [named_table, public, {read_concurrency, true} | Heir]), - ok; - _ -> - ok - end. + ejabberd_sup:create_ets_table( + prepared_statements, [named_table, public, {read_concurrency, true}]). -spec start(mongooseim:host_type_or_global(), mongoose_wpool:tag(), mongoose_wpool:pool_opts(), mongoose_wpool:conn_opts()) -> {ok, pid()} | {error, any()}. From d3fb26d2d09224b81e04fd7c7f076042118055b5 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Thu, 12 Oct 2023 11:20:27 +0200 Subject: [PATCH 2/3] Move mod_register mnesia table into ets --- src/mod_register.erl | 34 +++++++++------------------------- 1 file changed, 9 insertions(+), 25 deletions(-) diff --git a/src/mod_register.erl b/src/mod_register.erl index f06cd25f724..cdfe42c011d 100644 --- a/src/mod_register.erl +++ b/src/mod_register.erl @@ -46,18 +46,13 @@ -include("mongoose.hrl"). -include("jlib.hrl"). -include("mongoose_config_spec.hrl"). +-define(TABLE, mod_register_ip). -spec start(mongooseim:host_type(), gen_mod:module_opts()) -> ok. start(HostType, #{iqdisc := IQDisc}) -> [gen_iq_handler:add_iq_handler_for_domain(HostType, ?NS_REGISTER, Component, Fn, #{}, IQDisc) || {Component, Fn} <- iq_handlers()], - %% TODO Add CETS backend, use mongoose_mnesia - mnesia:create_table(mod_register_ip, - [{ram_copies, [node()]}, - {local_content, true}, - {attributes, [key, value]}]), - mnesia:add_table_copy(mod_register_ip, node(), ram_copies), - ok. + ejabberd_sup:create_ets_table(?TABLE, [named_table, public, {read_concurrency, true}]). -spec stop(mongooseim:host_type()) -> ok. stop(HostType) -> @@ -440,35 +435,24 @@ check_timeout(Source) -> true -> Priority = -(erlang:system_time(second)), CleanPriority = Priority + Timeout, - F = fun() -> check_and_store_ip_entry(Source, Priority, CleanPriority) end, - - case mnesia:transaction(F) of - {atomic, Res} -> - Res; - {aborted, Reason} -> - ?LOG_ERROR(#{what => reg_check_timeout_failed, - reg_source => Source, reason => Reason}), - true - end; + check_and_store_ip_entry(Source, Priority, CleanPriority); false -> true end. check_and_store_ip_entry(Source, Priority, CleanPriority) -> - Treap = case mnesia:read(mod_register_ip, treap, write) of - [] -> - treap:empty(); - [{mod_register_ip, treap, T}] -> T + Treap = case ets:lookup(?TABLE, treap) of + [] -> treap:empty(); + [{treap, T}] -> T end, Treap1 = clean_treap(Treap, CleanPriority), case treap:lookup(Source, Treap1) of error -> - Treap2 = treap:insert(Source, Priority, [], - Treap1), - mnesia:write({mod_register_ip, treap, Treap2}), + Treap2 = treap:insert(Source, Priority, [], Treap1), + ets:insert(?TABLE, {treap, Treap2}), true; {ok, _, _} -> - mnesia:write({mod_register_ip, treap, Treap1}), + ets:insert(?TABLE, {treap, Treap1}), false end. From 3b4a2717c3e69c830eabbca156180f337b6fbcf4 Mon Sep 17 00:00:00 2001 From: Nelson Vides Date: Thu, 12 Oct 2023 15:32:28 +0200 Subject: [PATCH 3/3] mod_register: Migrate from one treap in ets to all in ets The old logic stores a single data structure in the ets table, a "treap", which is then cleaned and scaned, but we might do with simpler code and store the entries directly in the ets table, and clean up by full scans. Using a single treap means that the whole data structure is copied to processes and then garbage collection can be expensive. It also means that two processes adding records might override each other and therefore cause inconsistencies. The pitfall of the ets table is full scans when cleaning old timestamps, but with low timeouts it might be fine, also considering this module should be seldom enabled in public setups. --- src/mod_register.erl | 45 +++++++++++++++++--------------------------- 1 file changed, 17 insertions(+), 28 deletions(-) diff --git a/src/mod_register.erl b/src/mod_register.erl index cdfe42c011d..e5cf74bc6b5 100644 --- a/src/mod_register.erl +++ b/src/mod_register.erl @@ -52,7 +52,11 @@ start(HostType, #{iqdisc := IQDisc}) -> [gen_iq_handler:add_iq_handler_for_domain(HostType, ?NS_REGISTER, Component, Fn, #{}, IQDisc) || {Component, Fn} <- iq_handlers()], - ejabberd_sup:create_ets_table(?TABLE, [named_table, public, {read_concurrency, true}]). + Concurrency = case IQDisc of + one_queue -> []; + _ -> [{read_concurrency, true}] + end, + ejabberd_sup:create_ets_table(?TABLE, [named_table, public | Concurrency]). -spec stop(mongooseim:host_type()) -> ok. stop(HostType) -> @@ -61,7 +65,8 @@ stop(HostType) -> ok. iq_handlers() -> - [{ejabberd_local, fun ?MODULE:process_iq/5}, {ejabberd_sm, fun ?MODULE:process_iq/5}]. + [{ejabberd_local, fun ?MODULE:process_iq/5}, + {ejabberd_sm, fun ?MODULE:process_iq/5}]. -spec hooks(mongooseim:host_type()) -> gen_hook:hook_list(). hooks(HostType) -> @@ -433,40 +438,24 @@ check_timeout(Source) -> Timeout = mongoose_config:get_opt(registration_timeout), case is_integer(Timeout) of true -> - Priority = -(erlang:system_time(second)), - CleanPriority = Priority + Timeout, - check_and_store_ip_entry(Source, Priority, CleanPriority); + TS = erlang:system_time(second), + clean_ets(TS - Timeout), + check_and_store_ip_entry(Source, TS); false -> true end. -check_and_store_ip_entry(Source, Priority, CleanPriority) -> - Treap = case ets:lookup(?TABLE, treap) of - [] -> treap:empty(); - [{treap, T}] -> T - end, - Treap1 = clean_treap(Treap, CleanPriority), - case treap:lookup(Source, Treap1) of - error -> - Treap2 = treap:insert(Source, Priority, [], Treap1), - ets:insert(?TABLE, {treap, Treap2}), +check_and_store_ip_entry(Source, Timestamp) -> + case ets:member(?TABLE, Source) of + false -> + ets:insert(?TABLE, {Source, Timestamp}), true; - {ok, _, _} -> - ets:insert(?TABLE, {treap, Treap1}), + true -> false end. -clean_treap(Treap, CleanPriority) -> - case treap:is_empty(Treap) of - true -> - Treap; - false -> - {_Key, Priority, _Value} = treap:get_root(Treap), - case Priority > CleanPriority of - true -> clean_treap(treap:delete_root(Treap), CleanPriority); - false -> Treap - end - end. +clean_ets(CleanTimestamp) -> + ets:select_delete(?TABLE, [{ {'_', '$1'}, [{'<', '$1', CleanTimestamp}], [true]}]). ip_to_string(Source) when is_tuple(Source) -> inet_parse:ntoa(Source); ip_to_string(undefined) -> "undefined";