Skip to content

[FrameworkBundle] integrate the Cache component #18371

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

Merged
merged 7 commits into from
Apr 7, 2016

Conversation

nicolas-grekas
Copy link
Member

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.

@nicolas-grekas nicolas-grekas changed the title Issue 17537 [FrameworkBundle] integrate the Cache component Mar 30, 2016
<service id="cache.adapter.apcu" class="Symfony\Component\Cache\Adapter\ApcuAdapter" abstract="true">
<tag name="cache.pool" namespace-arg-index="0" default-lifetime-arg-index="1" />
<argument /> <!-- namespace -->
<argument /> <!-- default lifetime -->
Copy link
Contributor

Choose a reason for hiding this comment

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

In other services namespace is third argument but here is first. Maybe better change constructor signature for better consistency?

Copy link
Member Author

Choose a reason for hiding this comment

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

But default lifetime is always second...


<service id="cache.adapter.doctrine" class="Symfony\Component\Cache\Adapter\DoctrineAdapter" abstract="true">
<tag name="cache.pool" service-provider-arg-index="0" default-lifetime-arg-index="1" namespace-arg-index="2" />
<argument /> <!-- doctrine provider service -->
Copy link
Member

Choose a reason for hiding this comment

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

Doctrine

@xabbuh
Copy link
Member

xabbuh commented Mar 31, 2016

I am not convinced that people will need to create services starting with cache.adapter. to register their own adapters. This will prevent us from being able to add new adapters in the core in future releases as we would otherwise risk to break BC. I think we should, for now, restrict the configuration to the built-in adapters. We can then think of a clean way to let users register their own adapters for 3.2 (probably with another yet to be introduced DI tag).

Besides that I am 👍 on the current state of this feature.

@xabbuh
Copy link
Member

xabbuh commented Mar 31, 2016

ping @symfony/deciders

@nicolas-grekas
Copy link
Member Author

@xabbuh second review please :)

@xabbuh
Copy link
Member

xabbuh commented Mar 31, 2016

Not sure it's the best idea from the DX perspective when I have to pass the service name instead of a simple key, but I think we can live with that. :)

So to me this is ready to be merged.

ping @symfony/deciders

</service>

<service id="cache.adapter.doctrine" class="Symfony\Component\Cache\Adapter\DoctrineAdapter" abstract="true">
<tag name="cache.pool" service-provider-arg-index="0" default-lifetime-arg-index="1" namespace-arg-index="2" />
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need the default-lifetime-arg-index and namespace-arg-index parameters? They are so ugly 😭 and uncommon in Symfony codebase.

@fabpot
Copy link
Member

fabpot commented Apr 1, 2016

I'm really not a big fan of the arg-index thing.

@nicolas-grekas
Copy link
Member Author

nicolas-grekas commented Apr 1, 2016 via email

@HeahDude
Copy link
Contributor

HeahDude commented Apr 1, 2016

Since it should be documented maybe the "arg" part could be dropped.

@javiereguiluz
Copy link
Member

@nicolas-grekas the alternative would be to replace this handy argument resolver:

foreach ($tags[0] as $argName => $argValue) {
    if (isset($tags[0][$argName.'_arg_index']) && 0 <= $tags[0][$argName.'_arg_index']) {
        $pool->replaceArgument($tags[0][$argName.'_arg_index'], $argValue);
    }
}

And replace it with a switch ... case to treat the arguments of each cache type differently.

@HeahDude
Copy link
Contributor

HeahDude commented Apr 1, 2016

@javiereguiluz it's a very nice workaround to use the better name, but would default-lifetime="0" not be misleading ?

@xabbuh
Copy link
Member

xabbuh commented Apr 1, 2016

The drawback of that is that we wouldn't be able to support every kind of custom cache adapter a user might want to create, but we would stick to the built-in adapters instead.

@nicolas-grekas
Copy link
Member Author

