From 4606453fedafefc42392c7c78428d8c649319741 Mon Sep 17 00:00:00 2001 From: Maxime Coste Date: Wed, 7 Jun 2017 12:15:16 +0100 Subject: [PATCH] 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. --- src/commands.cc | 18 +++++++++--------- src/hook_manager.cc | 42 ++++++++++++++++++++++++++---------------- src/hook_manager.hh | 4 +++- 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 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& 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& 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(); const bool profile = debug_flags & DebugFlags::Profile; auto start_time = profile ? Clock::now() : TimePoint{}; auto& disabled_hooks = context.options()["disabled_hooks"].get(); - Vector hooks_to_run; - for (auto& hook : hook_list_it->value) + Vector 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 m_parent; - HashMap, MemoryDomain::Hooks> m_hooks; + HashMap, MemoryDomain::Hooks>, MemoryDomain::Hooks> m_hooks; + mutable Vector, MemoryDomain::Hooks> m_running_hooks; + mutable Vector, MemoryDomain::Hooks> m_hooks_trash; }; }