Avoid expensive copies of Hooks in HookManager::run_hooks

Use a deferred deletion mechanism to ensure hooks are kept alive
for the duration of run_hooks.

3 files changed, 38 insertions, 26 deletions

diff --git a/src/commands.cc b/src/commands.cc
index 835a246f..17570843 100644
--- a/src/commands.cc
+++ b/src/commands.cc
@@ -810,16 +810,16 @@ const CommandDesc add_hook_cmd = {
             ScopedSetBool disable_history{context.history_disabled()};
 
             MatchResults<const char*> res;
-            if (regex_match(param.begin(), param.end(), res, regex))
-            {
-                EnvVarMap env_vars{ {"hook_param", param.str()} };
-                for (size_t i = 0; i < res.size(); ++i)
-                    env_vars.insert({format("hook_param_capture_{}", i),
-                                     {res[i].first, res[i].second}});
+            if (not regex_match(param.begin(), param.end(), res, regex))
+                return;
 
-                CommandManager::instance().execute(command, context,
-                                                   { {}, std::move(env_vars) });
-            }
+            EnvVarMap env_vars{ {"hook_param", param.str()} };
+            for (size_t i = 0; i < res.size(); ++i)
+                env_vars.insert({format("hook_param_capture_{}", i),
+                                 {res[i].first, res[i].second}});
+
+            CommandManager::instance().execute(command, context,
+                                               { {}, std::move(env_vars) });
         };
         auto group = parser.get_switch("group").value_or(StringView{});
         get_scope(parser[0], context).hooks().add_hook(parser[1], group.str(), std::move(hook_func));
diff --git a/src/hook_manager.cc b/src/hook_manager.cc
index c9410345..9a412373 100644
--- a/src/hook_manager.cc
+++ b/src/hook_manager.cc
@@ -15,7 +15,7 @@ namespace Kakoune
 void HookManager::add_hook(StringView hook_name, String group, HookFunc func)
 {
     auto& hooks = m_hooks[hook_name];
-    hooks.push_back({std::move(group), std::move(func)});
+    hooks.emplace_back(new Hook{std::move(group), std::move(func)});
 }
 
 void HookManager::remove_hooks(StringView group)
@@ -23,9 +23,15 @@ void HookManager::remove_hooks(StringView group)
     if (group.empty())
         throw runtime_error("invalid id");
     for (auto& list : m_hooks)
-        list.value.erase(std::remove_if(list.value.begin(), list.value.end(),
-                                        [&](const Hook& h) { return h.group == group; }),
-                         list.value.end());
+    {
+        auto it = std::remove_if(list.value.begin(), list.value.end(),
+                                 [&](const std::unique_ptr<Hook>& h)
+                                 { return h->group == group; });
+        if (not m_running_hooks.empty()) // we are running some hooks, defer deletion
+            m_hooks_trash.insert(m_hooks_trash.end(), std::make_move_iterator(it),
+                                 std::make_move_iterator(list.value.end()));
+        list.value.erase(it, list.value.end());
+    }
 }
 
 CandidateList HookManager::complete_hook_group(StringView prefix, ByteCount pos_in_token)
@@ -33,7 +39,7 @@ CandidateList HookManager::complete_hook_group(StringView prefix, ByteCount pos_
     CandidateList res;
     for (auto& list : m_hooks)
     {
-        auto container = list.value | transform(std::mem_fn(&decltype(list.value)::value_type::group));
+        auto container = list.value | transform([](const std::unique_ptr<Hook>& h) -> const String& { return h->group; });
         for (auto& c : complete(prefix, pos_in_token, container))
         {
             if (!contains(res, c))
@@ -52,8 +58,8 @@ void HookManager::run_hook(StringView hook_name,
     if (m_parent)
         m_parent->run_hook(hook_name, param, context);
 
-    auto hook_list_it = m_hooks.find(hook_name);
-    if (hook_list_it == m_hooks.end())
+    auto hook_list = m_hooks.find(hook_name);
+    if (hook_list == m_hooks.end())
         return;
 
     if (contains(m_running_hooks, std::make_pair(hook_name, param)))
@@ -64,19 +70,23 @@ void HookManager::run_hook(StringView hook_name,
     }
 
     m_running_hooks.emplace_back(hook_name, param);
-    auto pop_running_hook = on_scope_end([this]{ m_running_hooks.pop_back(); });
+    auto pop_running_hook = on_scope_end([this]{
+        m_running_hooks.pop_back();
+        if (m_running_hooks.empty())
+            m_hooks_trash.clear();
+    });
 
     const DebugFlags debug_flags = context.options()["debug"].get<DebugFlags>();
     const bool profile = debug_flags & DebugFlags::Profile;
     auto start_time = profile ? Clock::now() : TimePoint{};
 
     auto& disabled_hooks = context.options()["disabled_hooks"].get<Regex>();
-    Vector<Hook> hooks_to_run;
-    for (auto& hook : hook_list_it->value)
+    Vector<Hook*> hooks_to_run; // The m_hooks_trash vector ensure hooks wont die during this method
+    for (auto& hook : hook_list->value)
     {
-        if (hook.group.empty() or disabled_hooks.empty() or
-            not regex_match(hook.group.begin(), hook.group.end(), disabled_hooks))
-            hooks_to_run.push_back(hook);
+        if (hook->group.empty() or disabled_hooks.empty() or
+            not regex_match(hook->group.begin(), hook->group.end(), disabled_hooks))
+            hooks_to_run.push_back(hook.get());
     }
 
     bool hook_error = false;
@@ -85,14 +95,14 @@ void HookManager::run_hook(StringView hook_name,
         try
         {
             if (debug_flags & DebugFlags::Hooks)
-                write_to_debug_buffer(format("hook {}({})/{}", hook_name, param, hook.group));
-            hook.func(param, context);
+                write_to_debug_buffer(format("hook {}({})/{}", hook_name, param, hook->group));
+            hook->func(param, context);
         }
         catch (runtime_error& err)
         {
             hook_error = true;
             write_to_debug_buffer(format("error running hook {}({})/{}: {}",
-                               hook_name, param, hook.group, err.what()));
+                               hook_name, param, hook->group, err.what()));
         }
     }
 
diff --git a/src/hook_manager.hh b/src/hook_manager.hh
index ae27b352..e2e238ea 100644
--- a/src/hook_manager.hh
+++ b/src/hook_manager.hh
@@ -35,8 +35,10 @@ private:
     };
 
     SafePtr<HookManager> m_parent;
-    HashMap<String, Vector<Hook, MemoryDomain::Hooks>, MemoryDomain::Hooks> m_hooks;
+    HashMap<String, Vector<std::unique_ptr<Hook>, MemoryDomain::Hooks>, MemoryDomain::Hooks> m_hooks;
+
     mutable Vector<std::pair<StringView, StringView>, MemoryDomain::Hooks> m_running_hooks;
+    mutable Vector<std::unique_ptr<Hook>, MemoryDomain::Hooks> m_hooks_trash;
 };
 
 }