Bug 19240 (bad image list performance regression):
authorTim Starling <tstarling@users.mediawiki.org>
Wed, 17 Jun 2009 07:31:00 +0000 (07:31 +0000)
committerTim Starling <tstarling@users.mediawiki.org>
Wed, 17 Jun 2009 07:31:00 +0000 (07:31 +0000)
* Don't connect to the commons DB on cache hits, in order to determine the cache key name. Removed remnants of that bright idea from GlobalFunctions.php.
* Fixed total failure of negative caching in checkRedirect() due to memcached stripping trailing spaces from string values. Probably never worked.

Also:
* Respect hasSharedCache in foreign repos. Recently-added code was apparently ignorant of this setting.
* Renamed getMemcKey() to getSharedCacheKey() to make its function more clear, introduced getLocalCacheKey() to do the other thing. Fixed its parameters to be like wfMemcKey() and used it in more places.
* Used getLocalCacheKey() in various places instead of wfMemc(), to avoid having multiple repositories overwrite each others' caches.
* Fixed the BagOStuff bug that the FIXME in LocalRepo::checkRedirect() appears to refer to.
* Removed getMasterDB() and getSlaveDB() from FileRepo, it's incorrect to assume that a repo other than a LocalRepo has an associated database.
* Made FileRepo::invalidateImageRedirect() a stub, to match FileRepo::checkRedirect(). Moved the functionality to LocalRepo where checkRedirect() is concretely implemented.

12 files changed:
RELEASE-NOTES
includes/BagOStuff.php
includes/GlobalFunctions.php
includes/filerepo/File.php
includes/filerepo/FileRepo.php
includes/filerepo/ForeignAPIFile.php
includes/filerepo/ForeignAPIRepo.php
includes/filerepo/ForeignDBFile.php
includes/filerepo/ForeignDBRepo.php
includes/filerepo/ForeignDBViaLBRepo.php
includes/filerepo/LocalFile.php
includes/filerepo/LocalRepo.php

index f308804..bead513 100644 (file)
@@ -185,6 +185,7 @@ it from source control: http://www.mediawiki.org/wiki/Download_from_SVN
 * (bug 18173) MediaWiki now fails when unable to determine a client IP
 * (bug 19170) Special:Version should follow the content language direction
 * (bug 19160) maintenance/purgeOldText.inc is now compatible with PostgreSQL
+* Fixed performance regression in "bad image list" feature
 
 == API changes in 1.16 ==
 
