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

API Enable persistent FileHashCaching #282

Conversation

maxime-rainville
Copy link
Contributor

Updates the FileHashCaching to be persistent and always be on.

Parent issue

#279

Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

I think we need to customise the cache layer here: The default cache will choose APC if available, and fall back to filesystem. APC is a relatively small in memory cache, here are the defaults in my VM:

vagrant@44:/var/www/mysite/www$ php -i | grep -i apc
...
apc.entries_hint => 4096 => 4096
apc.shm_segments => 1 => 1
apc.shm_size => 32M => 32M

So by default, you can only have 4k cache entries across all caches built by SilverStripe. Most systems will have more files than that. If combined with an infinite cache lifetime (which I'm still recommending), the SHA cache entries will crowd out other ("hotter") caches over time, meaning those caches will use the slower filesystem layer.

Note that APC caches are cleared on webserver restart (or when a new VM is provisioned as part of a full deployment like in SCP). Filesystem caches are often cleared as part of a deployment as well (either by launching a new VM, or keeping the cache folder connected to a specific deployment).

I think we should only use filesystem caches for this, and leave APC to other more important things.

_config/assetscache.yml Outdated Show resolved Hide resolved
return $keyOrFs;
}

foreach ([AssetStore::VISIBILITY_PUBLIC, AssetStore::VISIBILITY_PROTECTED] as $visibility) {
Copy link
Member

Choose a reason for hiding this comment

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

This ignores any other settings on a filesystem adapter. I'm really keen to have non-expiring caches on those hashes, but that's assuming the visibility alone is enough of a unique cache key. In other words, changes in config or metadata couldn't influence the file contents. For example, an AwsS3Adapater could have its bucket location changed (https://github.com/thephpleague/flysystem-aws-s3-v3/blob/master/src/AwsS3Adapter.php), which somehow changes the file contents. There's no toString() method on the Filesystem interface, and no common way to get "all the state which influences this adapter" other than spl_object_hash.

I think that's just a limitation we need to live with, unless we want to force everyone using other flysystem adapters to implement a wrapper that adds our own interface (toString(), getKey(), etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're going to change your adapter settings, I don't think it's an unreasonable expectation that you should flush your settings as well. In most cases you'll have to do that anyway to have your new settings take affect.

In any case, if your changes are somehow causing the hash of your files to change, you'll be completely screwed any way, because your database won't know anything about those new hashes.

@chillu
Copy link
Member

chillu commented Jun 6, 2019

Actually, it's a bit more subtle: ChainCache is initialised with APCu and Filesystem by default, and will write to every cache (see https://github.com/silverstripe/silverstripe-framework/blob/4/src/Core/Cache/DefaultCacheFactory.php). The APCu cache is defined with $defaultLifetime / 5, which is something that's been there since day one, I've copied the implementation from Symfony.

So this means APC keeps caches for less time than Filesystem caches, making it a more appropriate "hot cache". But it also means if you're setting cache lifetime to 0, the APC cache becomes '0 / 5 = 0`, and you'll start crowding out other caches. I think for this specific cacse, it's better to have longer lived caches which are a bit slower (filesystem), than shorter live caches which are faster (memory). The assumption here is that filesystem access is generally optimised via OPCode caching in PHP.

chillu added a commit to open-sausages/silverstripe-framework that referenced this pull request Jun 6, 2019
In-memory caches are typically more resource constrained (number of items and storage space).
Give cache consumers an opt-out if they are expecting to create large caches with long lifetimes.
Use case: silverstripe/silverstripe-assets#282
@chillu
Copy link
Member

chillu commented Jun 6, 2019

@maxime-rainville maxime-rainville force-pushed the pulls/1.4/always-enable-file-hash-caching branch from 9f5663c to 2d35d63 Compare June 6, 2019 22:23
maxime-rainville pushed a commit to open-sausages/silverstripe-framework that referenced this pull request Jun 7, 2019
In-memory caches are typically more resource constrained (number of items and storage space).
Give cache consumers an opt-out if they are expecting to create large caches with long lifetimes.
Use case: silverstripe/silverstripe-assets#282
@maxime-rainville
Copy link
Contributor Author

FYI I've squashed those warnings in FileIDHelperResolutionStrategyTest.php

Copy link
Member

@chillu chillu left a comment

Choose a reason for hiding this comment

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

b21bfd3 - isn't this the opposite of the fixes you've done yesterday in #287? There's other tests which started using DI, what makes this one special that it needed to be reverted, when the other ones don't?

There's still a test failure: https://travis-ci.org/silverstripe/silverstripe-assets/jobs/542541684#L883

@maxime-rainville maxime-rainville force-pushed the pulls/1.4/always-enable-file-hash-caching branch from b5dfd18 to 333dbed Compare June 7, 2019 01:57
@maxime-rainville
Copy link
Contributor Author

You can use the Injector in dataProvider methods because those get call before the manifest has been initialised. That's why I've undone those changes.

I just set the defaultLifetime to 0.

Looking at that last failure now.

@maxime-rainville
Copy link
Contributor Author

That last commit should make all the test go green.

@maxime-rainville maxime-rainville merged commit a30fd34 into silverstripe:1.4 Jun 9, 2019
@maxime-rainville maxime-rainville deleted the pulls/1.4/always-enable-file-hash-caching branch June 9, 2019 22:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants