perf: cache path by id to speedup getting nodes by id

Signed-off-by: Robin Appelman <[email protected]>
nextcloud · Feb 29, 2024 · 3dfef53 · 3dfef53
1 parent e995903
commit 3dfef53
Show file tree

Hide file tree

Showing 8 changed files with 124 additions and 48 deletions.
diff --git a/lib/private/Files/Node/Root.php b/lib/private/Files/Node/Root.php
@@ -49,6 +49,8 @@
 use OCP\Files\Node as INode;
 use OCP\Files\NotFoundException;
 use OCP\Files\NotPermittedException;
+use OCP\ICache;
+use OCP\ICacheFactory;
 use OCP\IUser;
 use OCP\IUserManager;
 use Psr\Log\LoggerInterface;
@@ -81,6 +83,7 @@ class Root extends Folder implements IRootFolder {
 	private LoggerInterface $logger;
 	private IUserManager $userManager;
 	private IEventDispatcher $eventDispatcher;
+	private ICache $pathByIdCache;
 
 	/**
 	 * @param Manager $manager
@@ -94,7 +97,8 @@ public function __construct(
 		IUserMountCache $userMountCache,
 		LoggerInterface $logger,
 		IUserManager $userManager,
-		IEventDispatcher $eventDispatcher
+		IEventDispatcher $eventDispatcher,
+		ICacheFactory $cacheFactory,
 	) {
 		parent::__construct($this, $view, '');
 		$this->mountManager = $manager;
@@ -107,6 +111,7 @@ public function __construct(
 		$eventDispatcher->addListener(FilesystemTornDownEvent::class, function () {
 			$this->userFolderCache = new CappedMemoryCache();
 		});
+		$this->pathByIdCache = $cacheFactory->createLocal('path-by-id');
 	}
 
 	/**
@@ -406,7 +411,28 @@ public function getUserMountCache() {
 	}
 
 	public function getFirstNodeByIdInPath(int $id, string $path): ?INode {
-		return current($this->getByIdInPath($id, $path));
+		// scope the cache by user, so we don't return nodes for different users
+		if ($this->user) {
+			$cachedPath = $this->pathByIdCache->get($this->user->getUID() . '::' . $id);
+			if ($cachedPath && str_starts_with($path, $cachedPath)) {
+				// getting the node by path is significantly cheaper than finding it by id
+				$node = $this->get($cachedPath);
+				// by validating that the cached path still has the requested fileid we can work around the need to invalidate the cached path
+				// if the cached path is invalid or a different file now we fall back to the uncached logic
+				if ($node && $node->getId() === $id) {
+					return $node;
+				}
+			}
+		}
+		$node = current($this->getByIdInPath($id, $path));
+		if (!$node) {
+			return null;
+		}
+
+		if ($this->user) {
+			$this->pathByIdCache->set($this->user->getUID() . '::' . $id, $node->getPath());
+		}
+		return $node;
 	}
 
 	/**

diff --git a/lib/private/Server.php b/lib/private/Server.php
@@ -448,14 +448,17 @@ public function __construct($webRoot, \OC\Config $config) {
 		$this->registerService('RootFolder', function (ContainerInterface $c) {
 			$manager = \OC\Files\Filesystem::getMountManager();
 			$view = new View();
+			/** @var IUserSession $userSession */
+			$userSession = $c->get(IUserSession::class);
 			$root = new Root(
 				$manager,
 				$view,
-				null,
+				$userSession->getUser(),
 				$c->get(IUserMountCache::class),
 				$this->get(LoggerInterface::class),
 				$this->get(IUserManager::class),
 				$this->get(IEventDispatcher::class),
+				$this->get(ICacheFactory::class),
 			);
 
 			$previewConnector = new \OC\Preview\WatcherConnector(

diff --git a/tests/lib/Files/Node/FileTest.php b/tests/lib/Files/Node/FileTest.php
@@ -39,7 +39,7 @@ protected function getViewDeleteMethod() {
 	public function testGetContent() {
 		/** @var \OC\Files\Node\Root|\PHPUnit\Framework\MockObject\MockObject $root */
 		$root = $this->getMockBuilder('\OC\Files\Node\Root')
-			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher])
+			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher, $this->cacheFactory])
 			->getMock();
 
 		$hook = function ($file) {
@@ -69,7 +69,7 @@ public function testGetContentNotPermitted() {
 
 		/** @var \OC\Files\Node\Root|\PHPUnit\Framework\MockObject\MockObject $root */
 		$root = $this->getMockBuilder('\OC\Files\Node\Root')
-			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher])
+			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher, $this->cacheFactory])
 			->getMock();
 
 		$root->expects($this->any())
