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

[2.2][WIP] Cache Component #3211

Closed
wants to merge 3 commits into from
Closed

Conversation

beberlei
Copy link
Contributor

Bug fix: no
Feature addition: yes
Backwards compatibility break: no
Symfony2 tests pass: no (!)
Fixes the following tickets: GH-1513

This addresses GH-1513, implementing a Caching interface as new Symfony Component.

It also ships a very simple ApcCache implementation, however the focus should be on just having the CacheInterface and let others provide more complex implementations and management. With the interface however everyone can rely on a common API in their Symfony bundles and applications.

As an example a cache delegate implementation has been provided for both Validator and ClassLoader components.

This is just the first step. A second step would be integration into FrameworkBundle with a section:

framework:
  cache:
    name: 
       provider: symfony.cache.apc

Would create a service "cache.name".

Integration points would be the provider (expecting a service id) and an options key, so we could have:

framework:
  cache:
    name: 
       provider: doctrine.common.cache.apc
       options:
          prefix: my_cache_prefix

The first cache provider in the list should also be aliased to "cache", so that all bundles can always trust on "cache" being available as service.

…ide integrations for Validator and ClassLoader
* use Symfony\Component\ClassLoader\ApcUniversalClassLoader;
* use Symfony\Component\Cache\ApcCache;
*
* $apc = new ApcCache();
Copy link
Member

Choose a reason for hiding this comment

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

this implies requiring the cache implementation and the interface first

@stof
Copy link
Member

stof commented Jan 29, 2012

you should also add an implementation in the bridge using Doctrine Common to implement the Symfony interface

@beberlei
Copy link
Contributor Author

Tests dont run because of APC redefined, only when CacheUniversalLoaderTEst is run in isolation. I copied the test 100%.

Question would be if we have this Cache component, we could drop the ApcUniversalClassLoader.

@beberlei
Copy link
Contributor Author

@stof yes, i wanted to wait for the general perception of this PR before going further.

* @param string $id cache id The id of the cache entry to fetch.
* @return string The cached data or FALSE, if no cache entry exists for the given id.
*/
function fetch($id);

Choose a reason for hiding this comment

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

I disagree with the idea of returning false if not found: what if data is boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What if data is NULL? same problem, fetch($id, $found) is not supported by all cache drivers though.

Thats what ->contains is for. The problem here is that we have to find the most common denominator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exceptions make sense here imo

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @alexandresalome disagreeing here, the first requirement should be the ability to store any type of value (this is one of the PR I have submitted against Doctrine common.)

So the interface could look like:

<?php
interface CacheInterface
{
    function fetch($id, &$success);
    function contains($id);
    function save($id, $data, $lifeTime = 0);
    function delete($id);
}

I think there is one more problem with providing an interface with both fetch and contains as it encourages flawed code as

<?php
if  (!$cache->contains('id')) {
   $cache->save('id', compute());
}
doSomething($cache->fetch('id'));

So what about dropping contains ?

<?php
interface CacheInterface
{
    function fetch($id, &$value, $fetchValue = true);
    function save($id, $data, $lifeTime = 0);
    function delete($id);
}
  • fetch would returns true when the value is in cache,
  • the third parameter could be set to false when you really need the contains behavior

The above example would become:

<?php
if  (!$cache->fetch('id', $value)) {
   $cache->save('id', $value = compute());
}
doSomething($value);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neither Memcache, Xcache nor Zends Cache support an API for checking if a key really exists or not. (Memcached does support it). An API that works by reference for the values is really unintuitive, an API interface should take expectations of users into account.

Throwing an exception is misusing exceptions as a control-flow and will lead to ugly code in practice. I guess fetch($id, &$found) does make sense and could get rid of contains(), but personally saving false/null/emptystring into a cache entry screams for trouble anyways.

Copy link
Member

Choose a reason for hiding this comment

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

@rande see the discussion page as things like that have already been suggested. And IMO, users should not have to bother with an extra object wrapping the data to be able to store them.

Copy link

Choose a reason for hiding this comment

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

@vicb

