introduce a static fileid->(storage, path) cache #39847

butonic · 2022-03-02T21:28:43Z

This PR significantly reduces the number of queries for Cache::pathById() lookups. This is less intrusive than a full cache implementation but it can at least cache and invalidate the id -> path lookup per request.

Before:

{
  "type": "SUMMARY",
  "reqId": "Yh-dLiX1gfglyR5qoetJZwAAAIw",
  "time": "2022-03-02T21:10:07+00:00",
  "remoteAddr": "172.20.0.11",
  "user": "jfd",
  "method": "GET",
  "url": "/ocs/v1.php/apps/files_sharing/api/v1/shares?format=json&include_tags=true",
  "diagnostics": {
    "totalSQLQueries": 2872,
    "totalSQLDurationmsec": 357.25855827331543,
    "totalSQLParams": 3430,
    "totalEvents": 32,
    "totalEventsDurationmsec": 445.83773612976074
  }
}

After:

{
  "type": "SUMMARY",
  "reqId": "Yh-dsiX1gfglyR5qoetJfQAAAIY",
  "time": "2022-03-02T21:12:19+00:00",
  "remoteAddr": "172.20.0.11",
  "user": "jfd",
  "method": "GET",
  "url": "/ocs/v1.php/apps/files_sharing/api/v1/shares?format=json&include_tags=true",
  "diagnostics": {
    "totalSQLQueries": 637,
    "totalSQLDurationmsec": 135.25128364562988,
    "totalSQLParams": 1195,
    "totalEvents": 32,
    "totalEventsDurationmsec": 207.22270011901855
  }
}

update-docs · 2022-03-02T21:28:46Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

ownclouders · 2022-03-02T21:59:33Z

💥 Acceptance tests pipeline cliMain-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/34803/116/1

ownclouders · 2022-03-02T21:59:34Z

💥 Acceptance tests pipeline cliProvisioning-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/34803/117/1

micbar · 2022-03-03T08:15:10Z

I think this has some value. Thanks @butonic

I would appreciate your POV @jvillafanez @mrow4a @DeepDiver1975

mrow4a · 2022-03-03T10:14:43Z

Problem with cache of filecache in OC is always a question: is this class always used for interfacing with filecache or in some places we do direct calls omitting this class. If yes than we should be safe from any desync.

butonic · 2022-03-03T11:19:24Z

@mrow4a true, which is why this pr is limited to the getPathByID lookup. We are neither caching etags or full filecache rows nor are we caching them across requests. The path lookup is only cached per request and needs to be invalidated when a delete or rename operation happens.

For a share mount/unmount the result actually does not need to be invalidated because the path is relative to the storage root, which remains the same.

mrow4a · 2022-03-03T11:24:00Z

ah true, then yes this would work.

butonic · 2022-03-03T11:37:18Z

https://github.com/owncloud/core/pull/28166/files has explored the places that are bypassing the Cache class.

I don't see places that affect this cache:

Jobs an Repair steps don't affect this static cache
when storages are deleted, the code can no longer instantiate a storage cache with the corret numeric storage id, so the static cache does not need to be cleared
Test, while green, don't affect runtime

jvillafanez · 2022-03-03T13:06:42Z

lib/private/Files/Cache/Cache.php

+	 * and a single trip to the db is sufficient to answer subsequent calls.
+	 *  @var CE[] $path_cache
+	 */
+	private static $path_cache = [];


Better use a CappedMemoryCache (https://github.com/owncloud/core/blob/master/lib/private/Cache/CappedMemoryCache.php), so we have limited entries and don't overload the memory.

jvillafanez · 2022-03-03T13:08:34Z

lib/private/Files/Cache/Cache.php

@@ -979,3 +1002,14 @@ public function normalize($path) {
 		return \trim(\OC_Util::normalizeUnicode($path), '/');
 	}
 }
