From b8eba88598601900d1bb55dcf7dc9314865eb777 Mon Sep 17 00:00:00 2001 From: Maximilian Ruta Date: Sun, 19 Apr 2020 23:22:58 +0200 Subject: [PATCH 1/3] Increase performance by adding cache for ACLs --- lib/ACL/ACLManager.php | 40 ++---- lib/ACL/ACLManagerFactory.php | 6 +- lib/ACL/ACLRuleCache.php | 248 ++++++++++++++++++++++++++++++++++ lib/ACL/RuleManager.php | 28 +++- lib/AppInfo/Application.php | 8 ++ 5 files changed, 299 insertions(+), 31 deletions(-) create mode 100644 lib/ACL/ACLRuleCache.php diff --git a/lib/ACL/ACLManager.php b/lib/ACL/ACLManager.php index f145e89d2..d6b4503c8 100644 --- a/lib/ACL/ACLManager.php +++ b/lib/ACL/ACLManager.php @@ -35,9 +35,9 @@ class ACLManager { private $rootStorageId = null; private $rootFolderProvider; - public function __construct(RuleManager $ruleManager, IUser $user, callable $rootFolderProvider) { + public function __construct(RuleManager $ruleManager, ACLRuleCache $ruleCache, IUser $user, callable $rootFolderProvider) { $this->ruleManager = $ruleManager; - $this->ruleCache = new CappedMemoryCache(); + $this->ruleCache = $ruleCache; $this->user = $user; $this->rootFolderProvider = $rootFolderProvider; } @@ -53,40 +53,24 @@ private function getRootStorageId() { return $this->rootStorageId; } - private function pathsAreCached(array $paths): bool { - foreach ($paths as $path) { - if (!$this->ruleCache->hasKey($path)) { - return false; - } - } - return true; - } - /** * @param int $folderId * @param array $paths * @return (Rule[])[] */ private function getRules(array $paths): array { - if ($this->pathsAreCached($paths)) { - $rules = array_combine($paths, array_map(function (string $path) { - return $this->ruleCache->get($path); - }, $paths)); - } else { - $rules = $this->ruleManager->getRulesForFilesByPath($this->user, $this->getRootStorageId(), $paths); - foreach ($rules as $path => $rulesForPath) { - $this->ruleCache->set($path, $rulesForPath); - } + $rules = $this->ruleCache->getByPaths($this->user, $this->getRootStorageId(), $paths); - if (count($paths) > 2) { - // also cache the direct sibling since it's likely that we'll be needing those later - $directParent = $paths[1]; - $siblingRules = $this->ruleManager->getRulesForFilesByParent($this->user, $this->getRootStorageId(), $directParent); - foreach ($siblingRules as $path => $rulesForPath) { - $this->ruleCache->set($path, $rulesForPath); - } - } + $uncachedPaths = array_values(array_diff($paths, array_keys($rules))); + + if (count($uncachedPaths) > 0) { + $this->ruleCache->cachePath( + $this->getRootStorageId(), + $this->ruleManager->getAllRulesForFilesByPath($this->getRootStorageId(), $uncachedPaths) + ); + $rules = $this->ruleCache->getByPaths($this->user, $this->getRootStorageId(), $paths); } + ksort($rules); return $rules; diff --git a/lib/ACL/ACLManagerFactory.php b/lib/ACL/ACLManagerFactory.php index cade98a22..ba03e217f 100644 --- a/lib/ACL/ACLManagerFactory.php +++ b/lib/ACL/ACLManagerFactory.php @@ -27,13 +27,15 @@ class ACLManagerFactory { private $ruleManager; private $rootFolderProvider; + private $ruleCache; - public function __construct(RuleManager $ruleManager, callable $rootFolderProvider) { + public function __construct(RuleManager $ruleManager, ACLRuleCache $ruleCache, callable $rootFolderProvider) { $this->ruleManager = $ruleManager; + $this->ruleCache = $ruleCache; $this->rootFolderProvider = $rootFolderProvider; } public function getACLManager(IUser $user): ACLManager { - return new ACLManager($this->ruleManager, $user, $this->rootFolderProvider); + return new ACLManager($this->ruleManager, $this->ruleCache, $user, $this->rootFolderProvider); } } diff --git a/lib/ACL/ACLRuleCache.php b/lib/ACL/ACLRuleCache.php new file mode 100644 index 000000000..e25a9cbf0 --- /dev/null +++ b/lib/ACL/ACLRuleCache.php @@ -0,0 +1,248 @@ +userMappingManager = $userMappingManager; + $this->ruleCache = new APCu('acl'); + } + + protected function set($key, $value, $ttl = null) : void + { + if ($ttl === null) { + $ttl = self::DEFAULT_TTL; + } + + $ttl = rand($ttl, round($ttl * self::DERIVATION)); + + $this->ruleCache->set($key, $value, $ttl); + } + + protected function has($key) : bool + { + return $this->ruleCache->hasKey($key); + } + + protected function get($key) + { + return $this->ruleCache->get($key); + } + + protected function remove($key) : void + { + $this->ruleCache->remove($key); + } + + protected function keyPath(int $storageId, string $path) : string + { + return 'path_' . $storageId . '_' . $path; + } + + protected function keyFileId(int $fileId) : string + { + return 'file_' . $fileId; + } + + protected function keyPathMapping(int $storageId, string $path, IUserMapping $mapping) : string + { + return $this->keyPath($storageId, $path) . '_' . $mapping->getType() . '_' . $mapping->getId(); + } + + public function clearByPathKey(string $pathKey) : void + { + $keys = $this->get($pathKey); + if (!isset($keys['rule_path_keys'])) { + $keys['rule_path_keys'] = []; + } + if (!isset($keys['rule_id_keys'])) { + $keys['rule_id_keys'] = []; + } + + foreach (array_merge($keys['rule_path_keys'], $keys['rule_id_keys']) as $key) { + $this->remove($key); + } + } + + public function clearByPath(int $storageId, string $path) : void + { + $pathKey = $this->keyPath($storageId, $path); + if (!$this->has($pathKey)) { + return; + } + + $this->clearByPathKey($pathKey); + } + + public function clearByFileId(int $fileId) : void + { + $fileIdKey = $this->keyFileId($fileId); + if (!$this->has($fileIdKey)) { + return; + } + + $this->clearByPathKey($this->get($fileIdKey)); + } + + /** + * @return Rule[]|null + */ + public function getByPath(IUser $user, int $storageId, string $path) : ?array + { + return $this->getByPathKey($user, $this->keyPath($storageId, $path)); + } + + /** + * @param string[] $paths + * @return (Rule[]|null)[] + */ + public function getByPaths(IUser $user, int $storageId, array $paths) : array + { + $rulePaths = []; + + foreach ($paths as $path) { + $cached = $this->getByPath($user, $storageId, $path); + if ($cached !== null) { + $rulePaths[$path] = $cached; + } + } + + return $rulePaths; + } + + /** + * @return Rule[]|null + */ + public function getByFileId(IUser $user, int $fileId) : ?array + { + $fileIdKey = $this->keyFileId($fileId); + if (!$this->has($fileIdKey)) { + return null; + } + + $cached = $this->getByPathKey($user, $this->get($fileIdKey)); + if ($cached === null) { + return null; + } + + $rules = []; + + foreach ($cached as $rule) { + if ($rule->getFileId() == $fileIdKey) { + $rules[] = $rule; + } + } + + return $rules; + } + + /** + * @param (Rule[])[] $paths + */ + + public function cachePath(int $storageId, array $paths) : void + { + foreach ($paths as $path => $rules) { + $this->cacheRules($storageId, $path, $rules); + } + } + + /** + * @param Rule[] $rules + */ + public function cacheRules(int $storageId, string $path, array $rules) : void + { + $rulePathKeys = []; + $fileIdKeys = []; + + foreach ($rules as $rule) { + $rulePathKeys[] = $rulePathKey = $this->keyPathMapping($storageId, $path, $rule->getUserMapping()); + $fileIdKeys[] = $ruleFileIdKey = $this->keyFileId($rule->getFileId()); + $this->set($rulePathKey, $rule); + } + + + $pathKey = $this->keyPath($storageId, $path); + $this->set($pathKey, [ + 'rule_path_keys' => $rulePathKeys, + 'rule_id_keys' => $fileIdKeys + ], self::INDEX_TTL); + foreach ($fileIdKeys as $fileIdKey) { + $this->set($fileIdKey, $pathKey, self::INDEX_TTL); + } + } + + /** + * @param IUserMapping[] $userMappings + * @param Rule $rule + * @return bool + */ + protected function checkMappingInRule(array $userMappings, Rule $rule) : bool + { + foreach ($userMappings as $userMapping) { + if ($userMapping->getId() == $rule->getUserMapping()->getId() && + $userMapping->getType() == $rule->getUserMapping()->getType()) { + return true; + } + } + + return false; + } + + /** + * @param IUser $user + * @param string $pathKey + * @return Rule[]|null + */ + protected function getByPathKey(IUser $user, string $pathKey) : ?array + { + if (!$this->has($pathKey)) { + return null; + } + + $keys = $this->get($pathKey); + if (!isset($keys['rule_path_keys'])) { + return null; + } + + $userMappings = $this->userMappingManager->getMappingsForUser($user); + + $rules = []; + + foreach ($keys['rule_path_keys'] as $key) { + if (!$this->has($key)) { + return null; + } + + $rule = $this->get($key); + + if ($this->checkMappingInRule($userMappings, $rule)) { + $rules[] = $rule; + } + } + + return $rules; + } +} diff --git a/lib/ACL/RuleManager.php b/lib/ACL/RuleManager.php index 0cea17f7d..611881afd 100644 --- a/lib/ACL/RuleManager.php +++ b/lib/ACL/RuleManager.php @@ -29,10 +29,12 @@ class RuleManager { private $connection; + private $ruleCache; private $userMappingManager; - public function __construct(IDBConnection $connection, IUserMappingManager $userMappingManager) { + public function __construct(IDBConnection $connection, ACLRuleCache $ruleCache, IUserMappingManager $userMappingManager) { $this->connection = $connection; + $this->ruleCache = $ruleCache; $this->userMappingManager = $userMappingManager; } @@ -261,6 +263,28 @@ public function getRulesForPrefix(IUser $user, int $storageId, string $prefix): return $this->rulesByPath($rows); } + /** + * @param int $storageId + * @param string[] $filePaths + * @return (Rule[])[] [$path => Rule[]] + */ + public function getAllRulesForFilesByPath(int $storageId, array $filePaths): array { + $query = $this->connection->getQueryBuilder(); + $query->select(['f.fileid', 'mapping_type', 'mapping_id', 'mask', 'a.permissions', 'path']) + ->from('group_folders_acl', 'a') + ->innerJoin('a', 'filecache', 'f', $query->expr()->eq('f.fileid', 'a.fileid')) + ->where($query->expr()->in('path_hash', $query->createNamedParameter(array_map('md5', $filePaths), IQueryBuilder::PARAM_STR_ARRAY))) + ->andWhere($query->expr()->eq('storage', $query->createNamedParameter($storageId, IQueryBuilder::PARAM_INT))); + + $rows = $query->execute()->fetchAll(); + + $result = []; + foreach ($filePaths as $path) { + $result[$path] = []; + } + return $this->rulesByPath($rows, $result); + } + private function hasRule(IUserMapping $mapping, int $fileId): bool { $query = $this->connection->getQueryBuilder(); $query->select('fileid') @@ -293,6 +317,7 @@ public function saveRule(Rule $rule) { ]); $query->execute(); } + $this->ruleCache->clearByFileId($rule->getFileId()); } public function deleteRule(Rule $rule) { @@ -302,5 +327,6 @@ public function deleteRule(Rule $rule) { ->andWhere($query->expr()->eq('mapping_type', $query->createNamedParameter($rule->getUserMapping()->getType()))) ->andWhere($query->expr()->eq('mapping_id', $query->createNamedParameter($rule->getUserMapping()->getId()))); $query->execute(); + $this->ruleCache->clearByFileId($rule->getFileId()); } } diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 218870b41..111fcf149 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -23,6 +23,7 @@ use OC\Group\Manager; use OCA\GroupFolders\ACL\ACLManagerFactory; +use OCA\GroupFolders\ACL\ACLRuleCache; use OCA\GroupFolders\ACL\RuleManager; use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager; use OCA\GroupFolders\ACL\UserMapping\UserMappingManager; @@ -114,12 +115,19 @@ public function __construct(array $urlParams = []) { } }); + $container->registerService(ACLRuleCache::class, function(IAppContainer $c) { + return new ACLRuleCache( + $c->query(IUserMappingManager::class) + ); + }); + $container->registerService(ACLManagerFactory::class, function(IAppContainer $c) { $rootFolderProvider = function () use ($c) { return $c->getServer()->getRootFolder(); }; return new ACLManagerFactory( $c->query(RuleManager::class), + $c->query(ACLRuleCache::class), $rootFolderProvider ); }); From eeea618dbad913db302b7913c3255d0312279938 Mon Sep 17 00:00:00 2001 From: Maximilian Ruta Date: Mon, 20 Apr 2020 00:02:22 +0200 Subject: [PATCH 2/3] Increase performance by selecting on indexed column --- lib/ACL/ACLRuleCache.php | 9 +++++---- lib/AppInfo/Application.php | 2 ++ 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/ACL/ACLRuleCache.php b/lib/ACL/ACLRuleCache.php index e25a9cbf0..13b583ae2 100644 --- a/lib/ACL/ACLRuleCache.php +++ b/lib/ACL/ACLRuleCache.php @@ -2,9 +2,10 @@ namespace OCA\GroupFolders\ACL; -use OC\Memcache\APCu; use OCA\GroupFolders\ACL\UserMapping\IUserMapping; use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IUser; class ACLRuleCache @@ -16,7 +17,7 @@ class ACLRuleCache const INDEX_TTL = (3600 * 24) + 3600; /** - * @var APCu + * @var ICache */ private $ruleCache; @@ -25,9 +26,9 @@ class ACLRuleCache */ protected $userMappingManager; - public function __construct(IUserMappingManager $userMappingManager) { + public function __construct(ICacheFactory $cacheFactory, IUserMappingManager $userMappingManager) { $this->userMappingManager = $userMappingManager; - $this->ruleCache = new APCu('acl'); + $this->ruleCache = $cacheFactory->createDistributed('acl'); } protected function set($key, $value, $ttl = null) : void diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 111fcf149..73b33810f 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -41,6 +41,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventDispatcher; use OCP\Files\NotFoundException; +use OCP\ICacheFactory; use OCP\IGroup; use OCP\IGroupManager; use OCP\IRequest; @@ -117,6 +118,7 @@ public function __construct(array $urlParams = []) { $container->registerService(ACLRuleCache::class, function(IAppContainer $c) { return new ACLRuleCache( + $c->query(ICacheFactory::class), $c->query(IUserMappingManager::class) ); }); From b41ec13ca4e6775a2a473ab9c8f8f20cb46336c3 Mon Sep 17 00:00:00 2001 From: Maximilian Ruta Date: Mon, 20 Apr 2020 00:16:07 +0200 Subject: [PATCH 3/3] Increase performance by selecting on indexed column --- lib/ACL/ACLRuleCache.php | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/lib/ACL/ACLRuleCache.php b/lib/ACL/ACLRuleCache.php index 13b583ae2..74d025c36 100644 --- a/lib/ACL/ACLRuleCache.php +++ b/lib/ACL/ACLRuleCache.php @@ -4,6 +4,7 @@ use OCA\GroupFolders\ACL\UserMapping\IUserMapping; use OCA\GroupFolders\ACL\UserMapping\IUserMappingManager; +use OCA\GroupFolders\ACL\UserMapping\UserMapping; use OCP\ICache; use OCP\ICacheFactory; use OCP\IUser; @@ -181,7 +182,9 @@ public function cacheRules(int $storageId, string $path, array $rules) : void foreach ($rules as $rule) { $rulePathKeys[] = $rulePathKey = $this->keyPathMapping($storageId, $path, $rule->getUserMapping()); $fileIdKeys[] = $ruleFileIdKey = $this->keyFileId($rule->getFileId()); - $this->set($rulePathKey, $rule); + $this->set($rulePathKey, array_merge($rule->jsonSerialize(), [ + 'file_id' => $rule->getFileId() + ])); } @@ -237,7 +240,14 @@ protected function getByPathKey(IUser $user, string $pathKey) : ?array return null; } - $rule = $this->get($key); + $ruleData = $this->get($key); + + $rule = new Rule( + new UserMapping($ruleData['mapping']['type'], $ruleData['mapping']['id']), + $ruleData['file_id'], + $ruleData['mask'], + $ruleData['permissions'] + ); if ($this->checkMappingInRule($userMappings, $rule)) { $rules[] = $rule;