I commented in the pull request and it's somewhat relevant to this. The basic idea in the request was that I agreed with @stof that we should consider the CacheInterface and the DriverInterface as separate pieces. The reason I bring this up is that my answer depends on the approach.

  • For the CacheInterface I think returning a null is critical. The users should be able to put any object they want in (null, false, objects, etc) and pull them out again directly- they shouldn't have to do type checking to confirm that their object is what they expect it to be. Additionally there is a huge difference between a "miss" and the object not existing- if the object is not there it's a miss, but there are cases where there will be an object and it will still be a miss.
  • For the DriverInterface I feel a CacheElement (or something) should be returned for every request, because the higher level caching library is going to need to know various pieces of metadata such as the TTL. The users (doctrine, etc) will never have to interact with the DriverInterface.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tedivm

  • I agree, my question was about the "DriverInterface". One question though, could you eplain: "there are cases where there will be an object and it will still be a miss" ?
  • You assume that the TTL is global for all the items here, right ? If this is the case we might be able to retrieve it from a different place. I am not saying that always returning an object is a bad, just trying to understand.

Copy link

Choose a reason for hiding this comment

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

I agree, my question was about the "DriverInterface". One question though, could you eplain: "there are cases where there will be an object and it will still be a miss" ?

On object can be present and stale, at which point it would be considered a miss even though it still exists. If you let the higher level caching layer that the drives feed into make the decisions about what to do with stale data (reuse it, discard it, etc) then you have a very powerful tool. Stash has some examples of this.

You assume that the TTL is global for all the items here, right ? If this is the case we might be able to retrieve it from a different place. I am not saying that always returning an object is a bad, just trying to understand.

To be clear, the TTL I'm referring to is the remaining TTL for that specific object. If the start TTL was 180 seconds, but only 20 seconds remain to expire, then it would be very useful to know that. Additionally it may be useful to know that it expired 120 minutes ago- using a datetime (or timestamp) internally for that expire date is important.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your first reply also answered my second question. That's a very interesting discussion, many thanks for you inputs ! I really need to find some time to take a deeper look at Stash

@lsmith77
Copy link
Contributor

@Seldaek
Copy link
Member

Seldaek commented Jan 30, 2012

@lsmith77: +1 on using the proposed PSR cache interface.. and to adjust to any changes it has until 2.1 is released. We should really push for finalization of it before 2.1, that way we can join the party.

@lstrojny
Copy link
Contributor

btw: for caching in "the real world" [tm] cache tagging is a feature one cannot ignore. It can be implemented for every backend source in a more or less complicated fashion. Can our interfaces at least allow for tags to be specified during on save(). Additionally a deleteTags(array $tags) method should be available.

@alexandresalome
Copy link

Concerning details of "what a cache abstraction" should provide, the discussion on the newsgroup, provided by @lsmith77 seems great.

The point is that since each component implements the cache on its own, performance is great.

The more abstraction you add between the needing and the solution, the less performance you have.

Keeping cache interfaces is important in component.

The best would probably be to create a PSR Bridge, isnt it?

@beberlei
Copy link
Contributor Author

@lstrojny tags are an important use-case, however implementations that hack this into backends do that at huge costs. Uses don't recognize that implementing tags with memcache/apc requires you to cache all keys in a key. This causes tons of race-conditions on high load.

@vicb
Copy link
Contributor

vicb commented Jan 31, 2012

@beberlei, @lstrojny Would it be possible to provide a basic CacheInterface and some other interfaces as TaggableInterface, XYZInterface with decorators for cache system where those features are not natively supported ?

@lstrojny
Copy link
Contributor

@vicb yep, that would be fine.

@fabpot
Copy link
Member

fabpot commented Feb 11, 2012

I also think that this is quite important to have "something" in core. How can we move forward?

@dlsniper
Copy link
Contributor

If this component will get more development on it Symfony2 should also support passing the components here as either services or something similar (think singletons) as one might bump into 'too many connection' problems if you need to use same type of service, say memcache, everywhere in the application.

The problem that I'm trying to solve is this:

  • one memcache connection for session
  • one memcache connection for DB cache
  • one memcache connection for Symfony Cache component
    all tied to the same server. Now, using the above approach, I've just limited the number of users that I can support on a single server because instead of reusing a existing connection I'm using 3 connections.

