Page MenuHomePhabricator

RFC: PSR-6 Cache interface in Mediawiki core
Closed, DeclinedPublic

Description

PSR-6 is the Caching interface introduced by PHP-FIG last year.
http://www.php-fig.org/psr/psr-6/

There has been a lot of discussion about the interface itself, both positive and negative.

I propose starting to use this standard in mediawiki core, initially using a wrapper around the BagOStuff abstract class (see the first patch below).
Proposed usage of the wrapper can be seen in the second and third patches where it is used in the new WatchedItemStore.

The case I am trying to cover here is to allow WatchedItemStore to be able to use any kind of cache (even one that persists for longer than a single process).
This would thus allow more caching of WatchedItems and less DB calls.
To be able to effectively keep the cache in check and purge items as they change all items must be indexed (by user and by page).

The PSR-6 patches below do this using the wrapper and pulling in a library for taggable PSR caches.
An alternate solution (not using PSR-6) would likely require a BagOStuff interface that would be implemented by the current abstract base class.
This would then allow the creation of some sort of IndexedBagOStuff.

Of course with time BagOStuff could be phased out, other PSR-6 cache implementations could be used etc.
Also core could make use of further libraries building on top of the PSR.

Alternatively, the use of the PSR-6 decorator could be allowed only for third party components that need a cache, but don't want to have a dependency on MediaWiki.

PSR6 Patches

BagOStuff interface extraction patch

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 21 2016, 2:10 PM
hoo updated the task description. (Show Details)Mar 21 2016, 2:33 PM
bd808 added a subscriber: bd808.Mar 21 2016, 7:20 PM

The vote summary on PSR-6 is worth a look I think:

For: 16
Abstain: 2
Against: 6
Amount required to meet quorum: 15
Amount of members voted: 24
Amount of members in group: 43
Time spent on this proposal: 4 years
This has been the longest running and arguably most controversial proposal to date, so on that here are some closing notes.

  1. Remember that this is a cache recommendation and nobody is forced to implement it, just like any other recommendation the group publishes.
  2. Remember that if there was a clear simple way to implement caching then there wouldn't be so many different libraries out there already. There's more than one way to skin a cat, and not everyone is going to agree. We all have to, as a community, accept this.
  3. Most importantly, remember to be respectful and polite to other community member's opinions.

Among the no votes:

  • Guzzle
  • Doctrine
  • Composer
  • Laravel
  • Wikibase & SMW

The description here doesn't make clear what is compelling about PSR-6 compatibility for MediaWiki in general or BagOStuff specifically.

The main use case seems to be using https://github.com/php-cache/taggable-cache in WatchedItemStore. Can't we find an easier way to store collections of tag -> item mappings?

The main use case seems to be using https://github.com/php-cache/taggable-cache in WatchedItemStore. Can't we find an easier way to store collections of tag -> item mappings?

I wouldnt say easier.. but.

An alternate solution (not using PSR-6) would likely require a BagOStuff interface that would be implemented by the current abstract base class.
This would then allow the creation of some sort of IndexedBagOStuff.

As this would of course require a BagOStuff interface or some caching interface (the abstract base class is no good) in core I figured we should discuss using the one that already exists first.

The case I am trying to cover here is to allow WatchedItemStore to be able to use any kind of cache (even one that persists for longer than a single process).

OTOH, we already have BagOStuff subclasses for every kind of cache that we actually use. If it comes down to adding a new one for some reason for something that already implements PSR-6, we could possibly write a "PSR6BagOStuff" for it.

To be able to effectively keep the cache in check and purge items as they change all items must be indexed (by user and by page).
The PSR-6 patches below do this using the wrapper and pulling in a library for taggable PSR caches.

Looking at the implementation of the library you're pulling in, all it does is add mappings in the store for "__tag.${tag}" to [ 'keys', 'having', 'that', 'tag' ], and turns every value saved into [ 'value' => $value, 'tags' => $tags ]. It doesn't even use CAS for its array manipulations, which might be problematic.

That doesn't seem particularly difficult to implement as a wrapper on top of BagOStuff instead.

OTOH, we already have BagOStuff subclasses for every kind of cache that we actually use. If it comes down to adding a new one for some reason for something that already implements PSR-6, we could possibly write a "PSR6BagOStuff" for it.

