From 3d49fe473a934c3fc9e17afadba3cbddd423603d Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Thu, 21 Apr 2022 16:31:19 +0200 Subject: [PATCH 1/3] Cache display name This should saves some query in the share backend when displaying the owner and it's not important if the display name is 10 minutes outdated as it is very rare that this gets changed. Signed-off-by: Carl Schwan --- apps/files_sharing/lib/Cache.php | 31 ++++------ apps/files_sharing/lib/SharedStorage.php | 4 +- apps/files_sharing/tests/TestCase.php | 2 + lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + lib/private/User/DisplayNameCache.php | 66 +++++++++++++++++++++ 6 files changed, 83 insertions(+), 22 deletions(-) create mode 100644 lib/private/User/DisplayNameCache.php diff --git a/apps/files_sharing/lib/Cache.php b/apps/files_sharing/lib/Cache.php index 8729426221b93..3482a9fc71a97 100644 --- a/apps/files_sharing/lib/Cache.php +++ b/apps/files_sharing/lib/Cache.php @@ -33,6 +33,7 @@ use OC\Files\Search\SearchBinaryOperator; use OC\Files\Search\SearchComparison; use OC\Files\Storage\Wrapper\Jail; +use OC\User\DisplayNameCache; use OCP\Files\Cache\ICacheEntry; use OCP\Files\Search\ISearchBinaryOperator; use OCP\Files\Search\ISearchComparison; @@ -46,27 +47,22 @@ * don't use this class directly if you need to get metadata, use \OC\Files\Filesystem::getFileInfo instead */ class Cache extends CacheJail { - /** @var \OCA\Files_Sharing\SharedStorage */ + /** @var SharedStorage */ private $storage; - /** @var ICacheEntry */ - private $sourceRootInfo; - /** @var IUserManager */ - private $userManager; - - private $rootUnchanged = true; - - private $ownerDisplayName; - - private $numericId; + private ICacheEntry $sourceRootInfo; + private bool $rootUnchanged = true; + private ?string $ownerDisplayName = null; + private int $numericId; + private DisplayNameCache $displayNameCache; /** - * @param \OCA\Files_Sharing\SharedStorage $storage + * @param SharedStorage $storage */ - public function __construct($storage, ICacheEntry $sourceRootInfo, IUserManager $userManager) { + public function __construct($storage, ICacheEntry $sourceRootInfo, DisplayNameCache $displayNameCache) { $this->storage = $storage; $this->sourceRootInfo = $sourceRootInfo; - $this->userManager = $userManager; $this->numericId = $sourceRootInfo->getStorageId(); + $this->displayNameCache = $displayNameCache; parent::__construct( null, @@ -173,12 +169,7 @@ protected function formatCacheEntry($entry, $path = null) { private function getOwnerDisplayName() { if (!$this->ownerDisplayName) { $uid = $this->storage->getOwner(''); - $user = $this->userManager->get($uid); - if ($user) { - $this->ownerDisplayName = $user->getDisplayName(); - } else { - $this->ownerDisplayName = $uid; - } + $this->ownerDisplayName = $this->displayNameCache->getDisplayName($uid); } return $this->ownerDisplayName; } diff --git a/apps/files_sharing/lib/SharedStorage.php b/apps/files_sharing/lib/SharedStorage.php index 8d123e7b873bb..37f1c12a2e441 100644 --- a/apps/files_sharing/lib/SharedStorage.php +++ b/apps/files_sharing/lib/SharedStorage.php @@ -38,6 +38,7 @@ use OC\Files\ObjectStore\HomeObjectStoreStorage; use OC\Files\Storage\Common; use OC\Files\Storage\Home; +use OC\User\DisplayNameCache; use OCP\Files\Folder; use OCP\Files\IHomeStorage; use OCP\Files\Node; @@ -51,7 +52,6 @@ use OCP\Files\NotFoundException; use OCP\Files\Storage\IDisableEncryptionStorage; use OCP\Files\Storage\IStorage; -use OCP\IUserManager; use OCP\Lock\ILockingProvider; use OCP\Share\IShare; @@ -416,7 +416,7 @@ public function getCache($path = '', $storage = null) { $this->cache = new \OCA\Files_Sharing\Cache( $storage, $sourceRoot, - \OC::$server->get(IUserManager::class) + \OC::$server->get(DisplayNameCache::class) ); return $this->cache; } diff --git a/apps/files_sharing/tests/TestCase.php b/apps/files_sharing/tests/TestCase.php index bb1e3125ab2c9..234ea2c69c897 100644 --- a/apps/files_sharing/tests/TestCase.php +++ b/apps/files_sharing/tests/TestCase.php @@ -39,6 +39,7 @@ use OCP\Files\Config\IMountProviderCollection; use OCP\Share\IShare; use Test\Traits\MountProviderTrait; +use OC\User\DisplayNameCache; /** * Class TestCase @@ -116,6 +117,7 @@ public static function setUpBeforeClass(): void { protected function setUp(): void { parent::setUp(); + \OC::$server->get(DisplayNameCache::class)->clear(); //login as user1 self::loginHelper(self::TEST_FILES_SHARING_API_USER1); diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php index 685fae9bef11c..831f5229d4a64 100644 --- a/lib/composer/composer/autoload_classmap.php +++ b/lib/composer/composer/autoload_classmap.php @@ -1540,6 +1540,7 @@ 'OC\\UserStatus\\Manager' => $baseDir . '/lib/private/UserStatus/Manager.php', 'OC\\User\\Backend' => $baseDir . '/lib/private/User/Backend.php', 'OC\\User\\Database' => $baseDir . '/lib/private/User/Database.php', + 'OC\\User\\DisplayNameCache' => $baseDir . '/lib/private/User/DisplayNameCache.php', 'OC\\User\\LoginException' => $baseDir . '/lib/private/User/LoginException.php', 'OC\\User\\Manager' => $baseDir . '/lib/private/User/Manager.php', 'OC\\User\\NoUserException' => $baseDir . '/lib/private/User/NoUserException.php', diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php index 7e778d73b8323..9471934401322 100644 --- a/lib/composer/composer/autoload_static.php +++ b/lib/composer/composer/autoload_static.php @@ -1569,6 +1569,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c 'OC\\UserStatus\\Manager' => __DIR__ . '/../../..' . '/lib/private/UserStatus/Manager.php', 'OC\\User\\Backend' => __DIR__ . '/../../..' . '/lib/private/User/Backend.php', 'OC\\User\\Database' => __DIR__ . '/../../..' . '/lib/private/User/Database.php', + 'OC\\User\\DisplayNameCache' => __DIR__ . '/../../..' . '/lib/private/User/DisplayNameCache.php', 'OC\\User\\LoginException' => __DIR__ . '/../../..' . '/lib/private/User/LoginException.php', 'OC\\User\\Manager' => __DIR__ . '/../../..' . '/lib/private/User/Manager.php', 'OC\\User\\NoUserException' => __DIR__ . '/../../..' . '/lib/private/User/NoUserException.php', diff --git a/lib/private/User/DisplayNameCache.php b/lib/private/User/DisplayNameCache.php new file mode 100644 index 0000000000000..f44cdac6e8556 --- /dev/null +++ b/lib/private/User/DisplayNameCache.php @@ -0,0 +1,66 @@ + + * @license AGPL-3.0-or-later + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + + +namespace OC\User; + +use OCP\ICache; +use OCP\ICacheFactory; +use OCP\IUserManager; + +/** + * Class that cache the relation UserId -> Display name + * + * This saves fetching the user from a user backend and later on fetching + * their preferences. It's generally not an issue if this data is slightly + * outdated. + */ +class DisplayNameCache { + private ICache $internalCache; + private IUserManager $userManager; + + public function __construct(ICacheFactory $cacheFactory, IUserManager $userManager) { + $this->internalCache = $cacheFactory->createDistributed('displayNameMappingCache'); + $this->userManager = $userManager; + } + + public function getDisplayName(string $userId) { + $displayName = $this->internalCache->get($userId); + if ($displayName) { + return $displayName; + } + + $user = $this->userManager->get($userId); + if ($user) { + $displayName = $user->getDisplayName(); + } else { + $displayName = $userId; + } + $this->internalCache->set($userId, $displayName, 60 * 10); // 10 minutes + + return $displayName; + } + + public function clear(): void { + $this->internalCache->clear(); + } +} From 40ac4e81976f2acceafce2d8c5b7e4aec2dba044 Mon Sep 17 00:00:00 2001 From: Carl Schwan Date: Fri, 22 Apr 2022 10:01:35 +0200 Subject: [PATCH 2/3] Update cache when display name change This improve the correctness of the data Signed-off-by: Carl Schwan --- apps/files_sharing/lib/AppInfo/Application.php | 3 +++ apps/files_sharing/lib/Cache.php | 2 +- lib/private/User/DisplayNameCache.php | 13 ++++++++++++- 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/apps/files_sharing/lib/AppInfo/Application.php b/apps/files_sharing/lib/AppInfo/Application.php index befd1532d0260..2539247b561bc 100644 --- a/apps/files_sharing/lib/AppInfo/Application.php +++ b/apps/files_sharing/lib/AppInfo/Application.php @@ -30,6 +30,7 @@ namespace OCA\Files_Sharing\AppInfo; use OC\Share\Share; +use OC\User\DisplayNameCache; use OCA\Files_Sharing\Capabilities; use OCA\Files_Sharing\Event\BeforeTemplateRenderedEvent; use OCA\Files_Sharing\External\Manager; @@ -65,6 +66,7 @@ use OCP\L10N\IFactory; use OCP\Share\Events\ShareCreatedEvent; use OCP\Share\IManager; +use OCP\User\Events\UserChangedEvent; use OCP\Util; use Psr\Container\ContainerInterface; use Symfony\Component\EventDispatcher\EventDispatcherInterface; @@ -98,6 +100,7 @@ function () use ($c) { $context->registerCapability(Capabilities::class); $context->registerNotifierService(Notifier::class); + $context->registerEventListener(UserChangedEvent::class, DisplayNameCache::class); } public function boot(IBootContext $context): void { diff --git a/apps/files_sharing/lib/Cache.php b/apps/files_sharing/lib/Cache.php index 3482a9fc71a97..d245727b138eb 100644 --- a/apps/files_sharing/lib/Cache.php +++ b/apps/files_sharing/lib/Cache.php @@ -52,7 +52,7 @@ class Cache extends CacheJail { private ICacheEntry $sourceRootInfo; private bool $rootUnchanged = true; private ?string $ownerDisplayName = null; - private int $numericId; + private $numericId; private DisplayNameCache $displayNameCache; /** diff --git a/lib/private/User/DisplayNameCache.php b/lib/private/User/DisplayNameCache.php index f44cdac6e8556..ed0c723ef37d2 100644 --- a/lib/private/User/DisplayNameCache.php +++ b/lib/private/User/DisplayNameCache.php @@ -23,9 +23,12 @@ namespace OC\User; +use OCP\EventDispatcher\Event; +use OCP\EventDispatcher\IEventListener; use OCP\ICache; use OCP\ICacheFactory; use OCP\IUserManager; +use OCP\User\Events\UserChangedEvent; /** * Class that cache the relation UserId -> Display name @@ -34,7 +37,7 @@ * their preferences. It's generally not an issue if this data is slightly * outdated. */ -class DisplayNameCache { +class DisplayNameCache implements IEventListener { private ICache $internalCache; private IUserManager $userManager; @@ -63,4 +66,12 @@ public function getDisplayName(string $userId) { public function clear(): void { $this->internalCache->clear(); } + + public function handle(Event $event): void { + if ($event instanceof UserChangedEvent && $event->getFeature() === 'displayName') { + $userId = $event->getUser()->getUID(); + $newDisplayName = $event->getValue(); + $this->internalCache->set($userId, $newDisplayName, 60 * 10); // 10 minutes + } + } } From 6ca689aff8d55685a56dd176e8dc19f7324aabfe Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Fri, 22 Apr 2022 12:50:42 +0200 Subject: [PATCH 3/3] cache display names in local memory before external memcache Signed-off-by: Robin Appelman --- lib/private/User/DisplayNameCache.php | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/lib/private/User/DisplayNameCache.php b/lib/private/User/DisplayNameCache.php index ed0c723ef37d2..22a79863e4978 100644 --- a/lib/private/User/DisplayNameCache.php +++ b/lib/private/User/DisplayNameCache.php @@ -38,17 +38,22 @@ * outdated. */ class DisplayNameCache implements IEventListener { - private ICache $internalCache; + private array $cache = []; + private ICache $memCache; private IUserManager $userManager; public function __construct(ICacheFactory $cacheFactory, IUserManager $userManager) { - $this->internalCache = $cacheFactory->createDistributed('displayNameMappingCache'); + $this->memCache = $cacheFactory->createDistributed('displayNameMappingCache'); $this->userManager = $userManager; } public function getDisplayName(string $userId) { - $displayName = $this->internalCache->get($userId); + if (isset($this->cache[$userId])) { + return $this->cache[$userId]; + } + $displayName = $this->memCache->get($userId); if ($displayName) { + $this->cache[$userId] = $displayName; return $displayName; } @@ -58,20 +63,23 @@ public function getDisplayName(string $userId) { } else { $displayName = $userId; } - $this->internalCache->set($userId, $displayName, 60 * 10); // 10 minutes + $this->cache[$userId] = $displayName; + $this->memCache->set($userId, $displayName, 60 * 10); // 10 minutes return $displayName; } public function clear(): void { - $this->internalCache->clear(); + $this->cache = []; + $this->memCache->clear(); } public function handle(Event $event): void { if ($event instanceof UserChangedEvent && $event->getFeature() === 'displayName') { $userId = $event->getUser()->getUID(); $newDisplayName = $event->getValue(); - $this->internalCache->set($userId, $newDisplayName, 60 * 10); // 10 minutes + $this->cache[$userId] = $newDisplayName; + $this->memCache->set($userId, $newDisplayName, 60 * 10); // 10 minutes } } }