@@ -88,7 +88,7 @@ public function testGetContentNotPermitted() {
 	public function testPutContent() {
 		/** @var \OC\Files\Node\Root|\PHPUnit\Framework\MockObject\MockObject $root */
 		$root = $this->getMockBuilder('\OC\Files\Node\Root')
-			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher])
+			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher, $this->cacheFactory])
 			->getMock();
 
 		$root->expects($this->any())
@@ -115,7 +115,7 @@ public function testPutContentNotPermitted() {
 
 		/** @var \OC\Files\Node\Root|\PHPUnit\Framework\MockObject\MockObject $root */
 		$root = $this->getMockBuilder('\OC\Files\Node\Root')
-			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher])
+			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher, $this->cacheFactory])
 			->getMock();
 
 		$this->view->expects($this->once())
@@ -130,7 +130,7 @@ public function testPutContentNotPermitted() {
 	public function testGetMimeType() {
 		/** @var \OC\Files\Node\Root|\PHPUnit\Framework\MockObject\MockObject $root */
 		$root = $this->getMockBuilder('\OC\Files\Node\Root')
-			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher])
+			->setConstructorArgs([$this->manager, $this->view, $this->user, $this->userMountCache, $this->logger, $this->userManager, $this->eventDispatcher, $this->cacheFactory])
 			->getMock();
 
 		$this->view->expects($this->once())
@@ -154,7 +154,8 @@ public function testFOpenRead() {
 			$this->userMountCache,
 			$this->logger,
 			$this->userManager,
-			$this->eventDispatcher
+			$this->eventDispatcher,
+			$this->cacheFactory,
 		);
 
 		$hook = function ($file) {
@@ -190,7 +191,8 @@ public function testFOpenWrite() {
 			$this->userMountCache,
 			$this->logger,
 			$this->userManager,
-			$this->eventDispatcher
+			$this->eventDispatcher,
+			$this->cacheFactory,
 		);
 		$hooksCalled = 0;
 		$hook = function ($file) use (&$hooksCalled) {
@@ -230,7 +232,8 @@ public function testFOpenReadNotPermitted() {
 			$this->userMountCache,
 			$this->logger,
 			$this->userManager,
-			$this->eventDispatcher
+			$this->eventDispatcher,
+			$this->cacheFactory,
 		);
 		$hook = function ($file) {
 			throw new \Exception('Hooks are not supposed to be called');
@@ -256,7 +259,8 @@ public function testFOpenReadWriteNoReadPermissions() {
 			$this->userMountCache,
 			$this->logger,
 			$this->userManager,
-			$this->eventDispatcher
+			$this->eventDispatcher,
+			$this->cacheFactory,
 		);
 		$hook = function () {
 			throw new \Exception('Hooks are not supposed to be called');
@@ -282,7 +286,8 @@ public function testFOpenReadWriteNoWritePermissions() {
 			$this->userMountCache,
 			$this->logger,
 			$this->userManager,
-			$this->eventDispatcher
+			$this->eventDispatcher,
+			$this->cacheFactory,
 		);
 		$hook = function () {
 			throw new \Exception('Hooks are not supposed to be called');