Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RFC: Cache locally the filecache information #31793

Closed
wants to merge 1 commit into from
Closed

RFC: Cache locally the filecache information #31793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
162 changes: 103 additions & 59 deletions lib/private/Files/Cache/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
use OCP\Files\Storage\IStorage;
use OCP\IDBConnection;
use Psr\Log\LoggerInterface;
use OC\Cache\CappedMemoryCache;

/**
* Metadata cache for a storage
Expand All @@ -76,41 +77,21 @@ class Cache implements ICache {
}

/**
* @var array partial data for the cache
* Internal cache that allows to store temporarely data without having to
* fetch them from the database
*/
protected $partial = [];
protected CappedMemoryCache $internalCache;

/** @var array partial data for the cache */
protected array $partial = [];
protected string $storageId;
private IStorage $storage;
protected Storage $storageCache;
protected IMimeTypeLoader $mimetypeLoader;
protected IDBConnection $connection;
protected IEventDispatcher $eventDispatcher;
protected QuerySearchHelper $querySearchHelper;

/**
* @var string
*/
protected $storageId;

private $storage;

/**
* @var Storage $storageCache
*/
protected $storageCache;

/** @var IMimeTypeLoader */
protected $mimetypeLoader;

/**
* @var IDBConnection
*/
protected $connection;

/**
* @var IEventDispatcher
*/
protected $eventDispatcher;

/** @var QuerySearchHelper */
protected $querySearchHelper;

