From b942850af93cf5bcee05bc4099899956ee0f3747 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 23 Oct 2023 13:27:13 +0200 Subject: [PATCH] optimize UserMountCache::registerStorage Signed-off-by: Robin Appelman Signed-off-by: Benjamin Gaussorgues --- lib/private/Files/Config/CachedMountInfo.php | 6 + .../Files/Config/LazyStorageMountInfo.php | 8 ++ lib/private/Files/Config/UserMountCache.php | 104 ++++++++---------- lib/public/Files/Config/ICachedMountInfo.php | 8 ++ tests/lib/Files/Config/UserMountCacheTest.php | 32 +++--- 5 files changed, 87 insertions(+), 71 deletions(-) diff --git a/lib/private/Files/Config/CachedMountInfo.php b/lib/private/Files/Config/CachedMountInfo.php index 43c9fae63ec..7c97135a565 100644 --- 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 @@ class CachedMountInfo implements ICachedMountInfo { throw new \Exception("Mount provider $mountProvider name exceeds the limit of 128 characters"); } $this->mountProvider = $mountProvider; + $this->key = $rootId . '::' . $mountPoint; } /** @@ -132,4 +134,8 @@ class CachedMountInfo implements ICachedMountInfo { 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 index 78055a2cdb8..7e4acb2e129 100644 --- a/lib/private/Files/Config/LazyStorageMountInfo.php +++ b/lib/private/Files/Config/LazyStorageMountInfo.php @@ -39,6 +39,7 @@ class LazyStorageMountInfo extends CachedMountInfo { $this->rootId = 0; $this->storageId = 0; $this->mountPoint = ''; + $this->key = ''; } /** @@ -87,4 +88,11 @@ class LazyStorageMountInfo extends CachedMountInfo { 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 index 7502d65d044..2fb7a7d83f4 100644 --- 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 @@ class UserMountCache implements IUserMountCache { 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 $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 @@ class UserMountCache implements IUserMountCache { $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 $newMounts + * @param array $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 @@ class UserMountCache implements IUserMountCache { * @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 $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 index dafd2423fdc..78a92874275 100644 --- a/lib/public/Files/Config/ICachedMountInfo.php +++ b/lib/public/Files/Config/ICachedMountInfo.php @@ -84,4 +84,12 @@ interface ICachedMountInfo { * @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 index ccad4671ae9..0a9355e9a46 100644 --- a/tests/lib/Files/Config/UserMountCacheTest.php +++ b/tests/lib/Files/Config/UserMountCacheTest.php @@ -118,6 +118,10 @@ class UserMountCacheTest extends TestCase { $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 @@ class UserMountCacheTest extends TestCase { $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 @@ class UserMountCacheTest extends TestCase { $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 @@ class UserMountCacheTest extends TestCase { $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 @@ class UserMountCacheTest extends TestCase { $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 @@ class UserMountCacheTest extends TestCase { $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 @@ class UserMountCacheTest extends TestCase { $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 @@ class UserMountCacheTest extends TestCase { $cachedMounts = $this->cache->getMountsForUser($user1); $this->assertCount(1, $cachedMounts); - $this->assertEquals('dummy', $cachedMounts[0]->getMountProvider()); + $this->assertEquals('dummy', $cachedMounts[$this->keyForMount($mount1)]->getMountProvider()); } } -- 2.39.5