Merge pull request #10054 from JosJuice/android-game-cache-lock

Android: Reduce gameFileCache lock contention

Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/model/GameFileCache.java

@@ -96,30 +96,41 @@ private static LinkedHashSet<String> getPathSet(boolean removeNonExistentFolders
     return pathSet;
   }
 
-  /**
-   * Scans through the file system and updates the cache to match.
-   *
-   * @return true if the cache was modified
-   */
-  public boolean update()
+  public static String[] getAllGamePaths()
   {
     boolean recursiveScan = BooleanSetting.MAIN_RECURSIVE_ISO_PATHS.getBooleanGlobal();
 
     LinkedHashSet<String> folderPathsSet = getPathSet(true);
 
     String[] folderPaths = folderPathsSet.toArray(new String[0]);
 
-    return update(folderPaths, recursiveScan);
+    return getAllGamePaths(folderPaths, recursiveScan);
   }
 
+  public static native String[] getAllGamePaths(String[] folderPaths, boolean recursiveScan);
+
   public native int getSize();
 
   public native GameFile[] getAllGames();
 
   public native GameFile addOrGet(String gamePath);
 
-  public native boolean update(String[] folderPaths, boolean recursiveScan);
+  /**
+   * Sets the list of games to cache.
+   *
+   * Games which are in the passed-in list but not in the cache are scanned and added to the cache,
+   * and games which are in the cache but not in the passed-in list are removed from the cache.
+   *
+   * @return true if the cache was modified
+   */
+  public native boolean update(String[] gamePaths);
 
+  /**
+   * For each game that already is in the cache, scans the folder that contains the game
+   * for additional metadata files (PNG/XML).
+   *
+   * @return true if the cache was modified
+   */
   public native boolean updateAdditionalMetadata();
 
   public native boolean load();

Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java

