Now that we're about to leverage OPCache shared memory even for objects (see#27543), there's no reason anymore to use APCu for system caches. Let's remove some complexity here.

As a bonus, this makes system caches pruneable using thecache:pool:prune command.

nicolas-grekas added this to thenext milestone

Jun 7, 2018

carsonbot added Status: Needs Review Cache Deprecation labels

Jun 7, 2018

nicolas-grekas force-pushed thecache-deprec-system-factory branch fromb2b40e4 to8cf36c7Compare

June 7, 2018 21:33

Copy link

palex-fpt commentedJun 8, 2018

There is one problem with PhpFilesAdapter - it does not use opcache till next request.

 $cache->set('key', 'value'); $cache->get('key'); // load directly from file, as file with timestamp >= request start time is not opcached.

nicolas-grekas force-pushed thecache-deprec-system-factory branch from8cf36c7 to19c30c9Compare

June 8, 2018 05:46

Copy link

MemberAuthor

nicolas-grekas commentedJun 8, 2018

There is one problem with PhpFilesAdapter - it does not use opcache till next request.

After having a deeper look, it does, but only if the file's mtime is in the past. This is now fixed. Thanks for the hint.

Copy link

palex-fpt commentedJun 8, 2018•
edited
Loading

to pass tests it should invalidate cache on file deletion (opcache_invalidate should be called before file is unlinked)

nicolas-grekas force-pushed thecache-deprec-system-factory branch from19c30c9 todf6dddbCompare

June 8, 2018 06:00

Copy link

MemberAuthor

nicolas-grekas commentedJun 8, 2018•
edited
Loading

to pass tests it should invalidate cache on file deletion (opcache_invalidate should be called before file is unlinked)

That would open race conditions (invalidating, then a concurrent request reloads the legacy file, then we write but this is ignored).~~Tests do pass so not sure what you meant~~ (hum, I missed the failures on FrameworkBundle, I'll have a look.)

nicolas-grekas force-pushed thecache-deprec-system-factory branch fromdf6dddb to647cfccCompare

June 8, 2018 06:13

Copy link

MemberAuthor

nicolas-grekas commentedJun 8, 2018

Hum, dunno why but tests pass locally and not the CI. I added the call you suggested but still red. Any idea why?

Copy link

palex-fpt commentedJun 8, 2018

calls to $cache->delete() or $cache->clear() unlinks files. but opcache is still has file cached. Andlist($expiresAt, $values[$id]) = include $file; returns data. Tests pass when opcache is not working (opcache_cli = false)

Copy link

palex-fpt commentedJun 8, 2018•
edited
Loading

Index: src/Symfony/Component/Cache/Traits/PhpFilesTrait.phpIDEA additional info:Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP<+>UTF-8===================================================================--- src/Symfony/Component/Cache/Traits/PhpFilesTrait.php(revision 3a2eb0d9514f0b322ab31a128aa48b9f89e28d50)+++ src/Symfony/Component/Cache/Traits/PhpFilesTrait.php(date 1528439846061)@@ -23,7 +23,9 @@  */ trait PhpFilesTrait {-    use FilesystemCommonTrait;+    use FilesystemCommonTrait {+        doUnlink as doFsUnlink;+    }      private $includeHandler;     private $zendDetectUnicode;@@ -155,4 +157,13 @@          return $ok;     }++    protected function doUnlink($file)+    {+        $allowCompile = 'cli' !== PHP_SAPI || ini_get('opcache.enable_cli');+        if ($allowCompile) {+            @opcache_invalidate($file, true);+        }+        return $this->doFsUnlink($file);+    } }Index: src/Symfony/Component/Cache/Traits/FilesystemCommonTrait.phpIDEA additional info:Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP<+>UTF-8===================================================================--- src/Symfony/Component/Cache/Traits/FilesystemCommonTrait.php(revision 3a2eb0d9514f0b322ab31a128aa48b9f89e28d50)+++ src/Symfony/Component/Cache/Traits/FilesystemCommonTrait.php(date 1528439709379)@@ -56,7 +56,7 @@         $ok = true;          foreach (new \RecursiveIteratorIterator(new \RecursiveDirectoryIterator($this->directory, \FilesystemIterator::SKIP_DOTS)) as $file) {-            $ok = ($file->isDir() || @unlink($file) || !file_exists($file)) && $ok;+            $ok = ($file->isDir() || @$this->doUnlink($file) || !file_exists($file)) && $ok;         }          return $ok;@@ -71,7 +71,7 @@          foreach ($ids as $id) {             $file = $this->getFile($id);-            $ok = (!file_exists($file) || @unlink($file) || !file_exists($file)) && $ok;+            $ok = (!file_exists($file) || @$this->doUnlink($file) || !file_exists($file)) && $ok;         }          return $ok;@@ -122,7 +122,12 @@             parent::__destruct();         }         if (null !== $this->tmp && file_exists($this->tmp)) {-            unlink($this->tmp);+            $this->doUnlink($this->tmp);         }     }++    protected function doUnlink($file)+    {+        return @unlink($file);+    } }

nicolas-grekas force-pushed thecache-deprec-system-factory branch from647cfcc to22bf5c3Compare

June 8, 2018 06:54

Copy link

