-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[PropertyAccess] Add PSR-6 cache #16838
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
Conversation
c0e70a7
to
9e38156
Compare
@@ -401,7 +410,7 @@ private function getReadAccessInfo($object, $property) | |||
|
|||
if (isset($this->readPropertyCache[$key])) { | |||
$access = $this->readPropertyCache[$key]; | |||
} else { | |||
} elseif (!$this->cache || !$access = $this->cache->fetch(self::CACHE_PREFIX_READ.$key)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid that it wouldn't work for ACCESS_TYPE_METHOD
case, because constant has value 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch it should be fixed now.
8f972f9
to
7108cc1
Compare
@@ -504,6 +504,7 @@ private function addPropertyAccessSection(ArrayNodeDefinition $rootNode) | |||
->children() | |||
->booleanNode('magic_call')->defaultFalse()->end() | |||
->booleanNode('throw_exception_on_invalid_index')->defaultFalse()->end() | |||
->scalarNode('cache')->end() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would be fine provide more information, like ->info('Service name used for cache')->example('property_accessor.cache.apc')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the example should not be included IMO as the examplified service doesn't exist. but info is indeed a good idea
IMO when a doctrine cache is injected, the internal array cache should not be used. If I already inject an in-memory cache, I probably don't want to have it cached twice in memory. Also doctrine already has an |
@Tobion class property could save 2 methods call: |
@Tobion if we go that way, we can just remove totally the local cache array and rely only on Doctrine Cache to enable this performance optimization. |
Yes relying fully on doctrine cache is also possible and the most flexible and the most explicit. The problem with an internal array cache like now is that it is magic from the outside. And it doesn't really work well together with another cache layer like doctrine. |
7108cc1
to
6f13bf5
Compare
Guys, please measure performance before and after removing internal array cache, maybe using blackfire profile. There is enough funnction calls to retrieve values from cache: |
3e8a560
to
82e363f
Compare
Here is a benchmark (tldr use the internal cache, not Doctrine ArrayCache). The comparison: https://blackfire.io/profiles/compare/3a1b8a29-125e-4f19-b5ba-ff0785c082fe/graph It's more than 30% faster and consume less memory to use the internal cache than relying on the DoctrineChain. I remove this commit. With Doctrine Cache only (internal array removed and chained ArrayCache and ApcCache): <?php
require 'vendor/autoload.php';
require 'Foo.php';
use Doctrine\Common\Cache\ArrayCache;
use Doctrine\Common\Cache\ApcCache;
use Doctrine\Common\Cache\ChainCache;
use Symfony\Component\PropertyAccess\PropertyAccess;
$cache = new ChainCache([new ArrayCache(), new ApcCache()]);
$accessor = new PropertyAccessor(false, false, $cache);
echo 'Property Access Benchmark'.PHP_EOL;
$start = microtime(true);
for ($i = 0; $i < 10000; ++$i) {
$foo = new Foo();
$accessor->setValue($foo, 'bar', 'Lorem');
$accessor->setValue($foo, 'baz', 'Ipsum');
$accessor->getValue($foo, 'bar');
$accessor->getValue($foo, 'baz');
}
echo 'Time: '.(microtime(true) - $start).PHP_EOL; With internal array and Doctrine ApcCache: <?php
require 'vendor/autoload.php';
require 'Foo.php';
use Doctrine\Common\Cache\ApcCache;
use Symfony\Component\PropertyAccess\PropertyAccess;
$cache = new ApcCache();
$accessor = new PropertyAccessor(false, false, $cache);
echo 'Property Access Benchmark'.PHP_EOL;
$start = microtime(true);
for ($i = 0; $i < 10000; ++$i) {
$foo = new Foo();
$accessor->setValue($foo, 'bar', 'Lorem');
$accessor->setValue($foo, 'baz', 'Ipsum');
$accessor->getValue($foo, 'bar');
$accessor->getValue($foo, 'baz');
}
echo 'Time: '.(microtime(true) - $start).PHP_EOL; |
82e363f
to
42ccf65
Compare
I've just added support for 35% of performance improvement just for this specific optim: https://blackfire.io/profiles/compare/4e5545b5-7537-4502-851b-baf65ad2f2e4/graph @stof you pointed out on the previous PR that |
How is invalidation of the cache handled? Does the user have to clear his cache manually each time an object in the property path is changed as this can change the access strategy? |
I'm not sure I understand your point. It caches the structure of classes and properties so the only time cache must be flushed it's when the code is modified (and it's almost always the time). |
User should use same invalidation method like for apc class loader or validator metadata cache: clear cache after deploy. @dunglas nice results 👍 |
So the recommended way is to only use caching on the property accessor in production mode I assume? Would be worth to mention that in the phpdoc I think. |
@Tobion yes it's only for production but it's already what we suggest for other similar caches. Using the standard edition, you just need to install APC and to uncomment these lines https://github.com/symfony/symfony-standard/blob/master/app/config/config_prod.yml#L4-L14 and you get good performance for free. It's also why I think we need to keep APC services (it's user friendly). |
Travis failures look unrelated. |
There is a "drawback": it's not possible to disable the ApcAdapter if apcu is installed and enabled. Is it acceptable? |
@dunglas: I'm not comfortable with apc being default. The cache cannot be cleared from the console this means that after each deploy the php-fpm/apache should be restarted, ... |
@dunglas can you try temporary disable psr6-caching inside |
…in (nicolas-grekas) This PR was merged into the 3.1-dev branch. Discussion ---------- [FrameworkBundle] Default to Apcu+Filesystem cache chain | Q | A | ------------- | --- | Branch? | 3.1 | Bug fix? | yes (perf) | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - In #16838, @dunglas benched that the filesystem cache is not the fastest one (not a surprise). Yet, it is the default for now. I propose to replace this default by a dynamically created one that uses APCu when available (of course, we cannot do the check at container-build time), chained with the filesystem. The benefit is double: APCu is automatically used when available without any configuration, and cache warming up is seeded by the filesystem cache so that the apcu cache can benefit from it. Commits ------- b9b57f9 [FrameworkBundle] Default to Apcu+Filesystem cache chain
1f469a1
to
1b4b8fd
Compare
👍, please squash my commit :-) |
f4bcdab
to
4d31722
Compare
@symfony/deciders can I merge this PR please (one +1 missing)? |
public static function createCache($namespace, $defaultLifetime, $version, LoggerInterface $logger = null) | ||
{ | ||
if (!class_exists('Symfony\Component\Cache\Adapter\ApcuAdapter')) { | ||
throw new \RuntimeException(sprintf('The Cache Component must be installed to use the "%s::%s" factory.', __CLASS__, __METHOD__)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sprintf('The Symfony Cache component must be installed to use %s().', __METHOD__)
👍 |
Comments from @nicolas-grekas @fabpot fixed. Can be merged. |
Thank you @dunglas. |
This PR was squashed before being merged into the 3.2-dev branch (closes #16838). Discussion ---------- [PropertyAccess] Add PSR-6 cache | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | n/a Follow #16294 Commits ------- 4ccabcd [PropertyAccess] Add PSR-6 cache
PropertyAccesor was added to Pool in order to reuse it for performance reasons in sonata-project#3488 In symfony/symfony#16838 cache was added to the component, so there is no need to reuse it thought the Pool service and it can be injected into the services.
PropertyAccesor was added to Pool in order to reuse it for performance reasons in sonata-project#3488 In symfony/symfony#16838 cache was added to the component, so there is no need to reuse it thought the Pool service and it can be injected into the services.
PropertyAccesor was added to Pool in order to reuse it for performance reasons in sonata-project#3488 In symfony/symfony#16838 cache was added to the component, so there is no need to reuse it thought the Pool service and it can be injected into the services.
PropertyAccesor was added to Pool in order to reuse it for performance reasons in sonata-project#3488 In symfony/symfony#16838 cache was added to the component, so there is no need to reuse it thought the Pool service and it can be injected into the services.
PropertyAccesor was added to Pool in order to reuse it for performance reasons in sonata-project#3488 In symfony/symfony#16838 cache was added to the component, so there is no need to reuse it thought the Pool service and it can be injected into the services.
PropertyAccesor was added to Pool in order to reuse it for performance reasons in #3488 In symfony/symfony#16838 cache was added to the component, so there is no need to reuse it thought the Pool service and it can be injected into the services.
Follow #16294