optimize UserMountCache::registerStorage

Signed-off-by: Robin Appelman <robin@icewind.nl> Signed-off-by: Benjamin Gaussorgues <benjamin.gaussorgues@nextcloud.com>
nextcloud · Nov 16, 2023 · 7a2d994 · 7a2d994
1 parent 9c3350b
commit 7a2d994
Show file tree

Hide file tree

Showing 6 changed files with 91 additions and 71 deletions.
diff --git a/.htaccess b/.htaccess
@@ -96,3 +96,7 @@
 
 AddDefaultCharset utf-8
 Options -Indexes
+#### DO NOT CHANGE ANYTHING ABOVE THIS LINE ####
+
+ErrorDocument 403 /index.php/error/403
+ErrorDocument 404 /index.php/error/404
diff --git a/lib/private/Files/Config/CachedMountInfo.php b/lib/private/Files/Config/CachedMountInfo.php
@@ -35,6 +35,7 @@ class CachedMountInfo implements ICachedMountInfo {
 	protected ?int $mountId;
 	protected string $rootInternalPath;
 	protected string $mountProvider;
+	protected string $key;
 
 	/**
 	 * CachedMountInfo constructor.
@@ -65,6 +66,7 @@ public function __construct(
 			throw new \Exception("Mount provider $mountProvider name exceeds the limit of 128 characters");
 		}
 		$this->mountProvider = $mountProvider;
+		$this->key = $rootId . '::' . $mountPoint;
 	}
 
 	/**
@@ -132,4 +134,8 @@ public function getRootInternalPath(): string {
 	public function getMountProvider(): string {
 		return $this->mountProvider;
 	}
+
+	public function getKey(): string {
+		return $this->key;
+	}
 }
diff --git a/lib/private/Files/Config/LazyStorageMountInfo.php b/lib/private/Files/Config/LazyStorageMountInfo.php
@@ -39,6 +39,7 @@ public function __construct(IUser $user, IMountPoint $mount) {
 		$this->rootId = 0;
 		$this->storageId = 0;
 		$this->mountPoint = '';
+		$this->key = '';
 	}
 
 	/**
@@ -87,4 +88,11 @@ public function getRootInternalPath(): string {
 	public function getMountProvider(): string {
 		return $this->mount->getMountProvider();
 	}
+
+	public function getKey(): string {
+		if (!$this->key) {
+			$this->key = $this->getRootId() . '::' . $this->getMountPoint();
+		}
+		return $this->key;
+	}
 }
diff --git a/lib/private/Files/Config/UserMountCache.php b/lib/private/Files/Config/UserMountCache.php
@@ -29,13 +29,11 @@
 namespace OC\Files\Config;
 
 use OCP\Cache\CappedMemoryCache;
-use OCA\Files_Sharing\SharedMount;
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\Diagnostics\IEventLogger;
 use OCP\Files\Config\ICachedMountFileInfo;
 use OCP\Files\Config\ICachedMountInfo;
 use OCP\Files\Config\IUserMountCache;
-use OCP\Files\Mount\IMountPoint;
 use OCP\Files\NotFoundException;
 use OCP\IDBConnection;
 use OCP\IUser;
@@ -78,41 +76,27 @@ public function __construct(
 
 	public function registerMounts(IUser $user, array $mounts, array $mountProviderClasses = null) {
 		$this->eventLogger->start('fs:setup:user:register', 'Registering mounts for user');
-		// filter out non-proper storages coming from unit tests
-		$mounts = array_filter($mounts, function (IMountPoint $mount) {
-			return $mount instanceof SharedMount || ($mount->getStorage() && $mount->getStorage()->getCache());
-		});
-		/** @var ICachedMountInfo[] $newMounts */
-		$newMounts = array_map(function (IMountPoint $mount) use ($user) {
+		/** @var array<string, ICachedMountInfo> $newMounts */
+		$newMounts = [];
+		foreach ($mounts as $mount) {
 			// filter out any storages which aren't scanned yet since we aren't interested in files from those storages (yet)
-			if ($mount->getStorageRootId() === -1) {
-				return null;
-			} else {
-				return new LazyStorageMountInfo($user, $mount);
+			if ($mount->getStorageRootId() !== -1) {
+				$mountInfo = new LazyStorageMountInfo($user, $mount);
+				$newMounts[$mountInfo->getKey()] = $mountInfo;
 			}
-		}, $mounts);
-		$newMounts = array_values(array_filter($newMounts));
-		$newMountKeys = array_map(function (ICachedMountInfo $mount) {
-			return $mount->getRootId() . '::' . $mount->getMountPoint();
-		}, $newMounts);
-		$newMounts = array_combine($newMountKeys, $newMounts);
+		}
 
 		$cachedMounts = $this->getMountsForUser($user);
 		if (is_array($mountProviderClasses)) {
 			$cachedMounts = array_filter($cachedMounts, function (ICachedMountInfo $mountInfo) use ($mountProviderClasses, $newMounts) {
 				// for existing mounts that didn't have a mount provider set
 				// we still want the ones that map to new mounts
-				$mountKey = $mountInfo->getRootId() . '::' . $mountInfo->getMountPoint();
-				if ($mountInfo->getMountProvider() === '' && isset($newMounts[$mountKey])) {
+				if ($mountInfo->getMountProvider() === '' && isset($newMounts[$mountInfo->getKey()])) {
 					return true;
 				}
 				return in_array($mountInfo->getMountProvider(), $mountProviderClasses);
 			});
 		}
-		$cachedRootKeys = array_map(function (ICachedMountInfo $mount) {
-			return $mount->getRootId() . '::' . $mount->getMountPoint();
-		}, $cachedMounts);
-		$cachedMounts = array_combine($cachedRootKeys, $cachedMounts);
 
 		$addedMounts = [];
 		$removedMounts = [];
@@ -131,46 +115,44 @@ public function registerMounts(IUser $user, array $mounts, array $mountProviderC
 
 		$changedMounts = $this->findChangedMounts($newMounts, $cachedMounts);
 
-		$this->connection->beginTransaction();
-		try {
-			foreach ($addedMounts as $mount) {
-				$this->addToCache($mount);
-				/** @psalm-suppress InvalidArgument */
-				$this->mountsForUsers[$user->getUID()][] = $mount;
-			}
-			foreach ($removedMounts as $mount) {
-				$this->removeFromCache($mount);
-				$index = array_search($mount, $this->mountsForUsers[$user->getUID()]);
-				unset($this->mountsForUsers[$user->getUID()][$index]);
-			}
-			foreach ($changedMounts as $mount) {
-				$this->updateCachedMount($mount);
+		if ($addedMounts || $removedMounts || $changedMounts) {
+			$this->connection->beginTransaction();
+			$userUID = $user->getUID();
+			try {
+				foreach ($addedMounts as $mount) {
+					$this->addToCache($mount);
+					/** @psalm-suppress InvalidArgument */
+					$this->mountsForUsers[$userUID][$mount->getKey()] = $mount;
+				}
+				foreach ($removedMounts as $mount) {
+					$this->removeFromCache($mount);
+					unset($this->mountsForUsers[$userUID][$mount->getKey()]);
+				}
+				foreach ($changedMounts as $mount) {
+					$this->updateCachedMount($mount);
+					/** @psalm-suppress InvalidArgument */
+					$this->mountsForUsers[$userUID][$mount->getKey()] = $mount;
+				}
+				$this->connection->commit();
+			} catch (\Throwable $e) {
+				$this->connection->rollBack();
+				throw $e;
 			}
-			$this->connection->commit();
-		} catch (\Throwable $e) {
-			$this->connection->rollBack();
-			throw $e;
 		}
 		$this->eventLogger->end('fs:setup:user:register');
 	}
 
 	/**
-	 * @param ICachedMountInfo[] $newMounts
-	 * @param ICachedMountInfo[] $cachedMounts
+	 * @param array<string, ICachedMountInfo> $newMounts
+	 * @param array<string, ICachedMountInfo> $cachedMounts
 	 * @return ICachedMountInfo[]
 	 */
 	private function findChangedMounts(array $newMounts, array $cachedMounts) {
-		$new = [];
-		foreach ($newMounts as $mount) {
-			$new[$mount->getRootId() . '::' . $mount->getMountPoint()] = $mount;
-		}
 		$changed = [];
-		foreach ($cachedMounts as $cachedMount) {
-			$key = $cachedMount->getRootId() . '::' . $cachedMount->getMountPoint();
-			if (isset($new[$key])) {
-				$newMount = $new[$key];
+		foreach ($cachedMounts as $key => $cachedMount) {
+			if (isset($newMounts[$key])) {
+				$newMount = $newMounts[$key];
 				if (
-					$newMount->getMountPoint() !== $cachedMount->getMountPoint() ||
 					$newMount->getStorageId() !== $cachedMount->getStorageId() ||
 					$newMount->getMountId() !== $cachedMount->getMountId() ||
 					$newMount->getMountProvider() !== $cachedMount->getMountProvider()
@@ -247,20 +229,28 @@ private function dbRowToMountInfo(array $row) {
 	 * @return ICachedMountInfo[]
 	 */
 	public function getMountsForUser(IUser $user) {
-		if (!isset($this->mountsForUsers[$user->getUID()])) {
+		$userUID = $user->getUID();
+		if (!isset($this->mountsForUsers[$userUID])) {
 			$builder = $this->connection->getQueryBuilder();
 			$query = $builder->select('storage_id', 'root_id', 'user_id', 'mount_point', 'mount_id', 'f.path', 'mount_provider_class')
 				->from('mounts', 'm')
 				->innerJoin('m', 'filecache', 'f', $builder->expr()->eq('m.root_id', 'f.fileid'))
-				->where($builder->expr()->eq('user_id', $builder->createPositionalParameter($user->getUID())));
+				->where($builder->expr()->eq('user_id', $builder->createPositionalParameter($userUID)));
 
 			$result = $query->execute();
 			$rows = $result->fetchAll();
 			$result->closeCursor();
 
-			$this->mountsForUsers[$user->getUID()] = array_filter(array_map([$this, 'dbRowToMountInfo'], $rows));
+			$this->mountsForUsers[$userUID] = [];
+			/** @var array<string, ICachedMountInfo> $mounts */
+			foreach ($rows as $row) {
+				$mount = $this->dbRowToMountInfo($row);
+				if ($mount !== null) {
+					$this->mountsForUsers[$userUID][$mount->getKey()] = $mount;
+				}
+			}
 		}
-		return $this->mountsForUsers[$user->getUID()];
+		return $this->mountsForUsers[$userUID];
 	}
 
 	/**

diff --git a/lib/public/Files/Config/ICachedMountInfo.php b/lib/public/Files/Config/ICachedMountInfo.php
@@ -84,4 +84,12 @@ public function getRootInternalPath(): string;
 	 * @since 24.0.0
 	 */
 	public function getMountProvider(): string;
+
+	/**
+	 * Get a key that uniquely identifies the mount
+	 *
+	 * @return string
+	 * @since 28.0.0
+	 */
+	public function getKey(): string;
 }
diff --git a/tests/lib/Files/Config/UserMountCacheTest.php b/tests/lib/Files/Config/UserMountCacheTest.php
@@ -118,6 +118,10 @@ private function clearCache() {
 		$this->invokePrivate($this->cache, 'mountsForUsers', [new CappedMemoryCache()]);
 	}
 
+	private function keyForMount(MountPoint $mount): string {
+		return $mount->getStorageRootId().'::'.$mount->getMountPoint();
+	}
+
 	public function testNewMounts() {
 		$user = $this->userManager->get('u1');
 
@@ -131,7 +135,7 @@ public function testNewMounts() {
 		$cachedMounts = $this->cache->getMountsForUser($user);
 
 		$this->assertCount(1, $cachedMounts);
-		$cachedMount = $cachedMounts[0];
+		$cachedMount = $cachedMounts[$this->keyForMount($mount)];
 		$this->assertEquals('/asd/', $cachedMount->getMountPoint());
 		$this->assertEquals($user, $cachedMount->getUser());
 		$this->assertEquals($storage->getCache()->getId(''), $cachedMount->getRootId());
@@ -155,7 +159,7 @@ public function testSameMounts() {
 		$cachedMounts = $this->cache->getMountsForUser($user);
 
 		$this->assertCount(1, $cachedMounts);
-		$cachedMount = $cachedMounts[0];
+		$cachedMount = $cachedMounts[$this->keyForMount($mount)];
 		$this->assertEquals('/asd/', $cachedMount->getMountPoint());
 		$this->assertEquals($user, $cachedMount->getUser());
 		$this->assertEquals($storage->getCache()->getId(''), $cachedMount->getRootId());
@@ -200,7 +204,7 @@ public function testChangeMounts() {
 		$cachedMounts = $this->cache->getMountsForUser($user);
 
 		$this->assertCount(1, $cachedMounts);
-		$cachedMount = $cachedMounts[0];
+		$cachedMount = $cachedMounts[$this->keyForMount($mount)];
 		$this->assertEquals('/foo/', $cachedMount->getMountPoint());
 	}
 
@@ -223,7 +227,7 @@ public function testChangeMountId() {
 		$cachedMounts = $this->cache->getMountsForUser($user);
 
 		$this->assertCount(1, $cachedMounts);
-		$cachedMount = $cachedMounts[0];
+		$cachedMount = $cachedMounts[$this->keyForMount($mount)];
 		$this->assertEquals(1, $cachedMount->getMountId());
 	}
 
@@ -248,15 +252,15 @@ public function testGetMountsForUser() {
 		$cachedMounts = $this->cache->getMountsForUser($user1);
 
 		$this->assertCount(2, $cachedMounts);
-		$this->assertEquals('/foo/', $cachedMounts[0]->getMountPoint());
-		$this->assertEquals($user1, $cachedMounts[0]->getUser());
-		$this->assertEquals($id1, $cachedMounts[0]->getRootId());
-		$this->assertEquals(1, $cachedMounts[0]->getStorageId());
+		$this->assertEquals('/foo/', $cachedMounts[$this->keyForMount($mount1)]->getMountPoint());
+		$this->assertEquals($user1, $cachedMounts[$this->keyForMount($mount1)]->getUser());
+		$this->assertEquals($id1, $cachedMounts[$this->keyForMount($mount1)]->getRootId());
+		$this->assertEquals(1, $cachedMounts[$this->keyForMount($mount1)]->getStorageId());
 
-		$this->assertEquals('/bar/', $cachedMounts[1]->getMountPoint());
-		$this->assertEquals($user1, $cachedMounts[1]->getUser());
-		$this->assertEquals($id2, $cachedMounts[1]->getRootId());
-		$this->assertEquals(2, $cachedMounts[1]->getStorageId());
+		$this->assertEquals('/bar/', $cachedMounts[$this->keyForMount($mount2)]->getMountPoint());
+		$this->assertEquals($user1, $cachedMounts[$this->keyForMount($mount2)]->getUser());
+		$this->assertEquals($id2, $cachedMounts[$this->keyForMount($mount2)]->getRootId());
+		$this->assertEquals(2, $cachedMounts[$this->keyForMount($mount2)]->getStorageId());
 
 		$cachedMounts = $this->cache->getMountsForUser($user3);
 		$this->assertEmpty($cachedMounts);
@@ -521,7 +525,7 @@ public function testMigrateMountProvider() {
 
 		$cachedMounts = $this->cache->getMountsForUser($user1);
 		$this->assertCount(1, $cachedMounts);
-		$this->assertEquals('', $cachedMounts[0]->getMountProvider());
+		$this->assertEquals('', $cachedMounts[$this->keyForMount($mount1)]->getMountProvider());
 
 		$mount1 = new MountPoint($storage1, '/foo/', null, null, null, null, 'dummy');
 		$this->cache->registerMounts($user1, [$mount1], ['dummy']);
@@ -530,6 +534,6 @@ public function testMigrateMountProvider() {
 
 		$cachedMounts = $this->cache->getMountsForUser($user1);
 		$this->assertCount(1, $cachedMounts);
-		$this->assertEquals('dummy', $cachedMounts[0]->getMountProvider());
+		$this->assertEquals('dummy', $cachedMounts[$this->keyForMount($mount1)]->getMountProvider());
 	}
 }