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] fix warming up cache.system and apcu #30331

Merged
merged 1 commit into from Feb 23, 2019

Conversation

Projects
None yet
6 participants
@nicolas-grekas
Copy link
Member

commented Feb 21, 2019

Q A
Branch? 4.2
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #29601, #29877
License MIT
Doc PR -

@nicolas-grekas nicolas-grekas added this to the 4.2 milestone Feb 21, 2019

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.2 Feb 21, 2019

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-system branch from 0564302 to 70b6994 Feb 21, 2019

@@ -108,16 +108,12 @@ function ($deferred, $namespace, &$expiredIds) use ($getId) {
*/
public static function createSystemCache($namespace, $defaultLifetime, $version, $directory, LoggerInterface $logger = null)
{
if (null === self::$apcuSupported) {
self::$apcuSupported = ApcuAdapter::isSupported();
$opcache = new PhpFilesAdapter($namespace, $defaultLifetime, $directory, true);

This comment has been minimized.

Copy link
@bendavies

bendavies Feb 21, 2019

Contributor

no PhpFilesAdapter::isSupported(); falling back to FilesystemAdapter?

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 21, 2019

Author Member

Nope, unconditional is simpler and we don't care if you don't have opcache :)

This comment has been minimized.

Copy link
@bendavies

bendavies Feb 21, 2019

Contributor

but, if the user does not have opcache or apcu, then you get no caching at all? possible hard to debug? up to the user to manually configure?

This comment has been minimized.

Copy link
@bendavies

bendavies Feb 21, 2019

Contributor

just looked at it does sort of fallback to filesystem in 4.1 FYI

public static function createSystemCache($namespace, $defaultLifetime, $version, $directory, LoggerInterface $logger = null)
{
if (null === self::$apcuSupported) {
self::$apcuSupported = ApcuAdapter::isSupported();
}
if (!self::$apcuSupported && null === self::$phpFilesSupported) {
self::$phpFilesSupported = PhpFilesAdapter::isSupported();
}
if (self::$phpFilesSupported) {
$opcache = new PhpFilesAdapter($namespace, $defaultLifetime, $directory);
if (null !== $logger) {
$opcache->setLogger($logger);
}
return $opcache;
}
$fs = new FilesystemAdapter($namespace, $defaultLifetime, $directory);
if (null !== $logger) {
$fs->setLogger($logger);
}
if (!self::$apcuSupported) {
return $fs;
}
$apcu = new ApcuAdapter($namespace, (int) $defaultLifetime / 5, $version);
if ('cli' === \PHP_SAPI && !filter_var(ini_get('apc.enable_cli'), FILTER_VALIDATE_BOOLEAN)) {
$apcu->setLogger(new NullLogger());
} elseif (null !== $logger) {
$apcu->setLogger($logger);
}
return new ChainAdapter([$apcu, $fs]);

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 21, 2019

Author Member

if you don't have opcache, you don't care about that

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Feb 21, 2019

Author Member

no opcache means you will still have the cache - but php will parse the files all the time

This comment has been minimized.

Copy link
@bendavies

bendavies Feb 21, 2019

Contributor

ah yes! gotcha.

@nicolas-grekas nicolas-grekas changed the title Cache system [Cache] fix warming up cache.system and apcu Feb 21, 2019

@dkarlovi

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Maybe related? #29877

@xabbuh

xabbuh approved these changes Feb 21, 2019

@dkarlovi
Copy link
Contributor

left a comment

Confirming this fix brings back my test suite runtime from 4.2 amount (9:25) back to 4.1 amount (4:10).

Since it's a regression, a test is probably required to avoid having it reappear?

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2019

I don't think a test case would have prevented the change - I would have changed the tests at the time actually.

@@ -108,16 +108,12 @@ function ($deferred, $namespace, &$expiredIds) use ($getId) {
*/
public static function createSystemCache($namespace, $defaultLifetime, $version, $directory, LoggerInterface $logger = null)

This comment has been minimized.

Copy link
@Tobion

Tobion Feb 22, 2019

Member

Returns an ApcuAdapter if supported, a PhpFilesAdapter otherwise.

Does not seem to be precise anymore.

@Tobion

Tobion approved these changes Feb 22, 2019

@dkarlovi

This comment has been minimized.

Copy link
Contributor

commented Feb 22, 2019

I meant adding a test would prevent a future regression.

Also, this doesn't seem to fix #29877 since that issue is due to incorrectly detecting APCu in CLI.

@nicolas-grekas nicolas-grekas force-pushed the nicolas-grekas:cache-system branch from 70b6994 to b0a85ad Feb 23, 2019

@nicolas-grekas nicolas-grekas merged commit b0a85ad into symfony:4.2 Feb 23, 2019

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Feb 23, 2019

bug #30331 [Cache] fix warming up cache.system and apcu (nicolas-grekas)
This PR was merged into the 4.2 branch.

Discussion
----------

[Cache] fix warming up cache.system and apcu

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29601, #29877
| License       | MIT
| Doc PR        | -

Commits
-------

b0a85ad [Cache] fix warming up cache.system and apcu

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:cache-system branch Feb 23, 2019

@fabpot fabpot referenced this pull request Mar 3, 2019

Merged

Release v4.2.4 #30431

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.