Merge pull request #1646 from piepacker/jake/osd_locking_opt

optimize locking mechanism for OSD messages (deadlock-proofing)

src/frontend-common/common_host_interface.cpp

@@ -934,30 +934,26 @@ void CommonHostInterface::AddOSDMessage(std::string message, float duration /*=
   msg.duration = duration;
 
   std::unique_lock<std::mutex> lock(m_osd_messages_lock);
-  m_osd_messages.push_back(std::move(msg));
+  m_osd_posted_messages.push_back(std::move(msg));
 }
 
 void CommonHostInterface::ClearOSDMessages()
 {
   std::unique_lock<std::mutex> lock(m_osd_messages_lock);
-  m_osd_messages.clear();
+  m_osd_posted_messages.clear();
 }
 
 bool CommonHostInterface::EnumerateOSDMessages(std::function<bool(const std::string&, float)> callback)
 {
-  std::unique_lock<std::mutex> lock(m_osd_messages_lock);
+  auto iter = m_osd_active_messages.begin();
-  if (m_osd_messages.empty())
+  while (iter != m_osd_active_messages.end())
-    return true;
-
-  auto iter = m_osd_messages.begin();
-  while (iter != m_osd_messages.end())
   {
     const OSDMessage& msg = *iter;
     const double time = msg.time.GetTimeSeconds();
     const float time_remaining = static_cast<float>(msg.duration - time);
     if (time_remaining <= 0.0f)
     {
-      iter = m_osd_messages.erase(iter);
+      iter = m_osd_active_messages.erase(iter);
       continue;
     }
 
@@ -967,7 +963,7 @@ bool CommonHostInterface::EnumerateOSDMessages(std::function<bool(const std::str
       continue;
     }
 
-    if (!callback(iter->text, time_remaining))
+    if (callback && !callback(iter->text, time_remaining))
       return false;
 
     ++iter;
@@ -976,17 +972,49 @@ bool CommonHostInterface::EnumerateOSDMessages(std::function<bool(const std::str
   return true;
 }
 
+void CommonHostInterface::AcquirePendingOSDMessages()
+{
+  // memory_order_consume is roughly equivalent to adding a volatile keyword to the read from the deque.
+  // we just want to force the compiler to always reload the deque size value from memory.
+  //
+  // ARM doesn't have good atomic read guarantees so it _could_ read some non-zero value here spuriously,
+  // but that's OK because we lock the mutex later and recheck things anyway. This early out will still 
+  // avoid 99.99% of the unnecessary lock attempts when size == 0.
+
+  std::atomic_thread_fence(std::memory_order_consume);
+  if (!m_osd_posted_messages.empty())
+  {
+    std::unique_lock<std::mutex> lock(m_osd_messages_lock);
+    for(;;)
+    {
+      // lock-and-copy mechanism.
+      // this allows us to unlock the deque and minimize time that the mutex is held.
+      // it is almost always the best model to follow for multithread deque.
+
+      if (m_osd_posted_messages.empty())
+        break;
+
+      m_osd_active_messages.push_back(std::move(m_osd_posted_messages.front()));
+      m_osd_posted_messages.pop_front();
+
+      // somewhat arbitrary hard cap on # of messages. This might be unnecessarily paranoid. If something is
+      // spamming the osd message log this badly, then probably this isn't going to really help things much.
+      static constexpr size_t MAX_ACTIVE_OSD_MESSAGES = 512;
+      if (m_osd_active_messages.size() > MAX_ACTIVE_OSD_MESSAGES)
+        m_osd_active_messages.pop_front();
+    }
+  }
+}
+
 void CommonHostInterface::DrawOSDMessages()
 {
+  AcquirePendingOSDMessages();
+
   constexpr ImGuiWindowFlags window_flags = ImGuiWindowFlags_NoTitleBar | ImGuiWindowFlags_NoInputs |
                                             ImGuiWindowFlags_NoMove | ImGuiWindowFlags_NoSavedSettings |
                                             ImGuiWindowFlags_NoScrollbar | ImGuiWindowFlags_NoNav |
                                             ImGuiWindowFlags_AlwaysAutoResize | ImGuiWindowFlags_NoFocusOnAppearing;
 
-  std::unique_lock<std::mutex> lock(m_osd_messages_lock);
-  if (m_osd_messages.empty())
-    return;
-
   const float scale = ImGui::GetIO().DisplayFramebufferScale.x;
   const float spacing = 5.0f * scale;
   const float margin = 10.0f * scale;
@@ -996,40 +1024,30 @@ void CommonHostInterface::DrawOSDMessages()
   float position_x = margin;
   float position_y = margin;
 
-  ImDrawList* dl = ImGui::GetBackgroundDrawList();
+  EnumerateOSDMessages(
-  ImFont* font = ImGui::GetFont();
+    [max_width, spacing, padding, rounding, &position_x, &position_y](const std::string& message, float time_remaining) -> bool {
-  auto iter = m_osd_messages.begin();
+      const float opacity = std::min(time_remaining, 1.0f);
-  while (iter != m_osd_messages.end())
+
-  {
+      if (position_y >= ImGui::GetIO().DisplaySize.y)
-    const OSDMessage& msg = *iter;
+        return false;
-    const double time = msg.time.GetTimeSeconds();
+
-    const float time_remaining = static_cast<float>(msg.duration - time);
+      const ImVec2 pos(position_x, position_y);
-    if (time_remaining <= 0.0f)
+      const ImVec2 text_size(ImGui::CalcTextSize(message.c_str(), nullptr, false, max_width));
-    {
+      const ImVec2 size(text_size.x + padding * 2.0f, text_size.y + padding * 2.0f);
-      iter = m_osd_messages.erase(iter);
+      const ImVec4 text_rect(pos.x + padding, pos.y + padding, pos.x + size.x - padding, pos.y + size.y - padding);
-      continue;
+
+      ImDrawList* dl = ImGui::GetBackgroundDrawList();
+      ImFont* font = ImGui::GetFont();
+      dl->AddRectFilled(pos, ImVec2(pos.x + size.x, pos.y + size.y), ImGui::GetColorU32(ImGuiCol_WindowBg, opacity),
+                        rounding);
+      dl->AddRect(pos, ImVec2(pos.x + size.x, pos.y + size.y), ImGui::GetColorU32(ImGuiCol_Border), rounding);
+      dl->AddText(font, font->FontSize, ImVec2(text_rect.x, text_rect.y), ImGui::GetColorU32(ImGuiCol_Text, opacity),
+                  message.c_str(), nullptr, max_width, &text_rect);
+      position_y += size.y + spacing;
+
+      return true;
     }
-
+  );
-    if (!g_settings.display_show_osd_messages || position_y >= ImGui::GetIO().DisplaySize.y)
-    {
-      ++iter;
-      continue;
-    }
-
-    const float opacity = std::min(time_remaining, 1.0f);
-
-    const ImVec2 pos(position_x, position_y);
-    const ImVec2 text_size(ImGui::CalcTextSize(msg.text.c_str(), nullptr, false, max_width));
-    const ImVec2 size(text_size.x + padding * 2.0f, text_size.y + padding * 2.0f);
-    const ImVec4 text_rect(pos.x + padding, pos.y + padding, pos.x + size.x - padding, pos.y + size.y - padding);
-    dl->AddRectFilled(pos, ImVec2(pos.x + size.x, pos.y + size.y), ImGui::GetColorU32(ImGuiCol_WindowBg, opacity),
-                      rounding);
-    dl->AddRect(pos, ImVec2(pos.x + size.x, pos.y + size.y), ImGui::GetColorU32(ImGuiCol_Border), rounding);
-    dl->AddText(font, font->FontSize, ImVec2(text_rect.x, text_rect.y), ImGui::GetColorU32(ImGuiCol_Text, opacity),
-                msg.text.c_str(), nullptr, max_width, &text_rect);
-    position_y += size.y + spacing;
-    ++iter;
-  }
 }
 
 void CommonHostInterface::DrawDebugWindows()

src/frontend-common/common_host_interface.h

@@ -184,6 +184,9 @@ public:
   bool EnumerateOSDMessages(std::function<bool(const std::string&, float)> callback);
   void ClearOSDMessages();
 
+  /// async message queue bookeeping for. Should be called on UI thread.
+  void AcquirePendingOSDMessages();
+
   /// Displays a loading screen with the logo, rendered with ImGui. Use when executing possibly-time-consuming tasks
   /// such as compiling shaders when starting up.
   void DisplayLoadingScreen(const char* message, int progress_min = -1, int progress_max = -1,
@@ -394,7 +397,8 @@ protected:
 
   std::unique_ptr<HostDisplayTexture> m_logo_texture;
 
-  std::deque<OSDMessage> m_osd_messages;
+  std::deque<OSDMessage> m_osd_active_messages;   // accessed only by GUI/OSD thread (no lock reqs)
+  std::deque<OSDMessage> m_osd_posted_messages;          // written to by multiple threads.
   std::mutex m_osd_messages_lock;
 
   bool m_fullscreen_ui_enabled = false;

src/frontend-common/fullscreen_ui.cpp

@@ -2712,12 +2712,7 @@ void DrawStatsOverlay()
 
 void DrawOSDMessages()
 {
-  if (!g_settings.display_show_osd_messages)
+  s_host_interface->AcquirePendingOSDMessages();
-  {
-    // we still need to remove them from the queue
-    s_host_interface->EnumerateOSDMessages([](const std::string& message, float time_remaining) { return true; });
-    return;
-  }
 
   ImGui::PushFont(g_large_font);