From bba33c7cede68259c7f2b05e9964960d9e2570e4 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Tue, 24 Aug 2021 14:36:08 +0200 Subject: [PATCH 1/3] Android: Don't hold gameFileCache lock while finding games FindAllGamePaths may take a little while, and holding the gameFileCache lock isn't actually necessary until it's time to put the results returned by FindAllGamePaths into gameFileCache. The downside of this change is that we have to do an extra round of JNI in between FindAllGamePaths and Update, but I don't think that's much of a problem. --- .../dolphinemu/model/GameFileCache.java | 27 +++++++++++++------ .../services/GameFileCacheService.java | 4 ++- Source/Android/jni/GameList/GameFileCache.cpp | 12 ++++++--- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/model/GameFileCache.java b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/model/GameFileCache.java index a6f5261048..ed0f65741a 100644 --- a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/model/GameFileCache.java +++ b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/model/GameFileCache.java @@ -96,12 +96,7 @@ public class GameFileCache 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(); @@ -109,17 +104,33 @@ public class GameFileCache 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(); diff --git a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java index 7cff59a3e9..1a9284a9c4 100644 --- a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java +++ b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java @@ -192,9 +192,11 @@ public final class GameFileCacheService extends IntentService { if (gameFileCache != null) { + String[] gamePaths = GameFileCache.getAllGamePaths(); + synchronized (gameFileCache) { - boolean changed = gameFileCache.update(); + boolean changed = gameFileCache.update(gamePaths); if (changed) { updateGameFileArray(); diff --git a/Source/Android/jni/GameList/GameFileCache.cpp b/Source/Android/jni/GameList/GameFileCache.cpp index 06329a70ac..b79d85db15 100644 --- a/Source/Android/jni/GameList/GameFileCache.cpp +++ b/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 From fb265b610de08df5509e56beeb3dbff68f7d4396 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Tue, 24 Aug 2021 15:07:15 +0200 Subject: [PATCH 2/3] Android: Don't hold gameFileCache lock during updateAdditionalMetadata It seems like we spend a lot of the game list scanning time in updateAdditionalMetadata, which I suppose makes sense considering how many different files that function attempts to open. With the addition of just one little atomic operation, we can make it safe to call updateAdditionalMetadata without holding a lock. --- .../services/GameFileCacheService.java | 33 ++++++++++--------- Source/CMakeLists.txt | 2 ++ Source/Core/UICommon/GameFileCache.cpp | 3 +- Source/VSProps/Base.props | 2 ++ 4 files changed, 23 insertions(+), 17 deletions(-) diff --git a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java index 1a9284a9c4..a8ecc367c9 100644 --- a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java +++ b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java @@ -194,26 +194,27 @@ public final class GameFileCacheService extends IntentService { String[] gamePaths = GameFileCache.getAllGamePaths(); + boolean changed; synchronized (gameFileCache) { - boolean changed = gameFileCache.update(gamePaths); - if (changed) - { - updateGameFileArray(); - sendBroadcast(CACHE_UPDATED); - } + changed = gameFileCache.update(gamePaths); + } + if (changed) + { + updateGameFileArray(); + sendBroadcast(CACHE_UPDATED); + } - boolean additionalMetadataChanged = gameFileCache.updateAdditionalMetadata(); - if (additionalMetadataChanged) - { - updateGameFileArray(); - sendBroadcast(CACHE_UPDATED); - } + boolean additionalMetadataChanged = gameFileCache.updateAdditionalMetadata(); + if (additionalMetadataChanged) + { + updateGameFileArray(); + sendBroadcast(CACHE_UPDATED); + } - if (changed || additionalMetadataChanged) - { - gameFileCache.save(); - } + if (changed || additionalMetadataChanged) + { + gameFileCache.save(); } } diff --git a/Source/CMakeLists.txt b/Source/CMakeLists.txt index 3f0afcaf34..8ac7f4fe26 100644 --- a/Source/CMakeLists.txt +++ b/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) diff --git a/Source/Core/UICommon/GameFileCache.cpp b/Source/Core/UICommon/GameFileCache.cpp index a0bf0caf42..30bbca924b 100644 --- a/Source/Core/UICommon/GameFileCache.cpp +++ b/Source/Core/UICommon/GameFileCache.cpp @@ -4,6 +4,7 @@ #include "UICommon/GameFileCache.h" #include +#include #include #include #include @@ -202,7 +203,7 @@ bool GameFileCache::UpdateAdditionalMetadata(std::shared_ptr* game_fil if (custom_cover_changed) copy->CustomCoverCommit(); - *game_file = std::move(copy); + std::atomic_store(game_file, std::move(copy)); return true; } diff --git a/Source/VSProps/Base.props b/Source/VSProps/Base.props index 79489143a7..e656544188 100644 --- a/Source/VSProps/Base.props +++ b/Source/VSProps/Base.props @@ -64,6 +64,8 @@ _WINSOCK_DEPRECATED_NO_WARNINGS;%(PreprocessorDefinitions) _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING;%(PreprocessorDefinitions) + + _SILENCE_CXX20_OLD_SHARED_PTR_ATOMIC_SUPPORT_DEPRECATION_WARNING;%(PreprocessorDefinitions) USE_UPNP;USE_USBDK;__LIBUSB__;%(PreprocessorDefinitions) SFML_STATIC;%(PreprocessorDefinitions) USE_ANALYTICS=1;%(PreprocessorDefinitions) From 719930bb390ed0020b99a7942289d83218e99d69 Mon Sep 17 00:00:00 2001 From: JosJuice Date: Tue, 24 Aug 2021 15:24:52 +0200 Subject: [PATCH 3/3] Android: Add fast path to addOrGet This path isn't really any faster in the normal case, but it does let us skip waiting for the lock to be available, which makes a huge difference if the lock is already taken. --- .../services/GameFileCacheService.java | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java index a8ecc367c9..3be5fb039f 100644 --- a/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java +++ b/Source/Android/app/src/main/java/org/dolphinemu/dolphinemu/services/GameFileCacheService.java @@ -159,9 +159,20 @@ public final class GameFileCacheService extends IntentService 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);