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.

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

Cache DBFile::exists() and reduce calls to file_exists() #388

Open
wants to merge 1 commit into
base: 1
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion src/Flysystem/FlysystemAssetStore.php
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ private function applyToFileOnFilesystem(callable $callable, ParsedFileID $parse
if ($parsedFileID->getHash()) {
$mainFileID = $strategy->buildFileID($strategy->stripVariant($parsedFileID));

if (!$fs->has($mainFileID)) {
if ($mainFileID !== $fileID && !$fs->has($mainFileID)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

That part is low-impact, I'm happy to merge it as an incremental improvement.

// The main file doesn't exists ... this is kind of weird.
continue;
}
Expand Down
70 changes: 70 additions & 0 deletions src/Services/ReadOnlyCacheService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
<?php

namespace SilverStripe\Assets\Services;

use SilverStripe\Core\Injector\Injectable;

/**
* Used to cache results for the duration of a request during read-only file operations
* Do not use this during any create, update or delete operations
*/
class ReadOnlyCacheService
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this should implement the Flushable interface?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should officially support this API. Mark it as @internal.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not flushable, there's nothing to flush. The cache is only used for a single request and then it's gone

Copy link
Member

Choose a reason for hiding this comment

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

Resettable is the appropriate interface for classes which have in-memory caches.

{

use Injectable;

private $enabled = false;

private $cache = [];

public function getEnabled(): bool
{
return $this->enabled;
}

public function setEnabled(bool $enabled): void
{
$this->enabled = $enabled;
}

public function reset(?array $cacheNameComponents = null): void
{
if (is_null($cacheNameComponents)) {
$this->cache = [];
return;
}
$cacheName = $this->buildKey($cacheNameComponents);
if (isset($this->cache[$cacheName])) {
unset($this->cache[$cacheName]);
}
}

public function setValue(array $cacheNameComponents, array $cacheKeyComponents, $value): void
{
$cacheName = $this->buildKey($cacheNameComponents);
$key = $this->buildKey($cacheKeyComponents);
if (!isset($this->cache[$cacheName])) {
$this->cache[$cacheName] = [];
}
$this->cache[$cacheName][$key] = $value;
}

public function getValue(array $cacheNameComponents, array $cacheKeyComponents)
{
$cacheName = $this->buildKey($cacheNameComponents);
$key = $this->buildKey($cacheKeyComponents);
return $this->cache[$cacheName][$key] ?? null;
}

public function hasValue(array $cacheNameComponents, array $cacheKeyComponents): bool
{
$cacheName = $this->buildKey($cacheNameComponents);
$key = $this->buildKey($cacheKeyComponents);
return isset($this->cache[$cacheName]);
}

private function buildKey(array $components)
{
return implode('-', $components);
}
}
13 changes: 12 additions & 1 deletion src/Storage/DBFile.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use SilverStripe\Assets\File;
use SilverStripe\Assets\ImageManipulation;
use SilverStripe\Assets\Services\ReadOnlyCacheService;
use SilverStripe\Assets\Thumbnail;
use SilverStripe\Control\Director;
use SilverStripe\Core\Injector\Injector;
Expand Down Expand Up @@ -359,12 +360,22 @@ public function getVisibility()

public function exists()
{
emteknetnz marked this conversation as resolved.
Show resolved Hide resolved
$cacheService = ReadOnlyCacheService::singleton();
$cacheNameComponents = [__CLASS__, __FUNCTION__];
$dbFileComponents = [$this->Filename, $this->Hash, $this->Variant];
if ($cacheService->getEnabled() && $cacheService->hasValue($cacheNameComponents, $dbFileComponents)) {
return $cacheService->getValue($cacheNameComponents, $dbFileComponents);
}
if (empty($this->Filename)) {
return false;
}
return $this
$exists = $this
->getStore()
->exists($this->Filename, $this->Hash, $this->Variant);
if ($cacheService->getEnabled()) {
$cacheService->setValue($cacheNameComponents, $dbFileComponents, $exists);
}
return $exists;
}

public function getFilename()
Expand Down
17 changes: 14 additions & 3 deletions src/Storage/Sha1FileHashingService.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use InvalidArgumentException;
use League\Flysystem\Filesystem;
use League\Flysystem\Util;
use League\Flysystem\FileNotFoundException;
use Psr\SimpleCache\CacheInterface;
use SilverStripe\Core\Config\Configurable;
use SilverStripe\Core\Flushable;
Expand Down Expand Up @@ -189,10 +190,20 @@ private function buildCacheKey($fileID, $fs)
*/
private function getTimestamp($fileID, $fs)
{
$timestamp = '';
Copy link
Contributor

Choose a reason for hiding this comment

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

That part is low-impact, I'm happy to merge it as an incremental improvement.

$filesystem = $this->getFilesystem($fs);
return $filesystem->has($fileID) ?
$filesystem->getTimestamp($fileID) :
DBDatetime::now()->getTimestamp();
// Using a try/catch block instead of Filesystem::has() because that's already
// called in Filesystem::assertPresent()
try {
// Filesystem::getTimestamp($path) will get a timestamp of the physical file
// if it fails for whatever reason, then it will either
// a) return false, or
// b) throw a FileNotFoundException from Filesystem::assertPresent()
$timestamp = $filesystem->getTimestamp($fileID);
} catch (FileNotFoundException $exception) {
// do nothing
}
return $timestamp ?: DBDatetime::now()->getTimestamp();
}

public function invalidate($fileID, $fs)
Expand Down
40 changes: 40 additions & 0 deletions tests/php/Services/ReadOnlyCacheServiceTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
<?php

namespace SilverStripe\Assets\Tests\Services;

use SilverStripe\Assets\Services\ReadOnlyCacheService;
use SilverStripe\Dev\SapphireTest;

class ReadOnlyCacheServiceTest extends SapphireTest
{

public function testGetSetEnabled()
{
$service = ReadOnlyCacheService::singleton();
$this->assertFalse($service->getEnabled());
$service->setEnabled(true);
$this->assertTrue($service->getEnabled());
}

public function testGetSetHasValue()
{
$service = ReadOnlyCacheService::singleton();
$this->assertFalse($service->hasValue(['A', 'B'], ['1', '2']));
$service->setValue(['A', 'B'], ['1', '2'], 'xyz');
$this->assertTrue($service->hasValue(['A', 'B'], ['1', '2']));
$this->assertEquals('xyz', $service->getValue(['A', 'B'], ['1', '2']));
}

public function testReset()
{
$service = ReadOnlyCacheService::singleton();
$service->setValue(['A', 'B'], ['1', '2'], 'xyz');
$service->setValue(['C', 'D'], ['3', '4'], 'wvu');
$this->assertTrue($service->hasValue(['A', 'B'], ['1', '2']));
$service->reset(['A', 'B']);
$this->assertFalse($service->hasValue(['A', 'B'], ['1', '2']));
$this->assertTrue($service->hasValue(['C', 'D'], ['3', '4']));
$service->reset();
$this->assertFalse($service->hasValue(['C', 'D'], ['3', '4']));
}
}