From 6cba5762b72cb5f0b0509f1d580a516b1f921f3d Mon Sep 17 00:00:00 2001 From: Tim Starling Date: Wed, 17 Jun 2009 07:31:00 +0000 Subject: [PATCH] Bug 19240 (bad image list performance regression): * 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. --- RELEASE-NOTES | 1 + includes/BagOStuff.php | 4 +- includes/GlobalFunctions.php | 14 ++--- includes/filerepo/File.php | 2 +- includes/filerepo/FileRepo.php | 43 +++++++++----- includes/filerepo/ForeignAPIFile.php | 4 +- includes/filerepo/ForeignAPIRepo.php | 4 +- includes/filerepo/ForeignDBFile.php | 10 ---- includes/filerepo/ForeignDBRepo.php | 15 +++++ includes/filerepo/ForeignDBViaLBRepo.php | 15 +++++ includes/filerepo/LocalFile.php | 11 ++-- includes/filerepo/LocalRepo.php | 73 ++++++++++++++++++++---- 12 files changed, 139 insertions(+), 57 deletions(-) diff --git a/RELEASE-NOTES b/RELEASE-NOTES index f308804e2d..bead513cfe 100644 --- a/RELEASE-NOTES +++ b/RELEASE-NOTES @@ -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 == diff --git a/includes/BagOStuff.php b/includes/BagOStuff.php index 572dca6c41..8f548c86f4 100644 --- a/includes/BagOStuff.php +++ b/includes/BagOStuff.php @@ -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; diff --git a/includes/GlobalFunctions.php b/includes/GlobalFunctions.php index 427543f44d..ca149e5ad3 100644 --- a/includes/GlobalFunctions.php +++ b/includes/GlobalFunctions.php @@ -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; } } diff --git a/includes/filerepo/File.php b/includes/filerepo/File.php index 179dab2f29..13dc007154 100644 --- a/includes/filerepo/File.php +++ b/includes/filerepo/File.php @@ -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) { diff --git a/includes/filerepo/FileRepo.php b/includes/filerepo/FileRepo.php index 48f9e5506c..79fd87e3ab 100644 --- a/includes/filerepo/FileRepo.php +++ b/includes/filerepo/FileRepo.php @@ -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 ); } - } diff --git a/includes/filerepo/ForeignAPIFile.php b/includes/filerepo/ForeignAPIFile.php index 03498fb115..f4c02ba6ca 100644 --- a/includes/filerepo/ForeignAPIFile.php +++ b/includes/filerepo/ForeignAPIFile.php @@ -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() ); diff --git a/includes/filerepo/ForeignAPIRepo.php b/includes/filerepo/ForeignAPIRepo.php index cda3500556..2f1c391c1f 100644 --- a/includes/filerepo/ForeignAPIRepo.php +++ b/includes/filerepo/ForeignAPIRepo.php @@ -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; diff --git a/includes/filerepo/ForeignDBFile.php b/includes/filerepo/ForeignDBFile.php index 8fe6f92124..a24ff72b9d 100644 --- a/includes/filerepo/ForeignDBFile.php +++ b/includes/filerepo/ForeignDBFile.php @@ -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(); } diff --git a/includes/filerepo/ForeignDBRepo.php b/includes/filerepo/ForeignDBRepo.php index e078dd25bc..35c2c4bf11 100644 --- a/includes/filerepo/ForeignDBRepo.php +++ b/includes/filerepo/ForeignDBRepo.php @@ -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' ); } diff --git a/includes/filerepo/ForeignDBViaLBRepo.php b/includes/filerepo/ForeignDBViaLBRepo.php index 13c9f434f2..803257521e 100644 --- a/includes/filerepo/ForeignDBViaLBRepo.php +++ b/includes/filerepo/ForeignDBViaLBRepo.php @@ -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' ); } diff --git a/includes/filerepo/LocalFile.php b/includes/filerepo/LocalFile.php index 4fe2c71408..4c0e499617 100644 --- a/includes/filerepo/LocalFile.php +++ b/includes/filerepo/LocalFile.php @@ -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 ); + } } /** diff --git a/includes/filerepo/LocalRepo.php b/includes/filerepo/LocalRepo.php index 1150964d19..677f6280b0 100644 --- a/includes/filerepo/LocalRepo.php +++ b/includes/filerepo/LocalRepo.php @@ -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 ); + } + } } + -- 2.20.1