From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 6 Apr 2012 17:38:38 +0000 (-0700)
Subject: [FileRepo] Added some cache code based on the problems in r97512.
X-Git-Tag: 1.31.0-rc.0~24000
X-Git-Url: https://git.cyclocoop.org/%28%28?a=commitdiff_plain;h=5d585a423f7bafc45ae67f4cf1cad2c1b928fbf7;p=lhc%2Fweb%2Fwiklou.git

[FileRepo] Added some cache code based on the problems in r97512.

* Made RepoGroup avoid caching files with large metadata and also reduced max cache size to 500 to avoid OOMs.
* Factored out a pingCache() function in RepoGroup.

Change-Id: I52f6413b9eb8b11fbffbde0f0e7acf97c7a2ff89
---

diff --git a/includes/filerepo/RepoGroup.php b/includes/filerepo/RepoGroup.php
index 809633632f..b109803b66 100644
--- a/includes/filerepo/RepoGroup.php
+++ b/includes/filerepo/RepoGroup.php
@@ -12,7 +12,6 @@
  * @ingroup FileRepo
  */
 class RepoGroup {
-
 	/**
 	 * @var LocalRepo
 	 */
@@ -26,7 +25,7 @@ class RepoGroup {
 	 * @var RepoGroup
 	 */
 	protected static $instance;
-	const MAX_CACHE_SIZE = 1000;
+	const MAX_CACHE_SIZE = 500;
 
 	/**
 	 * Get a RepoGroup instance. At present only one instance of RepoGroup is
@@ -114,48 +113,41 @@ class RepoGroup {
 			&& empty( $options['private'] )
 			&& empty( $options['bypassCache'] ) )
 		{
-			$useCache = true;
 			$time = isset( $options['time'] ) ? $options['time'] : '';
 			$dbkey = $title->getDBkey();
 			if ( isset( $this->cache[$dbkey][$time] ) ) {
 				wfDebug( __METHOD__.": got File:$dbkey from process cache\n" );
 				# Move it to the end of the list so that we can delete the LRU entry later
-				$tmp = $this->cache[$dbkey];
-				unset( $this->cache[$dbkey] );
-				$this->cache[$dbkey] = $tmp;
+				$this->pingCache( $dbkey );
 				# Return the entry
 				return $this->cache[$dbkey][$time];
-			} else {
-				# Add a negative cache entry, may be overridden
-				$this->trimCache();
-				$this->cache[$dbkey][$time] = false;
-				$cacheEntry =& $this->cache[$dbkey][$time];
 			}
+			$useCache = true;
 		} else {
 			$useCache = false;
 		}
 
 		# Check the local repo
 		$image = $this->localRepo->findFile( $title, $options );
-		if ( $image ) {
-			if ( $useCache ) {
-				$cacheEntry = $image;
-			}
-			return $image;
-		}
 
 		# Check the foreign repos
-		foreach ( $this->foreignRepos as $repo ) {
-			$image = $repo->findFile( $title, $options );
-			if ( $image ) {
-				if ( $useCache ) {
-					$cacheEntry = $image;
+		if ( !$image ) {
+			foreach ( $this->foreignRepos as $repo ) {
+				$image = $repo->findFile( $title, $options );
+				if ( $image ) {
+					break;
 				}
-				return $image;
 			}
 		}
-		# Not found, do not override negative cache
-		return false;
+
+		$image = $image ? $image : false; // type sanity
+		# Cache file existence or non-existence
+		if ( $useCache && ( !$image || $image->isCacheable() ) ) {
+			$this->trimCache();
+			$this->cache[$dbkey][$time] = $image;
+		}
+
+		return $image;
 	}
 
 	function findFiles( $inputItems ) {
@@ -371,6 +363,17 @@ class RepoGroup {
 		}
 	}
 
+	/**
+	 * Move a cache entry to the top (such as when accessed)
+	 */
+	protected function pingCache( $key ) {
+		if ( isset( $this->cache[$key] ) ) {
+			$tmp = $this->cache[$key];
+			unset( $this->cache[$key] );
+			$this->cache[$key] = $tmp;
+		}
+	}
+
 	/**
 	 * Limit cache memory
 	 */
diff --git a/includes/filerepo/file/File.php b/includes/filerepo/file/File.php
index 56272da4bb..ce69a03a54 100644
--- a/includes/filerepo/file/File.php
+++ b/includes/filerepo/file/File.php
@@ -755,7 +755,7 @@ abstract class File {
 
 	/**
 	 * Return either a MediaTransformError or placeholder thumbnail (if $wgIgnoreImageErrors)
-	 * 
+	 *
 	 * @param $thumbPath string Thumbnail storage path
 	 * @param $thumbUrl string Thumbnail URL
 	 * @param $params Array
@@ -1705,6 +1705,14 @@ abstract class File {
 		return false;
 	}
 
+	/**
+	 * Check if this file object is small and can be cached
+	 * @return boolean
+	 */
+	public function isCacheable() {
+		return true;
+	}
+
 	/**
 	 * Assert that $this->repo is set to a valid FileRepo instance
 	 * @throws MWException
diff --git a/includes/filerepo/file/LocalFile.php b/includes/filerepo/file/LocalFile.php
index b7097091a5..40ee0fa923 100644
--- a/includes/filerepo/file/LocalFile.php
+++ b/includes/filerepo/file/LocalFile.php
@@ -29,6 +29,8 @@ define( 'MW_FILE_VERSION', 8 );
  * @ingroup FileAbstraction
  */
 class LocalFile extends File {
+	const CACHE_FIELD_MAX_LEN = 1000;
+
 	/**#@+
 	 * @private
 	 */
@@ -1021,9 +1023,9 @@ class LocalFile extends File {
 		);
 
 		if ( $dbw->affectedRows() == 0 ) {
-			# (bug 34993) Note: $oldver can be empty here, if the previous 
-			# version of the file was broken. Allow registration of the new 
-			# version to continue anyway, because that's better than having 
+			# (bug 34993) Note: $oldver can be empty here, if the previous
+			# version of the file was broken. Allow registration of the new
+			# version to continue anyway, because that's better than having
 			# an image that's not fixable by user operations.
 
 			$reupload = true;
@@ -1431,6 +1433,11 @@ class LocalFile extends File {
 		return $this->sha1;
 	}
 
+	function isCacheable() {
+		$this->load();
+		return strlen( $this->metadata ) <= self::CACHE_FIELD_MAX_LEN; // avoid OOMs
+	}
+
 	/**
 	 * Start a transaction and lock the image for update
 	 * Increments a reference counter if the lock is already held