I've had this problem already into production where I had simply too many open connections for our memcache server pool and the way to solve it was to merge everything into a single central service.

I'm thinking something like this for example:

framework:
    session:
        storage_connection:   symfony.cache.memcached

doctrine:
    orm:
        entity_managers:
            default:
                result_cache_driver:
                    connection:  symfony.cache.memcached

I don't know if Doctrine would be able to support something like this, but maybe it could considered or I'm just talking non-sense. (I know connection is not currently supported in Doctrine configuration nor storage_connection exists in Symfony2).

Hope all of the above makes sense to you guys.

/Cheers

@stof
Copy link
Member

stof commented Feb 12, 2012

@dlsniper We first need to figure how the cache component should look like before configuring it. Providing a few services is not an issue at all once we have the code.

@vicb
Copy link
Contributor

vicb commented Feb 12, 2012

@dlsniper I'd like to come with something like this in the CacheBundle (#3225). I hope I can find time on work on this during the coming week.

@dlsniper
Copy link
Contributor

@stof I've added some of the changes in the above commit and I think I could do more to it but seems people are not sure about the right direction for now. IMHO the cache components should all provide the same functions and capabilities by default, using the cache interface, but for example, if one wants to use multi-fetch option from memcache(d) library, one should have access to it by calling the function, getMultiByKey, directly. I mean, if I configure the application to use Memcache(d) I'll know for sure which things I'm allowed to call on a memcache(d) object, don't I?

@vicb I could try and help you with this as well if you want me to. I can do some work on this as I'm fairly interested in getting this into Symfony2 master sooner rather that later.

@Seldaek
Copy link
Member

Seldaek commented Feb 12, 2012

I would like to reiterate that PLEASE take a look at the proposed PSR cache interface and PLEASE make this PR compatible as much as possible. It may well lead nowhere, but at least it has a lot of feedback built in already from many different projects.

@vicb
Copy link
Contributor

vicb commented Feb 12, 2012

@dlsniper Any help is welcomed, I'll try to update the PR in the coming days with code a detailed todo list (It will be based on doctrine common until this cmp gets ready.)

@stof
Copy link
Member

stof commented Feb 12, 2012

@dlsniper you did not added anything. You only referenced the PR from your fork (not even yours in fact by a fork from @dlsniper2). To be included in the PR, the commit needs to be part of @beberlei's branch as the PR is sent from his branch

@dlsniper
Copy link
Contributor

@Seldaek I'll update the code in the next hours to support that, it's part two of the work. Thanks for the link
@stof I know, I'm working on a final PR with @Seldaek suggestion as well

@vicb
Copy link
Contributor

vicb commented Feb 12, 2012

@Seldaek thanks for the link. The one thing I don't like there is get() with a single argument and the exists() method which encourage writing flawed code (race conditions). I'll try to take some time to go through the mail thread and submit feedback.

@Seldaek
Copy link
Member

Seldaek commented Feb 12, 2012

@vicb thanks. I know it's not perfect yet, but I would rather have us contribute to the effort than going our separate way.

@lstrojny
Copy link
Contributor

@ALL: I think it is a good idea to consider existing projects as blessed cache component for Symfony. My concerns with Stash would be:

  • Static Handlers and Utilities but, more problematic, the static Manager class
  • PSR-0
  • Coding style (whitespace, e.g. Exceptions not being named *Exception)
  • A lot of little quirks here and there that I'm sure can be solved

If we can overcome those things, I think it would be a great foundation.

@tedivm
Copy link

tedivm commented Feb 17, 2012

Static Handlers and Utilities but, more problematic, the static Manager class

