Cache in filecache #28166

butonic · 2017-06-20T10:57:36Z

alternative approach to #28165

DeepDiver1975 · 2017-06-20T14:51:54Z

lib/private/Files/Cache/Cache.php

@@ -79,6 +80,11 @@ class Cache implements ICache {
 	protected $connection;

 	/**
+	 * @var CappedMemoryCache
+	 */
+	public static $metaDataCache = null;


why static?

testing revealed that we instantiate more than one instance of this class which works against the caching effect.

DeepDiver1975 · 2017-06-20T14:52:35Z

lib/private/Files/Cache/Cache.php

@@ -137,9 +155,10 @@ public function get($file) {
 		//merge partial data
 		if (!$data and is_string($file)) {
 			if (isset($this->partial[$file])) {
-				$data = $this->partial[$file];
+				return $this->partial[$file]; // return here, no need cache partial entries


are we sure this is not necessary?

DeepDiver1975 · 2017-06-20T14:54:01Z

lib/private/Files/Cache/Wrapper/CacheIndex.php

+	/**
+	 * @var CappedMemoryCache
+	 */
+	private static $metaDataCache = null;


um leftover code from the previous attempt.

PVince81 · 2017-06-21T08:06:36Z

@mrow4a would be good if you could also check this out. I remember you looked into wasteful DB queries related to file cache. Maybe you have more insights. Didn't you have a PR adressing this or some piece of this differently ?

mrow4a · 2017-06-22T12:30:22Z

Will have a look also later, looks really promising, it is something I wanted to have. There is one disavantage -> if someone forgets to use CacheIndex somewhere and uses filecache directly to write (read is ok) -> 💥

mrow4a · 2017-06-27T09:54:16Z

Any new comments? I would really see sth like that be merged. Would small "filecache code" refactor and protection be needed here?

PVince81 · 2017-06-27T11:11:02Z

I'm not sure. It feels like a workaround on top of a sub-par architecture.

On the other hand, caching stuff locally is what we've been doing everywhere with other things to prevent talking to the DB repeatedly.

felixboehm · 2017-07-18T12:37:00Z

moved to triage, assuming this will not be ready for 10.0.3

tomneedham · 2017-07-24T11:16:52Z

Rebased

butonic · 2017-09-06T07:55:07Z

encryption also needs changes: owncloud/encryption#13

tomneedham · 2017-09-06T17:12:00Z

8.2 -> 10.0.3beta2 upgrade succeeded with this - which will have helped speed up the upgrade process for large amounts of reshares when coming from 8.2.

PVince81 · 2017-09-06T17:19:07Z

metaDataCache should rather be an instance variable of our default Cache implementation.

Custom cache implementations would not benefit from this and might not even extend from our Cache implementations.

The only advantage of having it static here is the ability to quickly clear it from various locations, while normally you should go to the storage and do a $storage->getCache() which is usually more expensive.

PVince81 · 2017-09-06T17:19:54Z

Also I wonder if this should/could be implemented as a CacheWrapper instead. We might lose the ability to clear it from outside unless we keep the staticness.

PVince81 · 2017-09-06T17:22:11Z

lib/private/Files/Cache/Cache.php

 			}, $files);
 		} else {
 			return [];
 		}
 	}