index 572dca6..8f548c8 100644 (file)
@@ -191,7 +191,7 @@ class HashBagOStuff extends BagOStuff {
        }
 
        function get($key) {
-               if(!$this->bag[$key])
+               if( !isset( $this->bag[$key] ) )
                        return false;
                if($this->_expire($key))
                        return false;
@@ -203,7 +203,7 @@ class HashBagOStuff extends BagOStuff {
        }
 
        function delete($key,$time=0) {
-               if(!$this->bag[$key])
+               if( !isset( $this->bag[$key] ) )
                        return false;
                unset($this->bag[$key]);
                return true;
index 427543f..ca149e5 100644 (file)
@@ -2809,16 +2809,12 @@ function wfForeignMemcKey( $db, $prefix /*, ... */ ) {
  * Get an ASCII string identifying this wiki
  * This is used as a prefix in memcached keys
  */
-function wfWikiID( $db = null ) {
-       if( $db instanceof Database ) {
-               return $db->getWikiID();
-       } else {
+function wfWikiID() {
        global $wgDBprefix, $wgDBname;
-               if ( $wgDBprefix ) {
-                       return "$wgDBname-$wgDBprefix";
-               } else {
-                       return $wgDBname;
-               }
+       if ( $wgDBprefix ) {
+               return "$wgDBname-$wgDBprefix";
+       } else {
+               return $wgDBname;
        }
 }
 
index 179dab2..13dc007 100644 (file)
@@ -1077,7 +1077,7 @@ abstract class File {
                if ( $renderUrl ) {
                        if ( $this->repo->descriptionCacheExpiry > 0 ) {
                                wfDebug("Attempting to get the description from cache...");
-                               $key = wfMemcKey( 'RemoteFileDescription', 'url', $wgContLang->getCode(), 
+                               $key = $this->getLocalCacheKey( 'RemoteFileDescription', 'url', $wgContLang->getCode(), 
                                                                        $this->getName() );
                                $obj = $wgMemc->get($key);
                                if ($obj) {
index 48f9e55..79fd87e 100644 (file)
@@ -545,15 +545,19 @@ abstract class FileRepo {
 
        /**
         * Invalidates image redirect cache related to that image
-        *
+        * Doesn't do anything for repositories that don't support image redirects.
+        * 
+        * STUB
         * @param Title $title Title of image
         */     
-       function invalidateImageRedirect( $title ) {
-               global $wgMemc;
-               $memcKey = $this->getMemcKey( "image_redirect:" . md5( $title->getPrefixedDBkey() ) );
-               $wgMemc->delete( $memcKey );
-       }
+       function invalidateImageRedirect( $title ) {}
        
+       /**
+        * Get an array or iterator of file objects for files that have a given 
+        * SHA-1 content hash.
+        *
+        * STUB
+        */
        function findBySha1( $hash ) {
                return array();
        }
@@ -574,16 +578,25 @@ abstract class FileRepo {
                return wfMsg( 'shared-repo' ); 
        }
        
-       function getSlaveDB() {
-               return wfGetDB( DB_SLAVE );
-       }
-
-       function getMasterDB() {
-               return wfGetDB( DB_MASTER );
+       /**
+        * Get a key on the primary cache for this repository.
+        * Returns false if the repository's cache is not accessible at this site. 
+        * The parameters are the parts of the key, as for wfMemcKey().
+        *
+        * STUB
+        */
+       function getSharedCacheKey( /*...*/ ) {
+               return false;
        }
 
-       function getMemcKey( $key ) {
-               return wfWikiID( $this->getSlaveDB() ) . ":{$key}";
+       /**
+        * Get a key for this repo in the local cache domain. These cache keys are 
+        * not shared with remote instances of the repo.
+        * The parameters are the parts of the key, as for wfMemcKey().
+        */
+       function getLocalCacheKey( /*...*/ ) {
+               $args = func_get_args();
+               array_unshift( $args, 'filerepo', $this->getName() );
+               return call_user_func_array( 'wfMemcKey', $args );
        }
-
 }
index 03498fb..f4c02ba 100644 (file)
@@ -162,13 +162,13 @@ class ForeignAPIFile extends File {
        function purgeDescriptionPage() {
                global $wgMemc, $wgContLang;
                $url = $this->repo->getDescriptionRenderUrl( $this->getName(), $wgContLang->getCode() );
-               $key = wfMemcKey( 'RemoteFileDescription', 'url', md5($url) );
+               $key = $this->repo->getLocalCacheKey( 'RemoteFileDescription', 'url', md5($url) );
                $wgMemc->delete( $key );
        }
        
        function purgeThumbnails() {
                global $wgMemc;
-               $key = wfMemcKey( 'ForeignAPIRepo', 'ThumbUrl', $this->getName() );
+               $key = $this->repo->getLocalCacheKey( 'ForeignAPIRepo', 'ThumbUrl', $this->getName() );
                $wgMemc->delete( $key );
                $files = $this->getThumbnails();
                $dir = $this->getThumbPath( $this->getName() );
index cda3500..2f1c391 100644 (file)
@@ -114,7 +114,7 @@ class ForeignAPIRepo extends FileRepo {
                                                'action' => 'query' ) ) );
                
                if( !isset( $this->mQueryCache[$url] ) ) {
-                       $key = wfMemcKey( 'ForeignAPIRepo', 'Metadata', md5( $url ) );
+                       $key = $this->getLocalCacheKey( 'ForeignAPIRepo', 'Metadata', md5( $url ) );
                        $data = $wgMemc->get( $key );
                        if( !$data ) {
                                $data = Http::get( $url );
@@ -176,7 +176,7 @@ class ForeignAPIRepo extends FileRepo {
                        return $this->getThumbUrl( $name, $width, $height );
                }
                
-               $key = wfMemcKey( 'ForeignAPIRepo', 'ThumbUrl', $name );
+               $key = $this->getLocalCacheKey( 'ForeignAPIRepo', 'ThumbUrl', $name );
                if ( $thumbUrl = $wgMemc->get($key) ) {
                        wfDebug("Got thumb from local cache. $thumbUrl \n");
                        return $thumbUrl;
index 8fe6f92..a24ff72 100644 (file)
@@ -19,16 +19,6 @@ class ForeignDBFile extends LocalFile {
                return $file;
        }
 
-       function getCacheKey() {
-               if ( $this->repo->hasSharedCache() ) {
-                       $hashedName = md5($this->name);
-                       return wfForeignMemcKey( $this->repo->dbName, $this->repo->tablePrefix,
-                               'file', $hashedName );
-               } else {
-                       return false;
-               }
-       }
-
        function publish( $srcPath, $flags = 0 ) {
                $this->readOnlyError();
        }
index e078dd2..35c2c4b 100644 (file)
@@ -44,6 +44,21 @@ class ForeignDBRepo extends LocalRepo {
                return $this->hasSharedCache;
        }
 
+       /**
+        * Get a key on the primary cache for this repository.
+        * Returns false if the repository's cache is not accessible at this site. 
+        * The parameters are the parts of the key, as for wfMemcKey().
+        */
+       function getSharedCacheKey( /*...*/ ) {
+               if ( $this->hasSharedCache() ) {
+                       $args = func_get_args();
+                       array_unshift( $args, $this->dbName, $this->tablePrefix );
+                       return call_user_func_array( 'wfForeignMemcKey', $args );
+               } else {
+                       return false;
+               }
+       }
+
        function store( $srcPath, $dstZone, $dstRel, $flags = 0 ) {
                throw new MWException( get_class($this) . ': write operations are not supported' );
        }
index 13c9f43..8032575 100644 (file)
@@ -27,6 +27,21 @@ class ForeignDBViaLBRepo extends LocalRepo {
                return $this->hasSharedCache;
        }
 
+       /**
+        * Get a key on the primary cache for this repository.
+        * Returns false if the repository's cache is not accessible at this site. 
+        * The parameters are the parts of the key, as for wfMemcKey().
+        */
+       function getSharedCacheKey( /*...*/ ) {
+               if ( $this->hasSharedCache() ) {
+                       $args = func_get_args();
+                       array_unshift( $args, $this->wiki );
+                       return implode( ':', $args );
+               } else {
+                       return false;
+               }
+       }
+
        function store( $srcPath, $dstZone, $dstRel, $flags = 0 ) {
                throw new MWException( get_class($this) . ': write operations are not supported' );
        }
index 4fe2c71..4c0e499 100644 (file)
@@ -133,11 +133,12 @@ class LocalFile extends File
        }
 
        /**
-        * Get the memcached key
+        * Get the memcached key for the main data for this file, or false if 
+        * there is no access to the shared cache.
         */
        function getCacheKey() {
                $hashedName = md5($this->getName());
-               return wfMemcKey( 'file', $hashedName );
+               return $this->repo->getSharedCacheKey( 'file', $hashedName );
        }
 
        /**
@@ -590,8 +591,10 @@ class LocalFile extends File
        function purgeHistory() {
                global $wgMemc;
                $hashedName = md5($this->getName());
-               $oldKey = wfMemcKey( 'oldfile', $hashedName );
-               $wgMemc->delete( $oldKey );
+               $oldKey = $this->getSharedCacheKey( 'oldfile', $hashedName );
+               if ( $oldKey ) {
+                       $wgMemc->delete( $oldKey );
+               }
        }
 
        /**
index 1150964..677f628 100644 (file)
@@ -83,17 +83,24 @@ class LocalRepo extends FSRepo {
                        $title = Title::makeTitle( NS_FILE, $title->getText() );
                }
 
-               $memcKey = $this->getMemcKey( "image_redirect:" . md5( $title->getPrefixedDBkey() ) );
+               $memcKey = $this->getSharedCacheKey( 'image_redirect', md5( $title->getPrefixedDBkey() ) );
+               if ( $memcKey === false ) {
+                       $memcKey = $this->getLocalCacheKey( 'image_redirect', md5( $title->getPrefixedDBkey() ) );
+                       $expiry = 300; // no invalidation, 5 minutes
+               } else {
+                       $expiry = 86400; // has invalidation, 1 day
+               }
                $cachedValue = $wgMemc->get( $memcKey );
-               if( $cachedValue ) {
-                       return Title::newFromDbKey( $cachedValue );
-               } elseif( $cachedValue == ' ' ) { # FIXME: ugly hack, but BagOStuff caching seems to be weird and return false if !cachedValue, not only if it doesn't exist
+               if ( $cachedValue === ' '  || $cachedValue === '' ) {
+                       // Does not exist
                        return false;
-               }
+               } elseif ( strval( $cachedValue ) !== '' ) {
+                       return Title::newFromText( $cachedValue );
+               } // else $cachedValue is false or null: cache miss
 
                $id = $this->getArticleID( $title );
                if( !$id ) {
-                       $wgMemc->set( $memcKey, " ", 9000 );
+                       $wgMemc->set( $memcKey, " ", $expiry );
                        return false;
                }
                $dbr = $this->getSlaveDB();
@@ -104,12 +111,14 @@ class LocalRepo extends FSRepo {
                        __METHOD__
                );
 
-               if( $row ) $targetTitle = Title::makeTitle( $row->rd_namespace, $row->rd_title );
-               $wgMemc->set( $memcKey, ($row ? $targetTitle->getPrefixedDBkey() : " "), 9000 );
-               if( !$row ) {
+               if( $row ) {
+                       $targetTitle = Title::makeTitle( $row->rd_namespace, $row->rd_title );
+                       $wgMemc->set( $memcKey, $targetTitle->getPrefixedDBkey(), $expiry );
+                       return $targetTitle;
+               } else {
+                       $wgMemc->set( $memcKey, '', $expiry );
                        return false;
                }
-               return $targetTitle;
        }
 
 
@@ -134,8 +143,10 @@ class LocalRepo extends FSRepo {
                return $id;
        }
 
-
-       
+       /**
+        * Get an array or iterator of file objects for files that have a given 
+        * SHA-1 content hash.
+        */
        function findBySha1( $hash ) {
                $dbr = $this->getSlaveDB();
                $res = $dbr->select(
@@ -174,4 +185,42 @@ class LocalRepo extends FSRepo {
                $res->free();
                return $result;
        }
+
+       /**
+        * Get a connection to the slave DB
+        */
+       function getSlaveDB() {
+               return wfGetDB( DB_SLAVE );
+       }
+
+       /**
+        * Get a connection to the master DB
+        */
+       function getMasterDB() {
+               return wfGetDB( DB_MASTER );
+       }
+
+       /**
+        * Get a key on the primary cache for this repository.
+        * Returns false if the repository's cache is not accessible at this site. 
+        * The parameters are the parts of the key, as for wfMemcKey().
+        */
+       function getSharedCacheKey( /*...*/ ) {
+               $args = func_get_args();
+               return call_user_func_array( 'wfMemcKey', $args );
+       }
+
+       /**
+        * Invalidates image redirect cache related to that image
+        *
+        * @param Title $title Title of image
+        */     
+       function invalidateImageRedirect( $title ) {
+               global $wgMemc;
+               $memcKey = $this->getSharedCacheKey( 'image_redirect', md5( $title->getPrefixedDBkey() ) );
+               if ( $memcKey ) {
+                       $wgMemc->delete( $memcKey );
+               }
+       }
 }
+