MemberAuthor

nicolas-grekas commentedJun 8, 2018

Thanks, patch applied.

Copy link

palex-fpt commentedJun 8, 2018

opcache_invalidate should be called before unlink

Copy link

palex-fpt commentedJun 8, 2018

invalidate fails when file does not exits and opcache still contains file entry.

nicolas-grekas force-pushed thecache-deprec-system-factory branch 6 times, most recently fromd1f6da9 tocc1edabCompare

June 8, 2018 11:09

Copy link

MemberAuthor

nicolas-grekas commentedJun 8, 2018

Thanks, now green!

nicolas-grekas force-pushed thecache-deprec-system-factory branch fromcc1edab toce1a15dCompare

June 8, 2018 20:30

nicolas-grekas mentioned this pull request

Jun 8, 2018

[Cache] serialize objects using native arrays when possible#27543

Merged

nicolas-grekas commented

Jun 8, 2018

View reviewed changes

src/Symfony/Component/Cache/Adapter/PhpArrayAdapter.php

		{
		$this->file =$file;
		$this->pool =$fallbackPool;
		$this->zendDetectUnicode =ini_get('zend.detect_unicode');

Copy link

MemberAuthor

nicolas-grekasJun 8, 2018•
edited
Loading

Choose a reason for hiding this comment

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

No need to deal withzend.detect_unicode anymore whenthis line is removed. Let's remove the related complexity and overhead.

nicolas-grekas force-pushed thecache-deprec-system-factory branch fromce1a15d toc91b778Compare

June 8, 2018 22:53

fabpot approved these changes

Jun 11, 2018

View reviewed changes

src/Symfony/Component/Cache/Adapter/AbstractAdapter.php Outdated

		*
		* @return AdapterInterface
		*
		* @deprecated since Symfony 4.2.

Copy link

Member

fabpotJun 11, 2018

Choose a reason for hiding this comment

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

no dot at the end (I know, our conventions are hard to follow :)).

Copy link

MemberAuthor

nicolas-grekasJun 11, 2018

Choose a reason for hiding this comment

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

updated

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Jun 11, 2018

[Cache] Unconditionally use PhpFilesAdapter for system pools

51381e5

nicolas-grekas force-pushed thecache-deprec-system-factory branch fromc91b778 to51381e5Compare

June 11, 2018 07:58

Copy link

Member

fabpot commentedJun 11, 2018

Thank you@nicolas-grekas.

fabpot merged commit51381e5 intosymfony:master

Jun 11, 2018

fabpot added a commit that referenced this pull request

Jun 11, 2018

feature#27549[Cache] Unconditionally use PhpFilesAdapter for system…

d4f5d46

… pools (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------[Cache] Unconditionally use PhpFilesAdapter for system pools| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Now that we're about to leverage OPCache shared memory even for objects (see#27543), there's no reason anymore to use APCu for system caches. Let's remove some complexity here.As a bonus, this makes system caches pruneable using the `cache:pool:prune` command.Commits-------51381e5 [Cache] Unconditionally use PhpFilesAdapter for system pools

nicolas-grekas deleted the cache-deprec-system-factory branch

June 11, 2018 09:02

stof reviewed

Jun 11, 2018

View reviewed changes

src/Symfony/Component/Cache/Adapter/AbstractAdapter.php

		*/
		publicstaticfunctioncreateSystemCache($namespace,$defaultLifetime,$version,$directory,LoggerInterface$logger =null)
		{
		@trigger_error(sprintf('The "%s()" method is deprecated since Symfony 4.2.',__CLASS__),E_USER_DEPRECATED);

Copy link

Member

stofJun 11, 2018

Choose a reason for hiding this comment

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

shouldn't it be__METHOD__ ?

nicolas-grekas added a commit to nicolas-grekas/symfony that referenced this pull request

Oct 10, 2018

Revert "featuresymfony#27549[Cache] Unconditionally use PhpFilesAda…

dbc1230

…pter for system pools (nicolas-grekas)"This reverts commitd4f5d46, reversingchanges made to7e3b7b0.

nicolas-grekas mentioned this pull request

Oct 10, 2018

Revert "feature #27549 [Cache] Unconditionally use PhpFilesAdapter for system pools"#28805

Merged

fabpot added a commit that referenced this pull request

Oct 10, 2018

bug#28805Revert "feature#27549[Cache] Unconditionally use PhpFile…

9a37ba4

…sAdapter for system pools" (nicolas-grekas)This PR was merged into the 4.2-dev branch.Discussion----------Revert "feature#27549 [Cache] Unconditionally use PhpFilesAdapter for system pools"This reverts commitd4f5d46, reversingchanges made to7e3b7b0.| Q             | A| ------------- | ---| Branch?       | 4.2| Bug fix?      | yes| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | -| License       | MIT| Doc PR        | -Reading#28800, I've just realized that#27549 breaks using system caches with read-only filesystem.Using ApcuAdapter makes system caches compatible with read-only filesystems.Note that this affects only non-warmed up pools, as the warmed-up ones use a faster `PhpArrayAdapter` in front.Commits-------dbc1230 Revert "feature#27549 [Cache] Unconditionally use PhpFilesAdapter for system pools (nicolas-grekas)"