PPCSymbolDB: Reduce lock contention in LoadMap/LoadMapOnBoot

By building the map in a local variable and then swapping it with the
member variable, we avoid the need to hold a lock while building the
map.

Source/Core/Common/SymbolDB.cpp

@@ -5,6 +5,7 @@
 
 #include <cstring>
 #include <map>
+#include <mutex>
 #include <string>
 #include <utility>
 
@@ -49,6 +50,8 @@ bool SymbolDB::IsEmpty() const
 
 bool SymbolDB::Clear(const char* prefix)
 {
+  std::lock_guard lock(m_mutex);
+
   // TODO: honor prefix
   m_map_name.clear();
   if (IsEmpty())
@@ -61,9 +64,14 @@ bool SymbolDB::Clear(const char* prefix)
 }
 
 void SymbolDB::Index()
+{
+  Index(&m_functions);
+}
+
+void SymbolDB::Index(XFuncMap* functions)
 {
   int i = 0;
-  for (auto& func : m_functions)
+  for (auto& func : *functions)
   {
     func.second.index = i++;
   }

Source/Core/Common/SymbolDB.h

@@ -7,6 +7,7 @@
 #pragma once
 
 #include <map>
+#include <mutex>
 #include <set>
 #include <string>
 #include <string_view>
@@ -105,9 +106,12 @@ public:
   void Index();
 
 protected:
+  static void Index(XFuncMap* functions);
+
   XFuncMap m_functions;
   XNoteMap m_notes;
   XFuncPtrMap m_checksum_to_function;
   std::string m_map_name;
+  std::mutex m_mutex;
 };
 }  // namespace Common

Source/Core/Core/PowerPC/PPCSymbolDB.cpp