+
+class CE {


I'd use just an array and document the keys if needed.

jvillafanez · 2022-03-03T13:10:29Z

lib/private/Files/Cache/Cache.php

@@ -324,6 +324,7 @@ public function insert($file, array $data) {
 	 * @param array $data [$key => $value] the metadata to update, only the fields provided in the array will be updated, non-provided values will remain unchanged
 	 */
 	public function update($id, array $data) {
+		unset(self::$path_cache[(int)$id]);


should we try to update the cache instead of removing the element?

over optimisation in my opinion :>

jvillafanez · 2022-03-03T13:17:32Z

lib/private/Files/Cache/Cache.php

@@ -507,6 +508,7 @@ public function inCache($file) {
 	public function remove($file) {
 		$entry = $this->get($file);
 		if ($entry !== false) {
+			unset(self::$path_cache[(int)$entry['fileid']]);


I think we should remove the cache after the operation. In case the operation fails, the cache will still have the value so we'll avoid a hit.

over optimisation in my opinion :>

butonic · 2022-03-08T07:09:47Z

The patch is in production as is. @jvillafanez feel free to take over the PR and polish it.

jvillafanez · 2022-03-22T07:55:32Z

This is ready to review again.

phil-davis

Maybe not worth fixing one word in a comment, unless there are other reasons to change something or rebase...

lib/private/Files/Cache/Cache.php

phil-davis · 2022-04-19T08:39:13Z

I guess this is ready for final review?

"This branch is 3 commits ahead, 102 commits behind master."
@jvillafanez IMO it is worth rebasing to get up-to-date CI.

And then request people again for review. (It looks OK to me)

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

sonarcloud · 2022-04-19T11:29:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.1% Coverage
0.0% Duplication

mrow4a

default for CappedMemoryCache not clear, and lots of (int) X casting. But overall looks good and logic approved earlier.

lib/private/Files/Cache/Cache.php

butonic force-pushed the cache-path-static branch from 28e5f1b to eee7bc0 Compare March 2, 2022 21:39

butonic requested review from micbar and jvillafanez March 2, 2022 21:47

micbar added the 3 - To Review label Mar 3, 2022

jvillafanez reviewed Mar 3, 2022

View reviewed changes

jvillafanez force-pushed the cache-path-static branch from 8e7f2e4 to 6ba1d6b Compare March 21, 2022 13:17

phil-davis reviewed Mar 22, 2022

View reviewed changes

lib/private/Files/Cache/Cache.php Outdated Show resolved Hide resolved

butonic and others added 3 commits April 19, 2022 13:05

introduce a static fileid->(storage, path) cache

970b303

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

Use a capped cache and arrays

9ccf269

Add changelog entry and fix typo

a9d3107

jvillafanez force-pushed the cache-path-static branch from b74af46 to a9d3107 Compare April 19, 2022 11:06

phil-davis requested a review from mrow4a April 19, 2022 14:06

mrow4a approved these changes Apr 23, 2022

View reviewed changes

lib/private/Files/Cache/Cache.php Show resolved Hide resolved

jvillafanez merged commit 1357792 into master May 3, 2022

delete-merged-branch bot deleted the cache-path-static branch May 3, 2022 11:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce a static fileid->(storage, path) cache #39847

introduce a static fileid->(storage, path) cache #39847

butonic commented Mar 2, 2022 •

edited

Loading

update-docs bot commented Mar 2, 2022

ownclouders commented Mar 2, 2022

ownclouders commented Mar 2, 2022

micbar commented Mar 3, 2022

mrow4a commented Mar 3, 2022

butonic commented Mar 3, 2022

mrow4a commented Mar 3, 2022

butonic commented Mar 3, 2022 •

edited

Loading

jvillafanez Mar 3, 2022

jvillafanez Mar 3, 2022

jvillafanez Mar 3, 2022

mrow4a Mar 3, 2022

jvillafanez Mar 3, 2022

mrow4a Mar 3, 2022

butonic commented Mar 8, 2022

jvillafanez commented Mar 22, 2022

phil-davis left a comment

phil-davis commented Apr 19, 2022

sonarcloud bot commented Apr 19, 2022

mrow4a left a comment

introduce a static fileid->(storage, path) cache #39847

introduce a static fileid->(storage, path) cache #39847

Conversation

butonic commented Mar 2, 2022 • edited Loading

update-docs bot commented Mar 2, 2022

ownclouders commented Mar 2, 2022

ownclouders commented Mar 2, 2022

micbar commented Mar 3, 2022

mrow4a commented Mar 3, 2022

butonic commented Mar 3, 2022

mrow4a commented Mar 3, 2022

butonic commented Mar 3, 2022 • edited Loading

jvillafanez Mar 3, 2022

Choose a reason for hiding this comment

jvillafanez Mar 3, 2022

Choose a reason for hiding this comment

jvillafanez Mar 3, 2022

Choose a reason for hiding this comment

mrow4a Mar 3, 2022

Choose a reason for hiding this comment

jvillafanez Mar 3, 2022

Choose a reason for hiding this comment

mrow4a Mar 3, 2022

Choose a reason for hiding this comment

butonic commented Mar 8, 2022

jvillafanez commented Mar 22, 2022

phil-davis left a comment

Choose a reason for hiding this comment

phil-davis commented Apr 19, 2022

sonarcloud bot commented Apr 19, 2022

mrow4a left a comment

Choose a reason for hiding this comment

butonic commented Mar 2, 2022 •

edited

Loading

butonic commented Mar 3, 2022 •

edited

Loading