This PR introduces two concepts:

  • cache pools: each cache pool is one single logical set of cache items of the same kind and in the same scope (e.g. one pool for caching SQL queries, another pool for caching html fragments, another one for caching parsed annotations, etc.). The kind of storage (apcu, redis, etc.) is a technical requirement but not a semantic requirement for the definition of a cache pool.
  • cache adapters: a cache adapter is well, a php class that implements a particular storage.

This PR provides a way to manipulate both concepts separately:

  • app authors need to define cache pools. This is easily done by using the semantic configuration so that one can configure e.g. cache.pools.foobar and that's it. By default, the concrete storage implementation will be the one implemented by the cache.adapter.default service, which is cache.adapter.filesystem by default. But the main idea is that when one creates cache.pools.foobar, it's a one-liner.

Then, devs can work on optimizing the cache storages. There are two ways to do so:

  • the first is to redefine the cache.adapter.default service/alias. Again, it's a one-liner in the service definition file to redefine it to e.g. cache.adapter.apcu instead of cache.adapter.filesystem. This way, all cache pools that have no explicit adapter configured switch to apcu storage instead of filesystem.
  • the second way is per cache pool, by explicitly configuring a cache adapter for each of them. Using semantic configuration, this is done with the "type" option. A "type" here, is the name of another cache pool, whose adapter configuration is reused (by service decoration). This allows grouping/factorizing adapters definitions.

On the service definitions side, a cache pool is any service that has the cache.pool tag.
A cache adapter is any service that has the cache.pool tag AND is abstract.
Concrete cache pools are usually defined as decorated cache adapter or cache pool definitions.
Thus, we need a way to map configuration parameters from concrete definitions to decorated definitions, and this is what cache.pool is used for: its attributes allow one to say: I want a cache pool that decorates the cache.adapter.apcu service with a default lifetime of 60s and a namespace of foobar. Then the *-arg-index attributes, defined at the abstract cache.adapter.apcu service definition level, allow mapping these namespace+default-lifetime to the correct constructor argument of the apcu service class.

In short: defining *-arg-index attributes won't be part of the daily work to do on a Symfony app. The list of cache storage adapter implementations is short. This PR provides a few, a bundle can provide more.
What people will do in there app is reuse these. They won't have to manipulate *-arg-index attributes. Still, *-arg-index attributes provides extensibility, which is an important target to me.

@nicolas-grekas
Copy link
Member Author

Btw, *-arg-index could be made useless if the DI component had a "parameterized abstract service definition decoration" feature.

@nicolas-grekas
Copy link
Member Author

I replaced *arg-index wiring by a conventional constructor arguments ordering.
Better DYT?

@javiereguiluz
Copy link
Member

@nicolas-grekas much better! Thanks.

@nicolas-grekas
Copy link
Member Author

Just added and wired a Psr6CacheClearer so that the e.g. cache:clear command clears all cache.pool tagged services.

new RemovePrivateAliasesPass(),
new RemoveAbstractDefinitionsPass(),
new ReplaceAliasByActualDefinitionPass(),
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like a bug fix to me, is it? Aliases should be resolved before removing private and abstract definitions, isn't it?

@javiereguiluz
Copy link
Member

@nicolas-grekas to check if this is optimized for the expected default behavior, please tell me what should a developer do by hand to achieve this:

  1. I have a server with APCu enabled.
  2. I execute symfony new my_project to create a 3.1-based project
  3. I want to boost the performance of my app. I want to cache anything that is cacheable: serializer, entities metadata, validator, property path, forms, etc. (If possible, third-party bundles too)

And now, what should that same developer do to achieve this:

  1. I've installed Redis in that same server and now I want to keep caching everything, but using Redis instead of APCu.

@nicolas-grekas
Copy link
Member Author

