From 7ceb768a2e87781289a6f887da2f85de67530026 Mon Sep 17 00:00:00 2001
From: Maxime Coste <mawww@kakoune.org>
Date: Fri, 23 Jun 2017 09:54:21 +0100
Subject: [PATCH] Use an HashMap to store options in option manager

Turns out looking for options can get pretty slow, so O(1) lookup
seems better.

This should improve the performance of the #1460 issue
---
 src/option_manager.cc | 26 ++++++++++++--------------
 src/option_manager.hh | 21 ++++++++-------------
 src/string.hh         |  1 +
 3 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/src/option_manager.cc b/src/option_manager.cc
index d842a254..b3c56d31 100644
--- a/src/option_manager.cc
+++ b/src/option_manager.cc
@@ -48,13 +48,13 @@ struct option_not_found : public runtime_error
 
 Option& OptionManager::get_local_option(StringView name)
 {
-    auto it = find_option(m_options, name);
+    auto it = m_options.find(name);
     if (it != m_options.end())
-        return **it;
+        return *(it->value);
     else if (m_parent)
     {
-        m_options.emplace_back((*m_parent)[name].clone(*this));
-        return *m_options.back();
+        auto* clone = (*m_parent)[name].clone(*this);
+        return *m_options.insert({clone->name(), std::unique_ptr<Option>{clone}});
     }
     else
         throw option_not_found(name);
@@ -63,9 +63,9 @@ Option& OptionManager::get_local_option(StringView name)
 
 Option& OptionManager::operator[](StringView name)
 {
-    auto it = find_option(m_options, name);
+    auto it = m_options.find(name);
     if (it != m_options.end())
-        return **it;
+        return *it->value;
     else if (m_parent)
         return (*m_parent)[name];
     else
@@ -80,10 +80,9 @@ const Option& OptionManager::operator[](StringView name) const
 void OptionManager::unset_option(StringView name)
 {
     kak_assert(m_parent); // cannot unset option on global manager
-    auto it = find_option(m_options, name);
-    if (it != m_options.end())
+    if (m_options.contains(name))
     {
-        m_options.erase(it);
+        m_options.erase(name);
         on_option_changed((*m_parent)[name]);
     }
 }
@@ -93,11 +92,11 @@ OptionManager::OptionList OptionManager::flatten_options() const
     OptionList res = m_parent ? m_parent->flatten_options() : OptionList{};
     for (auto& option : m_options)
     {
-        auto it = find_option(res, option->name());
+        auto it = find_if(res, [&](const Option* opt) { return opt->name() == option.key; });
         if (it != res.end())
-            *it = option.get();
+            *it = option.value.get();
         else
-            res.emplace_back(option.get());
+            res.emplace_back(option.value.get());
     }
     return res;
 }
@@ -105,8 +104,7 @@ OptionManager::OptionList OptionManager::flatten_options() const
 void OptionManager::on_option_changed(const Option& option)
 {
     // if parent option changed, but we overrided it, it's like nothing happened
-    if (&option.manager() != this and
-        find_option(m_options, option.name()) != m_options.end())
+    if (&option.manager() != this and m_options.contains(option.name()))
         return;
 
     // The watcher list might get mutated during calls to on_option_changed
diff --git a/src/option_manager.hh b/src/option_manager.hh
index 97556749..190fdd9c 100644
--- a/src/option_manager.hh
+++ b/src/option_manager.hh
@@ -6,6 +6,8 @@
 #include "exception.hh"
 #include "option.hh"
 #include "vector.hh"
+#include "hash_map.hh"
+#include "utils.hh"
 
 #include <memory>
 #include <type_traits>
@@ -101,7 +103,7 @@ private:
     friend class Scope;
     friend class OptionsRegistry;
 
-    Vector<std::unique_ptr<Option>, MemoryDomain::Options> m_options;
+    HashMap<StringView, std::unique_ptr<Option>, MemoryDomain::Options> m_options;
     OptionManager* m_parent;
 
     mutable Vector<OptionManagerWatcher*, MemoryDomain::Options> m_watchers;
@@ -190,13 +192,6 @@ template<typename T> bool Option::is_of_type() const
     return dynamic_cast<const TypedOption<T>*>(this) != nullptr;
 }
 
-template<typename T>
-auto find_option(T& container, StringView name) -> decltype(container.begin())
-{
-    using ptr_type = decltype(*container.begin());
-    return find_if(container, [&name](const ptr_type& opt) { return opt->name() == name; });
-}
-
 class OptionsRegistry
 {
 public:
@@ -217,18 +212,18 @@ public:
             throw runtime_error{format("name '{}' contains char out of [a-zA-Z0-9_]", name)};
 
         auto& opts = m_global_manager.m_options;
-        auto it = find_option(opts, name);
+        auto it = opts.find(name);
         if (it != opts.end())
         {
-            if ((*it)->is_of_type<T>() and (*it)->flags() == flags)
-                return **it;
+            if (it->value->is_of_type<T>() and it->value->flags() == flags)
+                return *it->value;
             throw runtime_error{format("option '{}' already declared with different type or flags", name)};
         }
         String doc =  docstring.empty() ? format("[{}]", option_type_name(Meta::Type<T>{}))
                                         : format("[{}] - {}", option_type_name(Meta::Type<T>{}), docstring);
         m_descs.emplace_back(new OptionDesc{name.str(), std::move(doc), flags});
-        opts.emplace_back(new TypedCheckedOption<T, validator>{m_global_manager, *m_descs.back(), value});
-        return *opts.back();
+        return *opts.insert({m_descs.back()->name(),
+                             make_unique<TypedCheckedOption<T, validator>>(m_global_manager, *m_descs.back(), value)});
     }
 
     const OptionDesc* option_desc(StringView name) const
diff --git a/src/string.hh b/src/string.hh
index bfafa482..418ce298 100644
--- a/src/string.hh
+++ b/src/string.hh
@@ -257,6 +257,7 @@ private:
 static_assert(std::is_trivial<StringView>::value, "");
 
 template<> struct HashCompatible<String, StringView> : std::true_type {};
+template<> struct HashCompatible<StringView, String> : std::true_type {};
 
 inline String::String(StringView str) : String{str.begin(), str.length()} {}