Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Default to Apcu+Filesystem cache chain#18715
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
dunglas commentedMay 5, 2016
👍 |
nicolas-grekas commentedMay 5, 2016
Includes#18716 until it's merged |
005e8d4 toe300496Compare| $apcu->setLogger($logger); | ||
| } | ||
| returnnewChainAdapter(array($apcu,$fs)); |
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.
Not sure if creation of the default adapter belongs here. Imo we should deal with this in the FrameworkBundle extension.
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.
factory moved to FrameworkBundle (I don't like creating/loading yet another utility class just for the factory)
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.
factory back on the component following comment from@fabpot
| privatefunctionregisterCacheConfiguration(array$config,ContainerBuilder$container,XmlFileLoader$loader) | ||
| { | ||
| $container->setParameter('kernel.cache_nonce',substr(base64_encode(md5(uniqid(mt_rand(),true),true)),0,4)); |
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.
Usingmt_rand means that each rebuilding of the container will change this value. And each server behind the load balancer may have a different value if they build their container themselves (rather than building it once before deploying), making it unusable for configuring a shared cache. Is it the intended behavior ?
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.
Yes, this is the indented behavior: this nonce is only for local caches (apcu is the only use case in fact today).
7ea44b2 to3a3e789Comparenicolas-grekas commentedMay 12, 2016
ping @symfony/deciders votes pending for 3.1-beta which should/may be released tomorrow |
| </service> | ||
| <serviceid="cache.adapter.system"class="Symfony\Component\Cache\Adapter\ChainAdapter"abstract="true"> | ||
| <factoryclass="Symfony\Bundle\FrameworkBundle\FrameworkBundle"method="createSystemCache" /> |
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 would move this factory to the component instead as the code in the method is not tied to the full-stack framework. I'd like to be able to reuse this strategy into Silex for instance.
| <tagname="cache.pool" /> | ||
| </service> | ||
| <serviceid="cache.adapter.system"class="Symfony\Component\Cache\Adapter\ChainAdapter"abstract="true"> |
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.
What about using the interface as the class here as the service can also just be aFilesystemAdapter?
xabbuh commentedMay 12, 2016
👍 |
5e3dc85 toae1d7dcComparefabpot commentedMay 13, 2016
Thank you@nicolas-grekas. |
…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
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.