/**
* @param IStorage $storage
*/
public function __construct(IStorage $storage) {
$this->storageId = $storage->getId();
$this->storage = $storage;
Expand All @@ -123,9 +104,10 @@ public function __construct(IStorage $storage) {
$this->connection = \OC::$server->getDatabaseConnection();
$this->eventDispatcher = \OC::$server->get(IEventDispatcher::class);
$this->querySearchHelper = \OC::$server->query(QuerySearchHelper::class);
$this->internalCache = new CappedMemoryCache();
}

protected function getQueryBuilder() {
protected function getQueryBuilder(): CacheQueryBuilder {
return new CacheQueryBuilder(
$this->connection,
\OC::$server->getSystemConfig(),
Expand All @@ -143,12 +125,18 @@ public function getNumericStorageId() {
}

/**
* get the stored metadata of a file or folder
* Get the stored metadata of a file or folder
*
* @param string | int $file either the path of a file or folder or the file id for a file or folder
* @param string|int $file either the path of a file or folder or the file id for a file or folder
* @return ICacheEntry|false the cache entry as array of false if the file is not found in the cache
*/
public function get($file) {
// Try fetching from memory cache first
if ($this->internalCache->hasKey($file)) {
return $this->internalCache->get($file);
}

// Fetch cache entry from database
$query = $this->getQueryBuilder();
$query->selectFileCache();

Expand All @@ -166,25 +154,22 @@ public function get($file) {
$data = $result->fetch();
$result->closeCursor();

//merge partial data
// If no cache entry is found, return the partially saved in-memory data
if (!$data && is_string($file) && isset($this->partial[$file])) {
return $this->partial[$file];
} elseif (!$data) {
return $data;
} else {
return self::cacheEntryFromData($data, $this->mimetypeLoader);
}

if (!$data) {
return false;
}
return self::cacheEntryFromData($data, $this->mimetypeLoader);
}

/**
* Create a CacheEntry from database row
*
* @param array $data
* @param IMimeTypeLoader $mimetypeLoader
* @return CacheEntry
*/
public static function cacheEntryFromData($data, IMimeTypeLoader $mimetypeLoader) {
//fix types
public static function cacheEntryFromData(array $data, IMimeTypeLoader $mimetypeLoader): CacheEntry {
// fix types
$data['fileid'] = (int)$data['fileid'];
$data['parent'] = (int)$data['parent'];
$data['size'] = 0 + $data['size'];
Expand Down Expand Up @@ -221,7 +206,7 @@ public function getFolderContents($folder) {
}

/**
* get the metadata of all files stored in $folder
* Get the metadata of all files stored in $folder
*
* @param int $fileId the file id of the folder
* @return ICacheEntry[]
Expand All @@ -237,15 +222,18 @@ public function getFolderContentsById($fileId) {
$files = $result->fetchAll();
$result->closeCursor();

return array_map(function (array $data) {
return self::cacheEntryFromData($data, $this->mimetypeLoader);
return array_map(function (array $data): ICacheEntry {
$cacheEntry = self::cacheEntryFromData($data, $this->mimetypeLoader);
$this->internalCache->set($cacheEntry->getId(), $cacheEntry);
$this->internalCache->set($cacheEntry->getPath(), $cacheEntry);
return $cacheEntry;
}, $files);
}
return [];
}

/**
* insert or update meta data for a file or folder
* Insert or update meta data for a file or folder
*
* @param string $file
* @param array $data
Expand All @@ -263,7 +251,7 @@ public function put($file, array $data) {
}

/**
* insert meta data for a new file or folder
* Insert meta data for a new file or folder
*
* @param string $file
* @param array $data
Expand Down Expand Up @@ -316,10 +304,34 @@ public function insert($file, array $data) {
$query->setValue('fileid', $query->createNamedParameter($fileId, IQueryBuilder::PARAM_INT));
foreach ($extensionValues as $column => $value) {
$query->setValue($column, $query->createNamedParameter($value));
$values[$column] = $value;
}
$query->execute();
}

// Add possible missing values
$values['fileid'] = $fileId;
$valuesDefault = [
'encrypted' => 0,
'storage_mtime' => 0,
'permissions' => 0,
'etag' => null,
'checksum' => null,
'metadata_etag' => null,
'creation_time' => null,
'upload_time' => null,
];
Comment on lines +313 to +323
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only part I don't like.
Hardcoded values? Do we have a single point of truth for what can be populated as file data? Is this not dynamic?
Any reason we only store those ones?

As a summary, could you give a little background on how you found that specific list? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the default value coming from the database, I don't like that we duplicate it either

Currently, we basically create an entry in the DB with partial data, then the missing data is filled by the DB with the default value and then we fetch the object from the DB.

foreach ($valuesDefault as $key => $default) {
if (!array_key_exists($key, $values)) {
$values[$key] = $default; // not set => set it to 0 as default
}
}
// FIXME maybe we should fix the test instead
// $values['storageid'] = (string)$values['storageid'];

// Save in in-memory cache
$this->internalCache->set($fileId, self::cacheEntryFromData($values, $this->mimetypeLoader));

$event = new CacheEntryInsertedEvent($this->storage, $file, $fileId, $storageId);
$this->eventDispatcher->dispatch(CacheInsertEvent::class, $event);
$this->eventDispatcher->dispatchTyped($event);
Expand Down Expand Up @@ -360,6 +372,7 @@ public function update($id, array $data) {
}

[$values, $extensionValues] = $this->normalizeData($data);
$valuesToUpdateInCache = [];

if (count($values)) {
$query = $this->getQueryBuilder();
Expand All @@ -375,6 +388,7 @@ public function update($id, array $data) {

foreach ($values as $key => $value) {
$query->set($key, $query->createNamedParameter($value));
$valuesToUpdateInCache[$key] = $value;
}

$query->execute();
Expand All @@ -388,6 +402,7 @@ public function update($id, array $data) {
$query->setValue('fileid', $query->createNamedParameter($id, IQueryBuilder::PARAM_INT));
foreach ($extensionValues as $column => $value) {
$query->setValue($column, $query->createNamedParameter($value));
$valuesToUpdateInCache[$column] = $value;
}

$query->execute();
Expand All @@ -413,6 +428,16 @@ public function update($id, array $data) {
$path = $this->getPathById($id);
// path can still be null if the file doesn't exist
if ($path !== null) {
if ($this->internalCache->hasKey($id)) {
// Only update when we already have an entry in the cache
// otherwise we don't have all the values
$cacheEntry = $this->internalCache->get($id);
foreach ($valuesToUpdateInCache as $key => $value) {
$cacheEntry[$key] = $value;
}
$this->internalCache->set($id, $cacheEntry);
}

$event = new CacheEntryUpdatedEvent($this->storage, $path, $id, $this->getNumericStorageId());
$this->eventDispatcher->dispatch(CacheUpdateEvent::class, $event);
$this->eventDispatcher->dispatchTyped($event);
Expand Down Expand Up @@ -469,7 +494,7 @@ protected function normalizeData(array $data): array {
}

/**
* get the file id for a file
* Get the file id for a file
*
* A file id is a numeric id for a file or folder that's unique within an owncloud instance which stays the same for the lifetime of a file
*
Expand All @@ -482,6 +507,10 @@ public function getId($file) {
// normalize file
$file = $this->normalize($file);

if ($this->internalCache->hasKey($file)) {
return $this->internalCache->get($file)->getId();
}

$query = $this->getQueryBuilder();
$query->select('fileid')
->from('filecache')
Expand All @@ -505,6 +534,9 @@ public function getParentId($file) {
if ($file === '') {
return -1;
} else {
if ($this->internalCache->hasKey($file)) {
return $this->internalCache->get($file)['parent'];
}
$parent = $this->getParentPath($file);
return (int)$this->getId($parent);
}
Expand All @@ -529,9 +561,9 @@ public function inCache($file) {
}

/**
* remove a file or folder from the cache
* Remove a file or folder from the cache
*
* when removing a folder from the cache all files and folders inside the folder will be removed as well
* When removing a folder from the cache all files and folders inside the folder will be removed as well.
*
* @param string $file
*/
Expand All @@ -549,14 +581,22 @@ public function remove($file) {
->whereFileId($entry->getId());
$query->execute();

if ($entry->getMimeType() == FileInfo::MIMETYPE_FOLDER) {
if ($entry->getMimeType() === FileInfo::MIMETYPE_FOLDER) {
$this->removeChildren($entry);
$this->internalCache->clear();
} else {
$this->internalCache->remove($entry->getId());
$this->internalCache->remove($entry->getPath());
}

$this->eventDispatcher->dispatchTyped(new CacheEntryRemovedEvent($this->storage, $entry->getPath(), $entry->getId(), $this->getNumericStorageId()));
}
}

public function clearInternalCache(): void {
$this->internalCache->clear();
}

/**
* Get all sub folders of a folder
*
Expand All @@ -565,8 +605,8 @@ public function remove($file) {
*/
private function getSubFolders(ICacheEntry $entry) {
$children = $this->getFolderContentsById($entry->getId());
return array_filter($children, function ($child) {
return $child->getMimeType() == FileInfo::MIMETYPE_FOLDER;
return array_filter($children, function ($child):bool {
return $child->getMimeType() === FileInfo::MIMETYPE_FOLDER;
});
}

Expand All @@ -576,7 +616,7 @@ private function getSubFolders(ICacheEntry $entry) {
* @param ICacheEntry $entry the cache entry of the folder to remove the children of
* @throws \OC\DatabaseException
*/
private function removeChildren(ICacheEntry $entry) {
private function removeChildren(ICacheEntry $entry): void {
$parentIds = [$entry->getId()];
$queue = [$entry->getId()];

Expand Down Expand Up @@ -690,6 +730,7 @@ public function moveFromCache(ICache $sourceCache, $sourcePath, $targetPath) {

try {
$query->execute();
$this->internalCache->clear();
} catch (\OC\DatabaseException $e) {
$this->connection->rollBack();
throw $e;
Expand All @@ -705,6 +746,9 @@ public function moveFromCache(ICache $sourceCache, $sourcePath, $targetPath) {
->set('parent', $query->createNamedParameter($newParentId, IQueryBuilder::PARAM_INT))
->whereFileId($sourceId);
$query->execute();
$this->internalCache->clear();
$this->internalCache->remove($sourceId);
$this->internalCache->remove($sourceData->getPath());

$this->connection->commit();

Expand Down
2 changes: 2 additions & 0 deletions lib/private/Files/Cache/Propagator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ public function propagateChange($internalPath, $time, $sizeDifference = 0) {

$builder->execute();
}
$this->storage->getCache()->clearInternalCache();
}

protected function getParents($path) {
Expand Down Expand Up @@ -195,5 +196,6 @@ public function commitBatch() {
$this->batch = [];

$this->connection->commit();
$this->storage->getCache()->clearInternalCache();
}
}
2 changes: 2 additions & 0 deletions lib/private/Files/Cache/Wrapper/CacheWrapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@

use OC\Files\Cache\Cache;
use OC\Files\Cache\QuerySearchHelper;
use OC\Cache\CappedMemoryCache;
use OCP\Files\Cache\ICache;
use OCP\Files\Cache\ICacheEntry;
use OCP\Files\Search\ISearchOperator;
Expand All @@ -50,6 +51,7 @@ public function __construct($cache) {
$this->mimetypeLoader = \OC::$server->getMimeTypeLoader();
$this->connection = \OC::$server->getDatabaseConnection();
$this->querySearchHelper = \OC::$server->get(QuerySearchHelper::class);
$this->internalCache = new CappedMemoryCache();
}

protected function getCache() {
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/Files/ViewTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ public function testCacheAPI() {
$this->assertFalse($cachedData['encrypted']);
$id = $rootView->putFileInfo('/foo.txt', ['encrypted' => true]);
$cachedData = $rootView->getFileInfo('/foo.txt');
$this->assertTrue($cachedData['encrypted']);
$this->assertTrue((bool)$cachedData['encrypted']);
$this->assertEquals($cachedData['fileid'], $id);

$this->assertFalse($rootView->getFileInfo('/non/existing'));
Expand Down