Note all - see for instance MapCacheLRU and ProcessCacheLRU (they don't even share an interfrace), or WANObjectCache; not to mention special purpose things like LinkCache or ParserCache. If there is a standard that would allow us to unify MapCacheLRU and ProcessCacheLRU with BagOStuff, and would also cover WANObjectCache, that would be really nice.

It doesn't even use CAS for its array manipulations, which might be problematic.

Yes that is concerning (re the tagging library used)

That doesn't seem particularly difficult to implement as a wrapper on top of BagOStuff instead.

I agree.
My first draft was a wrapper for a BagOStuff that would cache things based on the key parts that were passed in when generating the key (not ideal)

My aim as an outcome of this RFC would be to have a caching interface defined for use in core.
The outcomes basically are:

  1. Use PSR-6
  2. Make an interface in core that looks like BagOStuff
  3. Make an interface in core that looks like something else

Note all - see for instance MapCacheLRU and ProcessCacheLRU (they don't even share an interfrace), or WANObjectCache; not to mention special purpose things like LinkCache or ParserCache. If there is a standard that would allow us to unify MapCacheLRU and ProcessCacheLRU with BagOStuff, and would also cover WANObjectCache, that would be really nice.

I was referring to "kind of cache" as in "backend caching engine, e.g. memcached, redis, SQL database tables", not "PHP caching class built on top of some backend".

For a current view on the public interface the BagOStuff abstract class provides see

https://gerrit.wikimedia.org/r/#/c/279147/1/includes/libs/objectcache/IBagOStuff.php

Addshore updated the task description. (Show Details)Mar 23 2016, 5:02 PM
Krinkle edited projects, added TechCom-RFC; removed Proposal.Mar 23 2016, 8:25 PM
Krinkle renamed this task from RFC PSR-6 in Mediawiki core to RFC: PSR-6 Cache interface in Mediawiki core.Mar 23 2016, 8:37 PM
GWicke moved this task from Inbox to (unused) on the TechCom-RFC board.Mar 23 2016, 8:39 PM
Addshore added a comment.EditedApr 7 2016, 9:52 AM

So I also went ahead at wrote a basic PSR-6 wrapper for WANObjectCache https://gerrit.wikimedia.org/r/#/c/282121/ (The code is essentially the same as the BagOStuff wrapper)

I also tried drawing some common interface between WANObjectCache and BagOStuff but there are difference there.

RobLa-WMF mentioned this in Unknown Object (Event).Apr 13 2016, 6:54 PM

@Addshore, do you think this is ready for an RFC meeting? Are there specific questions you would want to see answered in such a meeting? Or are there any blockers to be resolved first?

@Addshore, do you think this is ready for an RFC meeting? Are there specific questions you would want to see answered in such a meeting? Or are there any blockers to be resolved first?

I think this could be ready for a meeting.

Main points would be

  • Can we / should we just use PSR6, if so why, if not why not?
  • In the case PSR6 is a no, what should our cache interface look like?
daniel claimed this task.Apr 19 2016, 3:10 PM

taking this on as a shepherd

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 19 2016, 3:10 PM
RobLa-WMF mentioned this in Unknown Object (Event).Apr 20 2016, 6:43 AM

@Addshore We could discuss this in the IRC meeting on May 4, 9pm UTC. Would that work for you?

@Addshore We could discuss this in the IRC meeting on May 4, 9pm UTC. Would that work for you?

Yup!

@Addshore We could discuss this in the IRC meeting on May 4, 9pm UTC. Would that work for you?

Yup!

Thanks @Addshore! I look forward to discussing this at E169: RFC Meeting: PSR-6 Cache interface in Mediawiki core (2016-05-04, #wikimedia-office)

@Addshore I suggest to write to Wikitech-l a few days before the meeting to generate some discussion here. That's seems a good way to jumpstart irc discussions.

Note all - see for instance MapCacheLRU and ProcessCacheLRU (they don't even share an interfrace), or WANObjectCache; not to mention special purpose things like LinkCache or ParserCache. If there is a standard that would allow us to unify MapCacheLRU and ProcessCacheLRU with BagOStuff, and would also cover WANObjectCache, that would be really nice.

I'm not really sure that it would be nice. Common interfaces are for things that can be arbitrarily swapped for each other. In the case of most of the cache implementations I see named here there are very different use cases which require different usage by the calling code. Arbitrarily replacing one cache backend with another seems problematic.

We already have problems with this from BagOStuff variants where for example replacing a MemcachedBagOStuff with an APCBagOStuff can lead to resource exhaustion if the calling code assumes that the BagOStuff has some sort of memory pressure eviction system and thus cached items can be given an infinite expiration date. The APC backing store fails this assumption and will either fill up and block all later additions (Zend) or run the host computer out of memory (HHVM).

Having a deep hierarchy of interfaces that very granularly define the characteristics of the backing store doesn't to me seem to be more useful in a closed system (MediaWiki) than only having a set of classes that do the same thing. I'm not seeing the benefit of Cache\Taggable\TaggablePoolInterface over TaggedBagOStuff.

Things I find specifically troubling in the PSR-6 specification:

  • clear() is difficult to implement in any distributed cache system and likely to be misused in code. Its a bit like having a dropDatabase() method as a standard part of your ORM.
  • Interfaces named CacheException and InvalidArgumentException are confusing. Maybe this is just an implementation detail that I should ignore.
  • The CacheItemInterface value object really seems like overkill for 90% of cache usage.

This is what we seem to be discussing in E169: RFC Meeting: PSR-6 Cache interface in Mediawiki core (2016-05-04, #wikimedia-office)

  1. what problem was this RFC trying to solve?
  2. if BagOStuff is an obstacle, then which is true?
    1. need a better caching abstraction than BagOStuff? or
    2. does BagOStuff just need incremental improvement
    3. should BagOStuff be deprecated, and not replaced with anything?
  3. if we decide to rethink our caching model, do we need tagged caching?
RobLa-WMF closed this task as Declined.May 4 2016, 10:31 PM

In E169, it became clear there wasn't consensus for this approach. The full log of E169 is available at P3002

Scott_WUaS updated the task description. (Show Details)May 18 2016, 9:52 PM
Scott_WUaS added a subscriber: Scott_WUaS.
Ejegg added a subscriber: Ejegg.EditedFeb 7 2017, 4:59 AM

Chiming in months late to support a PSR-6 wrapper for use by components that want to use caching but don't want to depend explicitly on mediawiki.

Fundraising tech is trying to library-ize more of the DonationInterface extension, and it would be great if the library could get a simple cache for things like bank code lookups and card attempts per IP address without depending on BagOStuff.

Sounds like the RFC was rejected at the meeting due to MW-internal code needing to know implementation details of caches. Even if the core code doesn't use the interfaces, would it hurt to make them available for less performance-critical or external code?

bd808 added a subscriber: aaron.Feb 7 2017, 4:36 PM

Sounds like the RFC was rejected at the meeting due to MW-internal code needing to know implementation details of caches. Even if the core code doesn't use the interfaces, would it hurt to make them available for less performance-critical or external code?

My main objection was just that PSR-6 is an inelegant abstraction for caching. It's a great example of design-by-committee and the tyranny of patterns.

@aaron, @Legoktm, and others have done a lot of work to move the (horribly named) BagOStuff interface used by MediaWiki into includes/libs/objectcache with the long term intent of publishing it as a Composer managed library (T146257: Create objectcache/BagOStuff library). Are we ready to pull the library out of core and publish it?

Ejegg added a comment.Jul 5 2018, 8:52 PM

Update: Addshore wrote this adapter, which the fundraising team has been using in a payments library that's used both under a mediawiki extension and from inside CiviCRM.
https://github.com/addshore/psr-6-mediawiki-bagostuff-adapter

Usage:
mediawiki extension setup note: https://github.com/wikimedia/mediawiki-extensions-DonationInterface/blob/master/README.txt#L65
mediawiki extension wrapper class: https://github.com/wikimedia/mediawiki-extensions-DonationInterface/blob/master/gateway_common/LocalClusterPsr6Cache.php
backend payment lib written against PSR-6: https://github.com/wikimedia/wikimedia-fundraising-SmashPig/blob/master/PaymentProviders/Ingenico/BankPaymentProvider.php