Merge pull request #8273 from CookiePLMonster/config-threading-fixes

Threading fixes for config layers

Source/Core/Common/Config/Config.cpp

@@ -5,51 +5,78 @@
 #include <algorithm>
 #include <list>
 #include <map>
+#if __APPLE__
+#include <mutex>
+#else
+#include <shared_mutex>
+#endif
 
 #include "Common/Config/Config.h"
 
 namespace Config
 {
+using Layers = std::map<LayerType, std::shared_ptr<Layer>>;
+
 static Layers s_layers;
 static std::list<ConfigChangedCallback> s_callbacks;
 static u32 s_callback_guards = 0;
 
-Layers* GetLayers()
-{
-  return &s_layers;
-}
+// Mac supports shared_mutex since 10.12 and we're targeting 10.10,
+// so only use unique locks there...
+#if __APPLE__
+static std::mutex s_layers_rw_lock;
+
+using ReadLock = std::unique_lock<std::mutex>;
+using WriteLock = std::unique_lock<std::mutex>;
+#else
+static std::shared_mutex s_layers_rw_lock;
 
-void AddLayer(std::unique_ptr<Layer> layer)
+using ReadLock = std::shared_lock<std::shared_mutex>;
+using WriteLock = std::unique_lock<std::shared_mutex>;
+#endif
+
+void AddLayerInternal(std::shared_ptr<Layer> layer)
 {
-  s_layers[layer->GetLayer()] = std::move(layer);
+  {
+    WriteLock lock(s_layers_rw_lock);
+
+    const Config::LayerType layer_type = layer->GetLayer();
+    s_layers.insert_or_assign(layer_type, std::move(layer));
+  }
   InvokeConfigChangedCallbacks();
 }
 
 void AddLayer(std::unique_ptr<ConfigLayerLoader> loader)
 {
-  AddLayer(std::make_unique<Layer>(std::move(loader)));
+  AddLayerInternal(std::make_shared<Layer>(std::move(loader)));
 }
 
-Layer* GetLayer(LayerType layer)
+std::shared_ptr<Layer> GetLayer(LayerType layer)
 {
-  if (!LayerExists(layer))
-    return nullptr;
-  return s_layers[layer].get();
+  ReadLock lock(s_layers_rw_lock);
+
+  std::shared_ptr<Layer> result;
+  const auto it = s_layers.find(layer);
+  if (it != s_layers.end())
+  {
+    result = it->second;
+  }
+  return result;
 }
 
 void RemoveLayer(LayerType layer)
 {
-  s_layers.erase(layer);
+  {
+    WriteLock lock(s_layers_rw_lock);
+
+    s_layers.erase(layer);
+  }
   InvokeConfigChangedCallbacks();
 }
-bool LayerExists(LayerType layer)
-{
-  return s_layers.find(layer) != s_layers.end();
-}
 
 void AddConfigChangedCallback(ConfigChangedCallback func)
 {
-  s_callbacks.emplace_back(func);
+  s_callbacks.emplace_back(std::move(func));
 }
 
 void InvokeConfigChangedCallbacks()
@@ -64,15 +91,23 @@ void InvokeConfigChangedCallbacks()
 // Explicit load and save of layers
 void Load()
 {
-  for (auto& layer : s_layers)
-    layer.second->Load();
+  {
+    ReadLock lock(s_layers_rw_lock);
+
+    for (auto& layer : s_layers)
+      layer.second->Load();
+  }
   InvokeConfigChangedCallbacks();
 }
 
 void Save()
 {
-  for (auto& layer : s_layers)
-    layer.second->Save();
+  {
+    ReadLock lock(s_layers_rw_lock);
+
+    for (auto& layer : s_layers)
+      layer.second->Save();
+  }
   InvokeConfigChangedCallbacks();
 }
 
@@ -84,13 +119,17 @@ void Init()
 
 void Shutdown()
 {
+  WriteLock lock(s_layers_rw_lock);
+
   s_layers.clear();
   s_callbacks.clear();
 }
 
 void ClearCurrentRunLayer()
 {
-  s_layers[LayerType::CurrentRun] = std::make_unique<Layer>(LayerType::CurrentRun);
+  WriteLock lock(s_layers_rw_lock);
+
+  s_layers.insert_or_assign(LayerType::CurrentRun, std::make_shared<Layer>(LayerType::CurrentRun));
 }
 
 static const std::map<System, std::string> system_to_name = {
@@ -129,13 +168,16 @@ const std::string& GetLayerName(LayerType layer)
 
 LayerType GetActiveLayerForConfig(const ConfigLocation& config)
 {
+  ReadLock lock(s_layers_rw_lock);
+
   for (auto layer : SEARCH_ORDER)
   {
-    if (!LayerExists(layer))
-      continue;
-
-    if (GetLayer(layer)->Exists(config))
-      return layer;
+    const auto it = s_layers.find(layer);
+    if (it != s_layers.end())
+    {
+      if (it->second->Exists(config))
+        return layer;
+    }
   }
 
   // If config is not present in any layer, base layer is considered active.

Source/Core/Common/Config/Config.h

@@ -16,16 +16,12 @@
 
 namespace Config
 {
-using Layers = std::map<LayerType, std::unique_ptr<Layer>>;
 using ConfigChangedCallback = std::function<void()>;
 
 // Layer management
-Layers* GetLayers();
-void AddLayer(std::unique_ptr<Layer> layer);
 void AddLayer(std::unique_ptr<ConfigLayerLoader> loader);
-Layer* GetLayer(LayerType layer);
+std::shared_ptr<Layer> GetLayer(LayerType layer);
 void RemoveLayer(LayerType layer);
-bool LayerExists(LayerType layer);
 
 void AddConfigChangedCallback(ConfigChangedCallback func);
 void InvokeConfigChangedCallbacks();

Source/Core/Common/Config/Layer.cpp

@@ -46,8 +46,14 @@ bool Layer::Exists(const ConfigLocation& location) const
 bool Layer::DeleteKey(const ConfigLocation& location)
 {
   m_is_dirty = true;
-  bool had_value = m_map[location].has_value();
-  m_map[location].reset();
+  bool had_value = false;
+  const auto iter = m_map.find(location);
+  if (iter != m_map.end() && iter->second.has_value())
+  {
+    iter->second.reset();
+    had_value = true;
+  }
+
   return had_value;
 }
 

Source/Core/Common/Config/Layer.h

@@ -103,18 +103,18 @@ class Layer
   void DeleteAllKeys();
 
   template <typename T>
-  T Get(const ConfigInfo<T>& config_info)
+  T Get(const ConfigInfo<T>& config_info) const
   {
     return Get<T>(config_info.location).value_or(config_info.default_value);
   }
 
   template <typename T>
-  std::optional<T> Get(const ConfigLocation& location)
+  std::optional<T> Get(const ConfigLocation& location) const
   {
-    const std::optional<std::string>& str_value = m_map[location];
-    if (!str_value)
+    const auto iter = m_map.find(location);
+    if (iter == m_map.end() || !iter->second.has_value())
       return std::nullopt;
-    return detail::TryParse<T>(*str_value);
+    return detail::TryParse<T>(*iter->second);
   }
 
   template <typename T>
@@ -129,13 +129,13 @@ class Layer
     Set(location, ValueToString(value));
   }
 
-  void Set(const ConfigLocation& location, const std::string& new_value)
+  void Set(const ConfigLocation& location, std::string new_value)
   {
-    std::optional<std::string>& current_value = m_map[location];
-    if (current_value == new_value)
+    const auto iter = m_map.find(location);
+    if (iter != m_map.end() && iter->second == new_value)
       return;
     m_is_dirty = true;
-    current_value = new_value;
+    m_map.insert_or_assign(location, std::move(new_value));
   }
 
   Section GetSection(System system, const std::string& section);