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

Prefer PhpFileCache for caching docs #7500

Closed
wants to merge 1 commit into from
Closed

Prefer PhpFileCache for caching docs #7500

wants to merge 1 commit into from

Conversation

shadowhand
Copy link
Contributor

Removes APC and XCache because they are deprecated.

Remvoes APCu because it is not as performant as PhpFileCache and cannot
be cleared using using console tools.

Refs #7493 (review)

@greg0ire
Copy link
Member

greg0ire commented Dec 2, 2018

Please amend your commit message:

- Remvoes APCu because it is not as performant as PhpFileCache and cannot
- be cleared using using console tools.
+ Removes APCu because it is not as performant as PhpFileCache and cannot
+ be cleared using console tools.

Also, what do you mean by "docs"?

Prefer PhpFileCache for caching docs

EDIT: at first, I thought you mean "for caching phpdoc", meaning annotations, now I get it.

docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
docs/en/reference/caching.rst Outdated Show resolved Hide resolved
@shadowhand
Copy link
Contributor Author

@greg0ire updated.

greg0ire
greg0ire previously approved these changes Dec 2, 2018
@greg0ire
Copy link
Member

greg0ire commented Dec 2, 2018

Please amend your commit message:

- be cleared using using console tools.
+ be cleared using console tools.

Removes APC and XCache because they are deprecated.

Removes APCu because it is not as performant as PhpFileCache and cannot
be cleared using console tools.
@shadowhand
Copy link
Contributor Author

@greg0ire fixed.

@Majkl578
Copy link
Contributor

Majkl578 commented Dec 3, 2018

Just like APCu, OPCache can't be cleared from CLI either. So in the end this isn't any improvement.

@shadowhand
Copy link
Contributor Author

shadowhand commented Dec 3, 2018

@Majkl578 clearing PhpFileCache means removing files, and opcache detects the removal of files. updating the namespace version, so the requested cache files will no longer be found.

Edited, see #7500 (comment)

@Majkl578
Copy link
Contributor

Majkl578 commented Dec 3, 2018

opcache detects the removal of files

Only if configured to do so, and this setting should be disabled on production.

Copy link
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

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

As per my comment above, one can of worms is being replaced by another can of worms while the original issue remains.
I support getting rid of APCu, but this is not the correct way.

@Ocramius
Copy link
Member

Ocramius commented Dec 5, 2018 via email

@lcobucci
Copy link
Member

lcobucci commented Dec 5, 2018

It's quite important to remember that our cache providers don't remove anything on removeAll() but simply bump up the namespace version: https://github.com/doctrine/cache/blob/36c114783b9f30abfd1054e59c28e6e2e8152481/lib/Doctrine/Common/Cache/CacheProvider.php#L153-L165

What @Majkl578 said is really important, the recommended opcache production setup ® doesn't track file system updates (which is what makes opcaching truly remarkable), therefore you can refresh stuff and the app won't get updated unless the deployment is done in a separated environment (phoenix servers, containers, etc) - or reseting opcache (which is possibly done when reloading the web service workers).

@shadowhand
Copy link
Contributor Author

shadowhand commented Dec 5, 2018

@lcobucci since the there is no filesystem change being done, will running apps see the cache clear, even when the server is not restarted? If so, that would invalidate @Majkl578's concern. I don't understand the subtleties of opcache internals.

@lcobucci
Copy link
Member

lcobucci commented Dec 5, 2018

@shadowhand let's remove opcache from equation for one sec. When we do the PhpFileCache#removeAll(), we get the file that stores the namespace version and write the new namespace version to it. That's how the upcoming commands/requests know which version to use.

For production, opcache usually is configured like this (apart from other ones):

opcache.validate_timestamps=0
opcache.enable_file_override=1

The comments of the first one give us the answer to your question:

; When disabled, you must reset the OPcache manually or restart the
; webserver for changes to the filesystem to take effect.

So, no, namespace version change won't be propagated unless opcache is cleared manually or via webserver restart.

@shadowhand
Copy link
Contributor Author

@lcobucci thanks.

@Majkl578 given that your concern is about how opcache might be configured, how do you suggest that this PR be changed to address that?

@lcobucci
Copy link
Member

@shadowhand sorry about the huge delay here... I'd say that it's fine to use file system cache for annotations, query, and metadata cache but for result set and L2C I'd rather suggest Redis.

@lcobucci lcobucci modified the milestones: 2.6.4, 2.6.5 Sep 23, 2019
Copy link
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

I would also not remove APCu and XCache from the docs because they're also helpful scenarios where it's just fine to use them (containerised apps or blue-green deployments).

<?php
$cacheDriver = new \Doctrine\Common\Cache\ApcCache();
$cacheDriver->save('cache_id', 'my_data');
PhpFileCache
Copy link
Member

Choose a reason for hiding this comment

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

I think the best way for this is to have another header grouping distributed (redis, memcached, etc) and non-distributed (filesystem, apcu, etc) providers. Stating that non-distributed ones are completely fine for query and metadata caches whereas distributed ones are better suited for result set and second-level caches.

APCu
~~~~
The preferred cache driver is ``PhpFileCache``. This driver serializes
cache items and writes them to a file. This allows for opcode caching
Copy link
Member

Choose a reason for hiding this comment

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

It would be really nice if we mention the impact of opcache configuration and what are the architectural consequences of them. Or maybe point to an external resource with that info.

Copy link
Member

Choose a reason for hiding this comment

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

Because an external resource might disappear, that short mention of the impact would be nice as an extra.

@lcobucci lcobucci removed this from the 2.6.5 milestone Nov 15, 2019
@lcobucci lcobucci added this to the 2.7.1 milestone Nov 15, 2019
@lcobucci
Copy link
Member

@shadowhand thanks a lot for the initial write-up. I hope my suggestions make sense to you and help us move this further.

We're working to finalise the 2.6.x minor and since this is not a really urgent bug fix I moved it to 2.7.1 so that we have time to discuss and organise this well.

Thanks again for your contribution 👍

@@ -304,8 +270,11 @@ use on your ORM configuration.
.. code-block:: php

<?php
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCache(

@@ -318,7 +287,11 @@ cache implementation.
.. code-block:: php

<?php
$config->setResultCacheImpl(new \Doctrine\Common\Cache\ApcuCache());
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCache(

@@ -335,7 +308,11 @@ result cache driver.
.. code-block:: php

<?php
$query->setResultCacheDriver(new \Doctrine\Common\Cache\ApcuCache());
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCache(

@@ -387,7 +364,11 @@ first.
.. code-block:: php

<?php
$config->setMetadataCacheImpl(new \Doctrine\Common\Cache\ApcuCache());
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCacheCache(
$cacheDriver = new \Doctrine\Common\Cache\PhpFileCache(

APCu
~~~~
The preferred cache driver is ``PhpFileCache``. This driver serializes
cache items and writes them to a file. This allows for opcode caching
Copy link
Member

Choose a reason for hiding this comment

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

Because an external resource might disappear, that short mention of the impact would be nice as an extra.

@beberlei
Copy link
Member

beberlei commented Jan 8, 2020

Merged in 20c4603

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants