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

[RFC] The new Cache component should be a centerpiece feature of Symfony #17537

Closed
javiereguiluz opened this issue Jan 26, 2016 · 23 comments
Closed
Labels
Cache RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@javiereguiluz
Copy link
Member

TL;DR of this issue:

  • Every Symfony component and bundle uses a centralized log service thanks to Monolog.
  • I want the same idea applied to the Cache component.

There is a lot of work around the new Cache component lately. But in my opinion, we are not talking about "the big picture" and how to make Cache a centerpiece feature for Symfony. This is how I imagine the Cache component being used in Symfony apps:

Defining application caches

When defining one cache, Symfony names it default automatically (same behavior as "swiftmailer" mailers and "doctrine" managers)

# app/config/config.yml

# simple cache without arguments
cache:
    type: array

# equivalent to:
# cache:
#    default:
#        type: array

Complex caches can define arguments:

# app/config/config.yml
cache:
    type: doctrine
    em: my_custom_em_id

You can also define lots of different caches in a single app (in this case is mandatory to name one of them default):

# app/config/config.yml
cache:
    default:
        type: array
    db:
        type: doctrine
        em: my_custom_em_id

They are available in the application as cache.default and cache.db services.
The cache service is an alias for cache.default.

Configuring the application caches

Params such as default_lifetime and namespace are available for any cache type:

# app/config/config.yml
cache:
    type: array
    default_lifetime: 600
    namespace: 'ACME__'

Using caches in Symfony components

All Symfony components define a cache option. Its value can be:

  1. A boolean, to enable or disable the cache. If true, the default cache is used:
# app/config/config.yml
framework:
    validation:
        cache: true
        enable_annotations: true
    serializer:
        cache: true
    property_path:
        cache: true

doctrine:
    orm:
        cache: true
# you can also set 'metadata_cache_driver', 'result_cache_driver' 
# and 'query_cache_driver' separately
  1. You can also set the id of the cache:
# app/config/config.yml
framework:
    validation:
        cache: db
        enable_annotations: true
    serializer:
        cache: default
    property_path:
        cache: default
  1. It's also possible to define an array of cache ids for complex services:
# app/config/config.yml
doctrine:
    orm:
        cache: [db, default]

This cache can also be used for third-party bundles:

# app/config/config.yml
liip_imagine:
    cache: true  # or false, or 'db', or ['db', 'default'], etc.

Thoughts? Comments? Other ideas?

@javiereguiluz javiereguiluz added RFC RFC = Request For Comments (proposals about features that you want to be discussed) Cache labels Jan 26, 2016
@mickaelandrieu
Copy link
Contributor

Hi @javiereguiluz,

  • agree for "Each bundle can be able to use the new Symfony Cache Component"
  • disagree for "Each component should depend on a new Symfony Cache Component"

To be honest I don't see why Symfony should provide his own implementation of Cache when a lot of good libraries already exists. Also, components are merged in organisation fast, maybe too fast ... look at Ldap component: tagged as internal and with no docs.

Regards,

@javiereguiluz
Copy link
Member Author

@mickaelandrieu thanks for your comments. About the dependency with this component: the default value for cache would be false, so no component/bundle would depend on the Cache component (only if you explicitly say so).

@stof
Copy link
Member

stof commented Jan 26, 2016

I don't like the idea of injecting a default cache shared by all services needing a cache. This would mean that each service needing a cache will need to build their cache key with their own unique prefix to ensure there is no conflict with caching done in totally unrelated parts of the app. This is a bad idea as it is far easier to create a mess (which would be undetected until the day you end up with a corrupted cache in prod creating weird bugs) than to get everything right.