+	private function fixTypes(array &$data) {
+		$data['fileid'] = 0 + $data['fileid'];


do we really need this here for caching ? I expect to see potential additional issues

never mind, I see this was moved from another place

tomneedham · 2017-09-06T19:31:24Z

Happy to postpone this.

butonic · 2017-09-07T07:15:04Z

@PVince81 The initial implementation was a cache wrapper: https://github.com/owncloud/core/pull/28165/files

Nice and clean approach ... isn't it? Unfortunately, we

Instantiate multiple instances for the same share storages, defeating the cache effect.
Tests, encryption and other technical debt directly manipulate the filecache in the db by executing their own SQL.

I added the necessary things we need to do: https://github.com/owncloud/core/pull/28166/files#diff-b4012cb6106f634acc5ff12ef395021fR85-R87

so ... no this cannot be an instance variable ... because ... still too much crappy code in other places.

PVince81 · 2017-09-07T08:28:38Z

so ... no this cannot be an instance variable ... because ... still too much crappy code in other places.

Argh, you're right

mrow4a · 2018-05-01T07:12:17Z

I can see one disadvantage - if new code is written which writes to filecache, dev needs to be aware that cache needs to be cleaned with that static function. Apart from that - if no fragment of the code which writes to filecache has not been forgoten - looks good.

mrow4a · 2018-05-05T11:07:44Z

lib/private/Files/Cache/Cache.php

+	 *
+	 * FIXME first, refactor this class as an entity and add any necessary methods to its mapper
+	 * FIXME then, get rid of all the places that directly touch the tables
+	 * FIXME finally, make this private non-static


Do you plan it to have as separate PR? Do you expect it to be large change later?

mrow4a

@butonic please see the comments

mrow4a · 2018-05-05T11:12:09Z

lib/private/Files/Cache/Cache.php

@@ -128,45 +154,21 @@ public function get($file) {
 		$result = $this->connection->executeQuery($sql, $params);
 		$data = $result->fetch();

-		//FIXME hide this HACK in the next database layer, or just use doctrine and get rid of MDB2 and PDO


Well, not sure if you removed the hack, so FIXME still needs to stay

Whole logic is not changed, and under function fixTypes

mrow4a · 2018-05-05T11:15:18Z

lib/private/Files/Cache/Cache.php

@@ -78,6 +79,18 @@ class Cache implements ICache {
 	 */
 	protected $connection;

+	/**
+	 * caches 2048 filecache rows


Why 2048? This PR usecase is I guess a repetitive calls during GET and PUT. Not sure if we need more then 32 rows in cache. What do you think? Won't it impact PROPFIND memory usage too much?

mrow4a · 2018-05-05T11:17:20Z

lib/private/Files/Cache/Cache.php

@@ -316,6 +346,7 @@ public function update($id, array $data) {
 			') AND `fileid` = ? ';
 		$this->connection->executeQuery($sql, $params);

+		self::$metaDataCache->clear();


I am not sure if this shouldnt be as first line. There is small chance your filecache will be out of sync for very small time (not sure if this is that important)

mrow4a · 2018-05-05T11:18:25Z

lib/private/Files/Cache/Cache.php

@@ -441,6 +482,7 @@ public function remove($file) {
 		if ($entry['mimetype'] === 'httpd/unix-directory') {
 			$this->removeChildren($entry);
 		}
+		self::$metaDataCache->clear();


the same here

mrow4a · 2018-05-05T11:18:46Z

lib/private/Files/Cache/Cache.php

@@ -544,12 +587,14 @@ public function moveFromCache(ICache $sourceCache, $sourcePath, $targetPath) {
 		} else {
 			$this->moveFromCacheFallback($sourceCache, $sourcePath, $targetPath);
 		}
+		self::$metaDataCache->clear();


PVince81 · 2018-05-15T09:57:59Z

please rebase for more recent CI
please run smashbox tests against this to confirm

butonic · 2018-05-15T10:30:01Z

move more sql to the filecache class
always clear filecache in memory cache after every repair step
always clear filecache in memory cache after every background job
always clear filecache in memory cache after every test class

mrow4a · 2018-05-19T23:37:38Z

Safer, alternative approach to this PR by fixing the app logic - #31486

DeepDiver1975 · 2018-06-22T14:39:15Z

@butonic would you agree on pushing #31486 instead of this one? THX

mrow4a · 2018-06-27T21:41:01Z

apps/files/tests/BackgroundJob/DeleteOrphanedItemsJobTest.php

@@ -220,6 +229,9 @@ public function testClearCommentReadMarks() {
 				'path' => $query->createNamedParameter('apps/files/tests/deleteorphanedtagsjobtest.php'),
 				'path_hash' => $query->createNamedParameter(\md5('apps/files/tests/deleteorphanedtagsjobtest.php')),
 			])->execute();
+		if (Cache::$metaDataCache !== null) {


Clear metadata cache before executing the query.

mrow4a · 2018-06-27T21:41:15Z

apps/files_sharing/lib/Command/CleanupRemoteStorages.php

@@ -129,6 +130,9 @@ public function deleteFiles ($numericId, OutputInterface $output) {
 			);
 		$output->write("deleting files for storage $numericId ... ");
 		$count = $queryBuilder->execute();
+		if (Cache::$metaDataCache !== null) {


clear before executing the query

mrow4a · 2018-06-27T21:41:38Z

apps/files_sharing/tests/Command/CleanupRemoteStoragesTest.php

@@ -101,6 +102,9 @@ protected function setup() {
 					$filesQuery->setParameter(2, \md5('file' . $i));
 					$filesQuery->execute();
 				}
+				if (Cache::$metaDataCache !== null) {


the same (clear)

mrow4a · 2018-06-27T21:41:44Z

apps/testing/lib/BigFileID.php

@@ -53,6 +54,9 @@ public function increaseFileIDsBeyondMax32bits(){
 												'', '4', '3', '163', '1499256550', '1499256550',
 												'0', '0', '595cd6e63f375', '27', 'NULL')";
 			$this->connection->executeQuery($query);
+			if (Cache::$metaDataCache !== null) {


the same (clear)

mrow4a · 2018-06-27T21:41:50Z

lib/private/Files/Cache/Propagator.php

@@ -96,6 +96,10 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) {
 		}

 		$builder->execute();
+


the same (clear)

also in other places

butonic · 2019-02-18T09:58:02Z

Closing in favor of OCIS.

DeepDiver1975 previously requested changes Jun 20, 2017

View reviewed changes

This was referenced Jun 29, 2017

[stable9] cache in filecache #28260

Closed

Handle inconsistent filecache reads #28273

Merged

PVince81 added the 2 - Developing label Jul 4, 2017

PVince81 added this to the development milestone Jul 4, 2017

mrow4a mentioned this pull request Jul 4, 2017

[WIP] no trip to the db is better than a fast trip to the db #28165

Closed

9 tasks

felixboehm modified the milestones: triage, development Jul 18, 2017

tomneedham force-pushed the cache-in-filecache branch from e0e3d27 to d4dc116 Compare July 24, 2017 11:16

butonic force-pushed the cache-in-filecache branch from d4dc116 to 3783534 Compare September 6, 2017 07:33

tomneedham modified the milestones: development, triage Sep 6, 2017

tomneedham added 3 - To Review and removed 2 - Developing labels Sep 6, 2017

PVince81 mentioned this pull request Sep 6, 2017

clear filecache cache manually owncloud/encryption#13

Closed

tomneedham changed the title ~~[WIP] Cache in filecache~~ Cache in filecache Sep 6, 2017

PVince81 reviewed Sep 6, 2017

View reviewed changes

butonic added performance comp:filesystem labels Apr 30, 2018

mrow4a reviewed May 5, 2018

View reviewed changes

ownclouders added the status/STALE label May 13, 2018

ownclouders removed the status/STALE label May 15, 2018

This was referenced May 19, 2018

Don't check for existence twice if we know it is not a part file (objectstorage only) #31299

Closed

Temporarily cache objectstore stat metadata for duration of storage->getMetadata #31482

Closed

mrow4a mentioned this pull request May 20, 2018

Objectstore storage metadata layer handler #31486

Closed

7 tasks

ownclouders added the status/STALE label May 27, 2018

ownclouders removed the status/STALE label Jun 22, 2018

mrow4a reviewed Jun 27, 2018

View reviewed changes

mrow4a mentioned this pull request Jun 30, 2018

[objectstorage] Cache stat while fetching objectstore object metadata #31958

Merged

ownclouders added the status/STALE label Jul 5, 2018

butonic mentioned this pull request Oct 26, 2018

do not iterate over all storages when you only need a single node anyway #33302

Merged

ownclouders mentioned this pull request Feb 11, 2019

make CI cache more reliable #34435

Closed

butonic closed this Feb 18, 2019

ownclouders removed the status/STALE label Feb 18, 2019

butonic deleted the cache-in-filecache branch February 18, 2019 09:58

lock bot locked as resolved and limited conversation to collaborators Feb 18, 2020

		@@ -96,6 +96,10 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) {
		}

		$builder->execute();

Cache in filecache #28166

Cache in filecache #28166

Conversation

butonic commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jun 21, 2017

mrow4a commented Jun 22, 2017 • edited Loading

mrow4a commented Jun 27, 2017 • edited Loading

PVince81 commented Jun 27, 2017

felixboehm commented Jul 18, 2017

tomneedham commented Jul 24, 2017

butonic commented Sep 6, 2017 • edited Loading

tomneedham commented Sep 6, 2017

PVince81 commented Sep 6, 2017

PVince81 commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomneedham commented Sep 6, 2017

butonic commented Sep 7, 2017 • edited Loading

PVince81 commented Sep 7, 2017

mrow4a commented May 1, 2018

Choose a reason for hiding this comment

mrow4a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented May 15, 2018

butonic commented May 15, 2018

mrow4a commented May 19, 2018 • edited Loading

DeepDiver1975 commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

butonic commented Feb 18, 2019

mrow4a commented Jun 22, 2017 •

edited

Loading

mrow4a commented Jun 27, 2017 •

edited

Loading

butonic commented Sep 6, 2017 •

edited

Loading

butonic commented Sep 7, 2017 •

edited

Loading

mrow4a commented May 19, 2018 •

edited

Loading