@@ -159,9 +159,20 @@ public static void startRescan(Context context)
 
   public static GameFile addOrGet(String gamePath)
   {
-    // The existence of this one function, which is called from one
-    // single place, forces us to use synchronization in onHandleIntent...
-    // A bit annoying, but should be good enough for now
+    // Common case: The game is in the cache, so just grab it from there.
+    // (Actually, addOrGet already checks for this case, but we want to avoid calling it if possible
+    // because onHandleIntent may hold a lock on gameFileCache for extended periods of time.)
+    GameFile[] allGames = gameFiles.get();
+    for (GameFile game : allGames)
+    {
+      if (game.getPath().equals(gamePath))
+      {
+        return game;
+      }
+    }
+
+    // Unusual case: The game wasn't found in the cache.
+    // Scan the game and add it to the cache so that we can return it.
     synchronized (gameFileCache)
     {
       return gameFileCache.addOrGet(gamePath);
@@ -192,26 +203,29 @@ protected void onHandleIntent(Intent intent)
     {
       if (gameFileCache != null)
       {
+        String[] gamePaths = GameFileCache.getAllGamePaths();
+
+        boolean changed;
         synchronized (gameFileCache)
         {
-          boolean changed = gameFileCache.update();
-          if (changed)
-          {
-            updateGameFileArray();
-            sendBroadcast(CACHE_UPDATED);
-          }
-
-          boolean additionalMetadataChanged = gameFileCache.updateAdditionalMetadata();
-          if (additionalMetadataChanged)
-          {
-            updateGameFileArray();
-            sendBroadcast(CACHE_UPDATED);
-          }
-
-          if (changed || additionalMetadataChanged)
-          {
-            gameFileCache.save();
-          }
+          changed = gameFileCache.update(gamePaths);
+        }
+        if (changed)
+        {
+          updateGameFileArray();
+          sendBroadcast(CACHE_UPDATED);
+        }
+
+        boolean additionalMetadataChanged = gameFileCache.updateAdditionalMetadata();
+        if (additionalMetadataChanged)
+        {
+          updateGameFileArray();
+          sendBroadcast(CACHE_UPDATED);
+        }
+
+        if (changed || additionalMetadataChanged)
+        {
+          gameFileCache.save();
         }
       }
 

Source/Android/jni/GameList/GameFileCache.cpp

@@ -38,6 +38,13 @@ JNIEXPORT void JNICALL Java_org_dolphinemu_dolphinemu_model_GameFileCache_finali
   delete GetPointer(env, obj);
 }
 
+JNIEXPORT jobjectArray JNICALL Java_org_dolphinemu_dolphinemu_model_GameFileCache_getAllGamePaths(
+    JNIEnv* env, jclass, jobjectArray folder_paths, jboolean recursive_scan)
+{
+  return VectorToJStringArray(
+      env, UICommon::FindAllGamePaths(JStringArrayToVector(env, folder_paths), recursive_scan));
+}
+
 JNIEXPORT jint JNICALL Java_org_dolphinemu_dolphinemu_model_GameFileCache_getSize(JNIEnv* env,
                                                                                   jobject obj)
 {
@@ -66,10 +73,9 @@ JNIEXPORT jobject JNICALL Java_org_dolphinemu_dolphinemu_model_GameFileCache_add
 }
 
 JNIEXPORT jboolean JNICALL Java_org_dolphinemu_dolphinemu_model_GameFileCache_update(
-    JNIEnv* env, jobject obj, jobjectArray folder_paths, jboolean recursive_scan)
+    JNIEnv* env, jobject obj, jobjectArray game_paths)
 {
-  return GetPointer(env, obj)->Update(
-      UICommon::FindAllGamePaths(JStringArrayToVector(env, folder_paths), recursive_scan));
+  return GetPointer(env, obj)->Update(JStringArrayToVector(env, game_paths));
 }
 
 JNIEXPORT jboolean JNICALL

Source/CMakeLists.txt

@@ -14,6 +14,8 @@ if(CMAKE_SYSTEM_NAME MATCHES "Windows")
   add_definitions(-D_CRT_SECURE_NO_DEPRECATE)
   add_definitions(-D_CRT_NONSTDC_NO_WARNINGS)
   add_definitions(-D_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
+  # The replacement for the old atomic shared_ptr functions was added in C++20, so we can't use it yet
+  add_definitions(-D_SILENCE_CXX20_OLD_SHARED_PTR_ATOMIC_SUPPORT_DEPRECATION_WARNING)
 endif()
 
 if (MSVC)

Source/Core/UICommon/GameFileCache.cpp

@@ -4,6 +4,7 @@
 #include "UICommon/GameFileCache.h"
 
 #include <algorithm>
+#include <atomic>
 #include <cstddef>
 #include <functional>
 #include <list>
@@ -202,7 +203,7 @@ bool GameFileCache::UpdateAdditionalMetadata(std::shared_ptr<GameFile>* game_fil
   if (custom_cover_changed)
     copy->CustomCoverCommit();
 
-  *game_file = std::move(copy);
+  std::atomic_store(game_file, std::move(copy));
 
   return true;
 }

Source/VSProps/Base.props

@@ -64,6 +64,8 @@
       <PreprocessorDefinitions>_WINSOCK_DEPRECATED_NO_WARNINGS;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <!--Currently needed for some code in StringUtil used only on Android-->
       <PreprocessorDefinitions>_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING;%(PreprocessorDefinitions)</PreprocessorDefinitions>
+      <!--The replacement for the old atomic shared_ptr functions was added in C++20, so we can't use it yet-->
+      <PreprocessorDefinitions>_SILENCE_CXX20_OLD_SHARED_PTR_ATOMIC_SUPPORT_DEPRECATION_WARNING;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <PreprocessorDefinitions>USE_UPNP;USE_USBDK;__LIBUSB__;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <PreprocessorDefinitions>SFML_STATIC;%(PreprocessorDefinitions)</PreprocessorDefinitions>
       <PreprocessorDefinitions>USE_ANALYTICS=1;%(PreprocessorDefinitions)</PreprocessorDefinitions>