The only static function in any of the handlers is the "canEnable" function, which can be used to make sure extensions are installed and whatnot. There is a static in-script cache that is very, very simple to disable but has some nice performance benefits (and I don't think should cause any issues, but would be happy to explore further).

The Manager and Box classes are not meant to be used in Frameworks like Symfony- they're basically example containers that people can throw into existing or quick applications. A better example for how to handle it in Symfony (static free) is the Cache Service in our bundle.

As to the utilities, I've actually been eliminating them over time. At this point most of them can be moved right into the filesystem handler itself, since it's the one that uses most. Others we can ditch if we moved to 5.3 (something I'm strongly considering).

PSR-0

This isn't a huge change for us- we're already structured in the same way, we're just lacking underscores. The big reason I haven't is because I was debating whether to go with full namespace support but was weary of dropping 5.2 support. There are other reasons why that's appealing

Coding style (whitespace, e.g. Exceptions not being named *Exception)

Making the Exception name changes shouldn't be a problem, and I'll make sure the bundle follows all the standards (I need to read through them again before I commit to any style changes on Stash, but I don't think that'll be an issue either). If there is anything else specific that you think should be addressed just let me know.

A lot of little quirks here and there that I'm sure can be solved

Yeah, just keep that list coming!

@lstrojny
Copy link
Contributor

I would personally be in favor of dropping 5.2 support, as 5.4 knocks already on the door and we should not support outdated PHP versions. Dropping 5.2 support allows you to use closures as well. But before going more into details I think we should make a decision if we want to go with Stash. It wouldn’t be that fair to ask you for a number of changes and than deciding not to do it.

@tedivm
Copy link

tedivm commented Feb 17, 2012

Eh, so far all of the changes you've mentioned are either things I've already intended to do or just make sense. Also, regardless of whether Stash is "the" caching library or not I do plan on fully supporting Symfony ( @jhallbachner and I use it quite a bit), so it won't be wasted effort. Really, my worst case scenario is that I put a bunch of work in to make my library better- not exactly something I'm worried about.

@lstrojny
Copy link
Contributor

OK, fair enough :)

@tedivm
Copy link

tedivm commented Feb 19, 2012

Alright, took the plunge and Stash now supports PSR-0 (at least, it passes all unit tests). The Symfony bundle hasn't been updated to reflect that yet, as there are some other changes I want to make as well first. The documentation still shows the old API (which I tagged right before making these changes, so you can still play around with that if it's easier).

@stof
Copy link
Member

stof commented Feb 28, 2012

@tedivm I don't see the need of defining an interface extending Iterator without changing anything. Simply use Iterator.

1 similar comment
@stof
Copy link
Member

stof commented Feb 28, 2012

@tedivm I don't see the need of defining an interface extending Iterator without changing anything. Simply use Iterator.

@tedivm
Copy link

tedivm commented Feb 28, 2012

@stof- The document is a work in progress (not a final draft), which is why I haven't passed it around too far yet (still soliciting feedback). With the Cache\Iterator I had originally thought I would do a little more with it, and then realized that was probably a mistake. Part of me still feels it should exist, since unlike an Iterator it is expected to only return Cache\Item objects, but I admit that's probably weak reasoning.

As I mentioned, this is a work in progress- I appreciate any feedback on this. If people want to open issues or send PR's along I'd be thrilled. At the moment I haven't sent it along to the PSR group themselves, as I wanted to make sure any obvious issues got cleared up.

On a separate note, Stash has been updated to address many of the issues brought up (coding conventions, static functionality, exception naming, PSR-0)- mainly due to a lot of hard work by @kbond, @vicb, and @lstrojny. It's also been integrated with travis-ci (which is so cool, and yet another reasons I should have moved to github awhile ago) and supports Composer. If there are any other things that would make integration with Symfony easier please let me know- I'm very receptive to suggestions.

@tedivm
Copy link

tedivm commented Feb 29, 2012

Since what you all saw was more of a draft than a finished or polished document, I thought I'd mention that I've updated it quite a bit today (including taking your advice, @stof, about using the Iterator directly).

I'm also curious about whether this proposal makes sense for this discussion. The basic interfaces (as well as some of the extensions) accomplish pretty much everything discussed here, and in fact incorporate a lot of this discussion.

@dlsniper
Copy link
Contributor

dlsniper commented Mar 9, 2012

Hello,

Any words on this? Or any position from the dev team about this matter?
Where should the community move it's efforts for this to come true?

Kind regards.

@fabpot
Copy link
Member

fabpot commented Mar 10, 2012

I have asked @vicb to lead this effort. This will probably land into Symfony 2.2.

@pulzarraider
Copy link
Contributor