And regarding the Doctrine case, I don't really agree with you, as Doctrine does not depend on a PSR-6 cache (and won't depend on it AFAIK). Thus, it has some cases with special needs which just cannot suit a typehint against PSR-6 (I'm thinking about the second level cache here for instance). And btw, Doctrine caches rely on being able to use the keys they want. Avoiding conflicts between different caches must be handled by the caches themselves when needed (through namespacing).

@javiereguiluz
Copy link
Member Author

@stof thanks for your comments ... although they go too deep in the code. The namespace problem would be easy to solve. Just append the name of the component (which is unique) to the namespace (e.g. if cache namespace = ACME__, then the namespace for validator is ACME__validator_, for serializer is ACME__serializer_, etc.)

@stof
Copy link
Member

stof commented Jan 26, 2016

@javiereguiluz but this would still mean you have to use different cache services for each place (configured with a different namespace). You cannot share the cache service between them. The namespace is part of the cache configuration, not of the cache usage.

@mickaelandrieu
Copy link
Contributor

@javiereguiluz I'm also interested on benchmarks, if someone in Symfony team is already working on this RFC :)

Regarding your proposal on configuration, use a boolean or a string to define cache key make sense. Maybe can we use cache and enable_cache ? I'm not easy with configuration key that can accept string or boolean.

Others ideas:

  • Add a new "Cache" section in WebProfiler so we can take a look at cache ids and human readable values (<3<3)
  • Be able to cache everything inside services, controllers, regardless the data and the cache driver (<3)

@javiereguiluz
Copy link
Member Author

@stof I'm not aware of the internal details. I just want to answer this question: how can a Symfony developer easily use a cache for all the services of the application?

@stof
Copy link
Member

stof commented Jan 26, 2016

@javiereguiluz you cannot easily use the same cache for all services (as you then have to make them all aware that they are not in control of their cache keys, which means you have to implement them in a specific way). Trying to make them use the same cache service is precisely the wrong thing in your proposal.

Regarding this issue, there are actually several distinct steps:

  • upgrading components to support PSR-6 for their cache
  • update bundles to provide an easy way to configure the PSR-6 caches. This is irrelevant if components are not yet using PSR-6 (even though we can start discussing how the config looks like).

Regarding the configuration, what we could imagine is having a cache configuration which would be used to configure templates for cache services. Then, any bundle needing a cache could allow users to specify the template to use, and would create a cache service based on it (with the only difference being the namespace used, where it would add a prefix or suffix to the namespace configured in the template). I don't know yet how this would look like (abstract services satisfy most of the needs of the template, except making it easy to prefix or suffix the namespace).
The config of a bundle could then look like that:

framework:
    validation:
        cache:
            enabled: true
            template: db
            id: ~ # use this instead of template to use your own PSR-6 service for full flexibility

The config normalization would allow the cache: true and cache: false shortcut (very easy as ->canBeEnabled() does it for us)

@sebastianblum
Copy link

I like the idea of an central cache because you can extend the cache component with an profiler and a cache tab in the symfony toolbar.

the last proposal with enabled: true and template is fine, please do not use cache: true and cache: db together because it is too much magic.

Because the cache component is a new feature, it will be released in symfony 3.1.
if you want to use it in bundles as a central cache, is it possible to merge already in 2.8 and 3.0?
that would make it more easier to use the cache in bundles.

@luispabon
Copy link
Contributor

This is a fine idea, symfony's current cache setup is byzantine and poorly documented.

@nicolas-grekas
Copy link
Member

I agree with @stof: having a single cache service makes no sense. Each code needing cache must have its own storage with tailored performance and items segregation.
I just proposed #17734 that adds a getStats method to cache adapters. It would allow building a data collector on top so that someone (@javiereguiluz ?) could build a profiler panel that gathers stats for all cache services (assuming they are e.g. all tagged with psr6).
This could be made to work with any PSR-6 implementation by wrapping the corresponding service definition in a ProxyAdapter object.
WDYT?

@javiereguiluz
Copy link
Member Author

having a single cache service makes no sense. Each code needing cache must have its own storage with tailored performance and items segregation.

I agree with that. The discussion is about: should the developer do everything by hand and result in a verbose config file ... or should Symfony framework help me with this?

I want to do this:

  • Install Redis in my server
  • Tell Symfony: use Redis cache for everything that can be cached!

And if someday I use Memcache, APCu or whatever, I want to tell Symfony: instead of Redis, use Memcache ... but keep caching everything.

@luispabon
Copy link
Contributor

Symfony could help by providing with an easy to configure, specific interface cache infrastructure. Bundles then can resort to that to implement their own caching strategies. It's the whole point of using a framework. At the moment there's a disconnect in between caching on the framework itself, doctrine and third party bundles.

@xabbuh
Copy link
Member

xabbuh commented Feb 9, 2016

What I find useful is if we make it possible to configure some abstract adapters like this:

cache:
    adapters:
        apc:
            type: apcu
            default_lifetime: 30
        tmp:
            type: filesystem
            directory: /tmp
        app_cache:
            type: filesystem
            directory: %kernel.root_dir%/../var/cache

Then in your own services you could inject a special cache service and use the cache tag to decide which cache adapter you want to have injected:

services:
    app.some_cool_service:
        class: AppBundle\SomeClass
        arguments:
            - '@cache'
        tags:
            - name: cache
              adapter: apc
              namespace: prefix

The namespace attribute would only be required for cache adapters that use a shared storage to avoid conflicts between cache keys. A compiler pass would ensure that concrete services will be created based on the configured adapter types and the attributes used for the cache tag.

In the core we could use a similar approach to enable caching for our components:

framework:
    validator:
        cache: apc

This configuration would automatically tag a service related to the Validator component.

@DavidBadura
Copy link
Contributor

i like @xabbuh proposal! 👍

@jakzal
Copy link
Contributor

jakzal commented Feb 10, 2016

@xabbuh I'm not fully convinced by the need of tagging.

Would be more straight forward if you could create cache services with the adapter configuration (although I'm not sure if adapters is a good config name here). The following configuration would register two services - apc and another.apc.cache:

cache:
    adapters:
        apc:
            type: apcu
            default_lifetime: 30
        another.apc.cache:
            type: apcu
            default_lifetime: 60

@xabbuh
Copy link
Member

xabbuh commented Feb 10, 2016

@jakzal The drawback is that you would then have to configure one adapter per service that gets injected the cache. Otherwise you would need to take care of the cache prefix/namespace in your custom code.

@jakzal
Copy link
Contributor

jakzal commented Feb 10, 2016

Yeah, that's a good thing I think. Tags would be a bit magic. Unless you read the code you don't know it creates a service behind the scenes. Configuring that service instead is more explicit.

@javiereguiluz
Copy link
Member Author

@xabbuh that's an implementation detail. Each cache service prepends a unique prefix silently. We already do that in some places. Example:

// Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
        if (isset($config['cache']) && $config['cache']) {
            $container->setParameter(
                'serializer.mapping.cache.prefix',
                // next line adds 'serializer_' prefix "automagically"
                'serializer_'.$this->getKernelRootHash($container)
            );

@nicolas-grekas
Copy link
Member

The service name is a good prefix candidate btw

@jvasseur
Copy link
Contributor

@xabbuh we could configure the components with a cache service and have each component decorate this service with a cache pool that handle the prefixing.

@xabbuh
Copy link
Member

xabbuh commented Feb 10, 2016

@javiereguiluz This approach does only work for code that we control in the core. But it does not make sense to let users do the same in their code when they just want to get a cache adapter injected.

However, another solution I see is to create a compiler pass that scans every definition for constructor and method arguments that are references and whose referenced service name is something like @cache.adapter_name and make the transition based on the name.

@xabbuh
Copy link
Member

xabbuh commented Feb 20, 2016

I created #17868 which integrates the Cache component into the FrameworkBundle. It's not finished but I would like to hear your thoughts on it.

fabpot added a commit that referenced this issue Mar 4, 2016
…as-grekas)

This PR was merged into the 3.1-dev branch.

Discussion
----------

[Cache] Count cache hits/misses in ProxyAdapter

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #17537 partially
| License       | MIT
| Doc PR        | -

I propose to add this subset of the `Doctrine\Common\Cache\Cache` interface so that we can build data collectors on top and show these stats in the web profiler.
ping @javiereguiluz

Commits
-------

e6f21f9 [Cache] Count cache hits/misses in ProxyAdapter
fabpot added a commit that referenced this issue Apr 7, 2016
…h, nicolas-grekas)

This PR was merged into the 3.1-dev branch.

Discussion
----------

[FrameworkBundle] integrate the Cache component

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

Last commit is the diff with #17868.

Commits
-------

4152634 [FrameworkBundle] Add default pool & system adapter
714b916 [FrameworkBundle] Add & use Psr6CacheClearer
4740c5c [FrameworkBundle] use abstract cache.pool decoration and aliases
92b1a20 [FrameworkBundle] Fix and add tests for cache pool wiring
e44bfdc [FrameworkBundle] Add cache-pool tag and wiring
281eafa [FrameworkBundle] Integrate the Cache component
bc51fde [Cache] Normalize constructor arguments order
@fabpot fabpot closed this as completed Apr 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cache RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests