r101507 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r101506‎ | r101507 | r101508 >
Date:18:31, 1 November 2011
Author:mah
Status:resolved (Comments)
Tags:
Comment:
Re-apply Vitaliy Filippov's patch from Bug #29283 with a modification
to make it work when no ACCEL cache is available.
Modified paths:
  • /trunk/phase3/includes/AutoLoader.php (modified) (history)
  • /trunk/phase3/includes/LocalisationCache.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/LocalisationCache.php
@@ -159,8 +159,16 @@
160160 case 'db':
161161 $storeClass = 'LCStore_DB';
162162 break;
 163+ case 'accel':
 164+ $storeClass = 'LCStore_Accel';
 165+ break;
163166 case 'detect':
164 - $storeClass = $wgCacheDirectory ? 'LCStore_CDB' : 'LCStore_DB';
 167+ try {
 168+ $c = wfGetCache( CACHE_ACCEL );
 169+ $storeClass = 'LCStore_Accel';
 170+ } catch( Exception $c ) {
 171+ $storeClass = $wgCacheDirectory ? 'LCStore_CDB' : 'LCStore_DB';
 172+ }
165173 break;
166174 default:
167175 throw new MWException(
@@ -348,7 +356,9 @@
349357 }
350358
351359 $deps = $this->store->get( $code, 'deps' );
352 - if ( $deps === null ) {
 360+ $keys = $this->store->get( $code, 'list', 'messages' );
 361+ // 'list:messages' sometimes expires separately of 'deps' in LCStore_Accel
 362+ if ( $deps === null || $keys === null ) {
353363 wfDebug( __METHOD__."($code): cache missing, need to make one\n" );
354364 return true;
355365 }
@@ -830,6 +840,56 @@
831841 }
832842
833843 /**
 844+ * LCStore implementation which uses PHP accelerator to store data.
 845+ * This will work if one of XCache, eAccelerator, or APC cacher is configured.
 846+ * (See ObjectCache.php)
 847+ */
 848+class LCStore_Accel implements LCStore {
 849+ var $currentLang;
 850+ var $keys;
 851+
 852+ public function __construct() {
 853+ $this->cache = wfGetCache( CACHE_ACCEL );
 854+ }
 855+
 856+ public function get( $code, $key ) {
 857+ $k = wfMemcKey( 'l10n', $code, 'k', $key );
 858+ $r = $this->cache->get( $k );
 859+ if ( $r === false ) return null;
 860+ return $r;
 861+ }
 862+
 863+ public function startWrite( $code ) {
 864+ $k = wfMemcKey( 'l10n', $code, 'l' );
 865+ $keys = $this->cache->get( $k );
 866+ if ( $keys ) {
 867+ foreach ( $keys as $k ) {
 868+ $this->cache->delete( $k );
 869+ }
 870+ }
 871+ $this->currentLang = $code;
 872+ $this->keys = array();
 873+ }
 874+
 875+ public function finishWrite() {
 876+ if ( $this->currentLang ) {
 877+ $k = wfMemcKey( 'l10n', $this->currentLang, 'l' );
 878+ $this->cache->set( $k, array_keys( $this->keys ) );
 879+ }
 880+ $this->currentLang = null;
 881+ $this->keys = array();
 882+ }
 883+
 884+ public function set( $key, $value ) {
 885+ if ( $this->currentLang ) {
 886+ $k = wfMemcKey( 'l10n', $this->currentLang, 'k', $key );
 887+ $this->keys[$k] = true;
 888+ $this->cache->set( $k, $value );
 889+ }
 890+ }
 891+}
 892+
 893+/**
834894 * LCStore implementation which uses the standard DB functions to store data.
835895 * This will work on any MediaWiki installation.
836896 */
@@ -1126,4 +1186,4 @@
11271187 $this->unload( $code );
11281188 }
11291189 }
1130 -}
 1190+}
\ No newline at end of file
Index: trunk/phase3/includes/AutoLoader.php
@@ -127,6 +127,7 @@
128128 'IndexPager' => 'includes/Pager.php',
129129 'Interwiki' => 'includes/interwiki/Interwiki.php',
130130 'IP' => 'includes/IP.php',
 131+ 'LCStore_Accel' => 'includes/LocalisationCache.php',
131132 'LCStore_CDB' => 'includes/LocalisationCache.php',
132133 'LCStore_DB' => 'includes/LocalisationCache.php',
133134 'LCStore_Null' => 'includes/LocalisationCache.php',

Follow-up revisions

RevisionCommit summaryAuthorDate
r102304re Bug #29283, r101507: Apply Vitaliy Filippov's followup fix:...mah17:50, 7 November 2011
r104029Followup r101507: don't automatically use APC for the localization cache if i...catrope13:12, 23 November 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r101492Apply Vitaliy Filippov's patch from Bug #29283 with some small style modsmah16:10, 1 November 2011

Comments

#Comment by Raymond (talk | contribs)   19:41, 1 November 2011

Translatewiki.net is horribly broken with this revision:

Exception from line 416 of /www/w/includes/LocalisationCache.php: No localisation cache found for English. Please run maintenance/rebuildLocalisationCache.php.
#Comment by Siebrand (talk | contribs)   21:34, 1 November 2011

Very nasty. Required us to update our LocalSettings.php, adding

$wgLocalisationCacheConf['store'] = 'file';

Where we already had:

$wgLocalisationCacheConf['manualRecache'] = true;

This needs RELEASE-NOTES and pretty high up, too. Thanks to Roan for his help in resolving this.

#Comment by Catrope (talk | contribs)   21:58, 1 November 2011

This is because the default value for ['store'] is 'detect', which now prefers APC over CDB. So on wikis with APC enabled where ['store'] is not overridden in the config, LC will automatically switch to using APC where it previously used CDB.

This is normally not an issue, because an empty cache is simply repopulated. However, if ['manualRecache'] is enabled, empty caches cause an error.

We could announce this as a scaptrap/breaking change, but I think it would be better to put APC at the bottom of the priority list, so a wiki won't suddenly decide to abandon another caching backend (CDB or DB) in favor of APC after enabling this code. Another reason is that on multi-server setups, LC in APC wouldn't be very efficient (every server has its own cache) whereas a DB cache is automatically synchronized and a CDB cache can be synchronized (I think TWN does the latter, and I think that's why they use manualRecache; WMF lets each server rebuild its own LC using CDB but I've been trying to talk Tim out of that). However, APC is typically enabled on these servers anyway, not for key-value caching but for opcode caching.

The final nail in the coffin for me is that the APC cache is not accessible from maintenance scripts. I believe Brion is very wary of APC caching for this reason, and expressed doubts as to whether we should even continue to support it. It sounds to me like, given that problem, we probably shouldn't be enabling APC by default for anything.

#Comment by Aaron Schulz (talk | contribs)   22:03, 1 November 2011

Sold me on the maintenance bit.

Maybe the LC cdb per server issue could go in private-l for discussion?

#Comment by Catrope (talk | contribs)   22:04, 1 November 2011

I guess so. So far it only exists as an IRC convo between me and Tim, after Niklas told me about manualRecache and how he uses it at TWN. But I don't feel overly strongly about it.

#Comment by Nikerabbit (talk | contribs)   07:59, 2 November 2011

I might be mistaken, but I think APC does have some kind of command line support nowdays.

#Comment by Aaron Schulz (talk | contribs)   01:25, 13 January 2012

apc.enable_cli = 1 apparently. So maybe all is not lost.

#Comment by MarkAHershberger (talk | contribs)   17:19, 8 November 2011

marking "new" since I think these issues have been dealt with.

#Comment by Nikerabbit (talk | contribs)   18:58, 10 November 2011

Not solved. With this config:

$wgCacheDirectory = "/resources/caches/sandwiki"; $wgLocalisationCacheConf['manualRecache'] = true;

running rebuildLocalisationCache does not create caches that stick.

#Comment by MarkAHershberger (talk | contribs)   20:31, 3 December 2011

r104029 ??

Status & tagging log