With my current project I've started using cache much more than before. My initial implementation was done on SonataCahceBundle, but I have run into some problems that I had to solve. Here are my experiences of what Cache component/Cache bundle should cover, maybe it can help you design the cache implementation for real usage (some of these issues was mentioned in previous comments):

  • Give availability to use many instances of memcache(d)/APC/... cache clasess with different configuration options (different server, different prefix etc.). Let user choose what he needs.
  • Allow increment/decrement methods - APC/memcache(d)/Redis/... - all of them have inc/decr methods implemented. They should be availbale also in Cache Component. If these methods are used, NO OBJECT (CacheElement or something similar) may be used. So using some caching object may not be ideal.
  • The cache component should provide some lock method to allow rendering of cache to one process only. The main reason for using cache is to accelerate project. If one process have started rendering cache, another process should wait. It's not good idea to let unlimited processes start rendering cache. It will cause many problems, if the rendering process is too difficult.
  • Cache component may be configured to use fallback cache services. The main reason for this is: if no data were found in normal cache service, one process will lock it and starts rendering the cache, other process accesing the same key is not waiting, but using cached data from fallback cache service. Fallback cache service contains old data, but in many cases it's much better than waiting for rendering. The ttl of data in fallback service should be always higher than in normal cache service. The fallback cache is updated after the rendering of normal cache is finished.
  • Cache component should give availability to speed up perfromance. Redis/memcache(d) have some methods, that accept array of keys to be deleted. It's much better that calling delete in a loop. Redis have also multi method - multiple command are send at once. I would like to have some abstract access to these special methods in the new Symfony cache component/bundle to speed up performance of my project. If some cache mechanism doesn't support these features, it may be implemented with basic methods on lower layer of cache component.
  • Cache SHOULD support tags. If some caching mechanism don't support it (memcache), the user should be warned to use some sophisticated cache mechanism (Redis). Invalidating and using tags under certain conditions can be simulated in mamcache, too.

@dlsniper
Copy link
Contributor

dlsniper commented Aug 4, 2012

I know it's a bit of a set aside talk for now but I'll write this as a reminder: the cache component should be able to be used from anywhere in the other components, think of Forms, Templating and so on.

@stof
Copy link
Member

stof commented Oct 13, 2012

@vicb any news on this ? According to the last comment of @fabpot on this ticket, you were in charge of this feature.

@vicb
Copy link
Contributor

vicb commented Oct 15, 2012

I won't have time to work on this for at least the coming month. Anybody feel free to start something (a RFC ?).

@lolautruche
Copy link
Contributor

Hi

I think we at least need an interface to rely on, with a basic implementation provided (the one from Config component maybe ?). This would be simple enough not to break too much tests/current implementation and would have the advantage to allow better integration of cache libs (Stash, Zeta Components).

I might be able to start an RFC (should I do it as an issue or as a PR ?).

@dlsniper
Copy link
Contributor

I think we need to wait for @fabpot to figure what we need/how he wants to deal with the problem in the framework then take any action.

I've taken my own implementations to a suspend mode just because I do agree with him that we are in no rush and we should make sure that the problem is being correctly handled.

If you have any ideas about it, you could/should post it either here or in the other existing cache issues/PRs so we could have a central point of reference about this problem.

@fabpot
Copy link
Member

fabpot commented Mar 23, 2013

Closing as there are 4 opened PR on this topic. I would prefer that we first discuss the interface and what we want before starting coding. I suggest that this discussion happens on the Symfony dev mailing-list. Also, we need to discuss what to do with the current discussion on the FIG group mailing-list.

@fabpot
Copy link
Member

fabpot commented Mar 23, 2013

@sukei
Copy link

sukei commented Sep 2, 2013

Hello, any news on this?

@dlsniper
Copy link
Contributor

dlsniper commented Sep 2, 2013

@sukei FIG is the place to ask, When they'll figure out about PSR Cache then this could go on...

@ghost
Copy link

ghost commented Dec 29, 2015

I think it would be nice to have a generic Cache component in Symfony.
Is PSR-6 ready? http://www.php-fig.org/psr/psr-6/

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

Successfully merging this pull request may close these issues.

None yet