@javiereguiluz in fact our expectations are too low here: Cache integration should not be limited to pre-computed information about the app itself. We should also ease caching of cluster wide items. APCu is not suited for them, because APCu doesn't allow a shared cache pool amongst e.g. several fronts.
This means we should allow grouping cache pools by cache items scope: APCu is perfect for reflection-related info. Redis is a good fit for cluster wide items.
My recommendation would be to configure at least two main abstract pools. They would both default to a filesystem cache adapter, but one could then easily change the configuration of each of those abstract pools, effectively reconfiguring all concrete pools that rely on them.
The goal of this PR is not to do this configuration, but to provide the basis to work on it in a later PR (during the feature freeze 3.1).
As an example, the current state of this PR is to provide a single cache.pool.default, that is designed to store only app-related items.
Symfony-related things should go in other pools that we'll add later.
As you can read in the attached functional tests, changing all adapters at once is as easy as redefining the cache.adapter.default service (in one test, the default filesystem adapter is changed for a redis one).

@nicolas-grekas
Copy link
Member Author

The Cache component is now a mandatory dep of the framework bundle, and a cache.adapter.system is created that auto-selects apcu when available at compile time and is the adapter to use for any reflection-related cache pools.

@nicolas-grekas nicolas-grekas force-pushed the issue-17537 branch 3 times, most recently from 610d8fc to 0ec352f Compare April 6, 2016 20:28
@mvrhov
Copy link

mvrhov commented Apr 7, 2016

@nicolas-grekas if the apcu is automatic this will probably bring problems when deploys are made. e.g the deploy would require php-fpm or apache restart.

@nicolas-grekas
Copy link
Member Author

@mvrhov if you're referring to restarting for the purpose of cache clearing, this is already handled with a cache clearer, so that clearing the cache does not require any reboot but is automatic when the cache:clear command is called.
Or did you have something else in mind?

@mvrhov
Copy link

mvrhov commented Apr 7, 2016

I haven't looked at the driver implementation, but I'm pretty sure that in the case of apcu is not. Because apcu has a different cache on cli, in apache , when running via php-fpm each pool has a different cache.

To clear a apcu cache one of the following has to be made

  • http request [apache,php-fpm]
  • apache restarted,
  • php-fpm restarted,
  • direct fast-cgi request made to correct pool [php-fpm]

if ($pool->isAbstract()) {
continue;
}
if (!isset($tags[0]['namespace'])) {
Copy link
Member

Choose a reason for hiding this comment

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

Could it make sense to apply several cache.pool tags on the service ? If yes, we need to handle all tags, not only the first one.
If not, we may want to warn the user that subsequent tags are ignored to ease debugging

Copy link
Member Author

Choose a reason for hiding this comment

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

I doesn't make sense. We can add a warning, but unless I'm wrong no other compiler passes do that...

@nicolas-grekas nicolas-grekas force-pushed the issue-17537 branch 3 times, most recently from 4f82a62 to 950435d Compare April 7, 2016 11:08
@nicolas-grekas
Copy link
Member Author

Online & offline comments addressed (hello from Symfony Live Paris).
The config now defines two default cache adapters: cache.adapter.shared & cache.adapter.local,
and two cache pools for the user/dev side of the app only: cache.pool.shared & cache.pool.local.
We'll have to create symfony-related pools in other PRs.
I also reverted the auto-magic selecting apcu when avail: filesystem is now always the default, don't forget to change the config when you need something else. (e.g. cache.adapter.redis for cache.adapter.shared and cache.adapter.apcu for cache.adapter.local).

@mvrhov
Copy link

mvrhov commented Apr 7, 2016

@nicolas-grekas: even if you removed the apcu being default, the problem with it still persists. IMO this requires a fat warning message in the docs. Because probably most people are not aware of that problem.

@@ -309,6 +309,20 @@ public function testProcessMergeAutowiringTypes()
$this->assertEquals(array('Foo', 'Bar'), $def->getAutowiringTypes());
}

public function testProcessResolvesAliases()
Copy link
Member Author

Choose a reason for hiding this comment

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

@stof test case added

@fabpot
Copy link
Member

fabpot commented Apr 7, 2016

Thank you @nicolas-grekas.

@fabpot fabpot merged commit 4152634 into symfony:master Apr 7, 2016
fabpot added a commit that referenced this pull request 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
@nicolas-grekas nicolas-grekas deleted the issue-17537 branch April 7, 2016 15:38
@fabpot fabpot mentioned this pull request May 13, 2016
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.

9 participants