@@ -56,8 +56,17 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
                                  const std::string& name, const std::string& object_name,
                                  Common::Symbol::Type type)
 {
-  auto iter = m_functions.find(startAddr);
+  AddKnownSymbol(guard, startAddr, size, name, object_name, type, &m_functions,
-  if (iter != m_functions.end())
+                 &m_checksum_to_function);
+}
+
+void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAddr, u32 size,
+                                 const std::string& name, const std::string& object_name,
+                                 Common::Symbol::Type type, XFuncMap* functions,
+                                 XFuncPtrMap* checksum_to_function)
+{
+  auto iter = functions->find(startAddr);
+  if (iter != functions->end())
   {
     // already got it, let's just update name, checksum & size to be sure.
     Common::Symbol* tempfunc = &iter->second;
@@ -70,7 +79,7 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
   else
   {
     // new symbol. run analyze.
-    auto& new_symbol = m_functions.emplace(startAddr, name).first->second;
+    auto& new_symbol = functions->emplace(startAddr, name).first->second;
     new_symbol.object_name = object_name;
     new_symbol.type = type;
     new_symbol.address = startAddr;
@@ -85,7 +94,7 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
                      name, size, new_symbol.size);
         new_symbol.size = size;
       }
-      m_checksum_to_function[new_symbol.hash].insert(&new_symbol);
+      (*checksum_to_function)[new_symbol.hash].insert(&new_symbol);
     }
     else
     {
@@ -96,9 +105,14 @@ void PPCSymbolDB::AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAdd
 
 void PPCSymbolDB::AddKnownNote(u32 start_addr, u32 size, const std::string& name)
 {
-  auto iter = m_notes.find(start_addr);
+  AddKnownNote(start_addr, size, name, &m_notes);
+}
 
-  if (iter != m_notes.end())
+void PPCSymbolDB::AddKnownNote(u32 start_addr, u32 size, const std::string& name, XNoteMap* notes)
+{
+  auto iter = notes->find(start_addr);
+
+  if (iter != notes->end())
   {
     // Already got it, just update the name and size.
     Common::Note* tempfunc = &iter->second;
@@ -112,22 +126,27 @@ void PPCSymbolDB::AddKnownNote(u32 start_addr, u32 size, const std::string& name
     tf.address = start_addr;
     tf.size = size;
 
-    m_notes[start_addr] = tf;
+    (*notes)[start_addr] = tf;
   }
 }
 
 void PPCSymbolDB::DetermineNoteLayers()
 {
-  if (m_notes.empty())
+  DetermineNoteLayers(&m_notes);
+}
+
+void PPCSymbolDB::DetermineNoteLayers(XNoteMap* notes)
+{
+  if (notes->empty())
     return;
 
-  for (auto& note : m_notes)
+  for (auto& note : *notes)
     note.second.layer = 0;
 
-  for (auto iter = m_notes.begin(); iter != m_notes.end(); ++iter)
+  for (auto iter = notes->begin(); iter != notes->end(); ++iter)
   {
     const u32 range = iter->second.address + iter->second.size;
-    auto search = m_notes.lower_bound(range);
+    auto search = notes->lower_bound(range);
 
     while (--search != iter)
       search->second.layer += 1;
@@ -312,21 +331,19 @@ bool PPCSymbolDB::FindMapFile(std::string* existing_map_file, std::string* writa
 // Returns true if m_functions was changed.
 bool PPCSymbolDB::LoadMapOnBoot(const Core::CPUThreadGuard& guard)
 {
-  std::lock_guard lock(m_mutex);
-
   std::string existing_map_file;
   if (!PPCSymbolDB::FindMapFile(&existing_map_file, nullptr))
     return Clear();
 
+  {
+    std::lock_guard lock(m_mutex);
     // If the map is already loaded (such as restarting the same game), skip reloading.
     if (!IsEmpty() && existing_map_file == m_map_name)
       return false;
+  }
 
-  // Load map into cleared m_functions.
+  if (!LoadMap(guard, std::move(existing_map_file)))
-  bool changed = Clear();
+    return Clear();
-
-  if (!LoadMap(guard, existing_map_file))
-    return changed;
 
   return true;
 }
@@ -338,14 +355,11 @@ bool PPCSymbolDB::LoadMapOnBoot(const Core::CPUThreadGuard& guard)
 // function names and addresses that have a BLR before the start and at the end, but ignore any that
 // don't, and then tell you how many were good and how many it ignored. That way you either find out
 // it is all good and use it, find out it is partly good and use the good part, or find out that
-// only
+// only a handful of functions lined up by coincidence and then you can clear the symbols. In the
-// a handful of functions lined up by coincidence and then you can clear the symbols. In the future
+// future I want to make it smarter, so it checks that there are no BLRs in the middle of the
-// I
+// function (by checking the code length), and also make it cope with added functions in the middle
-// want to make it smarter, so it checks that there are no BLRs in the middle of the function
+// or work based on the order of the functions and their approximate length. Currently that process
-// (by checking the code length), and also make it cope with added functions in the middle or work
+// has to be done manually and is very tedious.
-// based on the order of the functions and their approximate length. Currently that process has to
-// be
-// done manually and is very tedious.
 // The use case for separate handling of map files that aren't bad is that you usually want to also
 // load names that aren't functions(if included in the map file) without them being rejected as
 // invalid.
@@ -356,12 +370,16 @@ bool PPCSymbolDB::LoadMapOnBoot(const Core::CPUThreadGuard& guard)
 // This one can load both leftover map files on game discs (like Zelda), and mapfiles
 // produced by SaveSymbolMap below.
 // bad=true means carefully load map files that might not be from exactly the right version
-bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, const std::string& filename, bool bad)
+bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, std::string filename, bool bad)
 {
   File::IOFile f(filename, "r");
   if (!f)
     return false;
 
+  XFuncMap new_functions;
+  XNoteMap new_notes;
+  XFuncPtrMap checksum_to_function;
+
   // Two columns are used by Super Smash Bros. Brawl Korean map file
   // Three columns are commonly used
   // Four columns are used in American Mensa Academy map files and perhaps other games
@@ -573,9 +591,14 @@ bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, const std::string&
         ++good_count;
 
         if (section_name == ".note")
-          AddKnownNote(vaddress, size, name);
+        {
+          AddKnownNote(vaddress, size, name, &new_notes);
+        }
         else
-          AddKnownSymbol(guard, vaddress, size, name_string, object_filename_string, type);
+        {
+          AddKnownSymbol(guard, vaddress, size, name_string, object_filename_string, type,
+                         &new_functions, &checksum_to_function);
+        }
       }
       else
       {
@@ -584,10 +607,15 @@ bool PPCSymbolDB::LoadMap(const Core::CPUThreadGuard& guard, const std::string&
     }
   }
 
-  m_map_name = filename;
+  Index(&new_functions);
+  DetermineNoteLayers(&new_notes);
+
+  std::lock_guard lock(m_mutex);
+  std::swap(m_functions, new_functions);
+  std::swap(m_notes, new_notes);
+  std::swap(m_checksum_to_function, checksum_to_function);
+  std::swap(m_map_name, filename);
 
-  Index();
-  DetermineNoteLayers();
   NOTICE_LOG_FMT(SYMBOLS, "{} symbols loaded, {} symbols ignored.", good_count, bad_count);
   return true;
 }

Source/Core/Core/PowerPC/PPCSymbolDB.h

@@ -3,7 +3,6 @@
 
 #pragma once
 
-#include <mutex>
 #include <string>
 #include <string_view>
 
@@ -40,7 +39,7 @@ public:
   void FillInCallers();
 
   bool LoadMapOnBoot(const Core::CPUThreadGuard& guard);
-  bool LoadMap(const Core::CPUThreadGuard& guard, const std::string& filename, bool bad = false);
+  bool LoadMap(const Core::CPUThreadGuard& guard, std::string filename, bool bad = false);
   bool SaveSymbolMap(const std::string& filename) const;
   bool SaveCodeMap(const Core::CPUThreadGuard& guard, const std::string& filename) const;
 
@@ -51,5 +50,11 @@ public:
   static bool FindMapFile(std::string* existing_map_file, std::string* writable_map_file);
 
 private:
-  std::mutex m_mutex;
+  static void AddKnownSymbol(const Core::CPUThreadGuard& guard, u32 startAddr, u32 size,
+                             const std::string& name, const std::string& object_name,
+                             Common::Symbol::Type type, XFuncMap* functions,
+                             XFuncPtrMap* checksum_to_function);
+  static void AddKnownNote(u32 start_addr, u32 size, const std::string& name, XNoteMap* notes);
+
+  static void DetermineNoteLayers(XNoteMap* notes);
 };