From: Timo Tijhof <krinklemail@gmail.com>
Date: Thu, 4 Jun 2015 01:52:42 +0000 (+0100)
Subject: resourceloader: Refactor ResourceLoaderWikiModule to reduce database queries
X-Git-Tag: 1.31.0-rc.0~11170^2
X-Git-Url: http://git.cyclocoop.org/%40spipnet%40?a=commitdiff_plain;h=6b2a7fd4b1149bf78ae17d6035c87de847d75cce;p=lhc%2Fweb%2Fwiklou.git

resourceloader: Refactor ResourceLoaderWikiModule to reduce database queries

Wiki modules are special due to their isKnownEmpty implementation and support
for foreign databases. MediaWiki doesn't have convenient ways of making
Revision objects for remote wikis. As such, wiki modules will keep using meta
data to generate the hash.

However minimise needless cache invalidation by refining the implementation.

Impact:
* Remove use of getMsgBlobMtime(). This module doesn't support getMessages().
* In the title info, use the revision content sha1 and size for tracking.
  The page_touched previously used updates too often. It's updated both on edits
  for various types of purges. Using the rev_sha1 means old versions return
  when the content is the same. Regardless of how the content changed via
  revert or actual edits resulting in the same contnet.
* Change in-process cache to be keyed by page list instead of entire
  ResourceLoaderContext.
  Because of this, getTitleInfo() was previously performing its batch query
  twice on the same page. Once for only=styles (top) and only=scripts (bottom).
  Both operate on the full getPages() set but had different context keys.

Clean up:
* Better document the support for foreign databases.
* Move Title construction to getContent to reduce duplication.
* Remove use of getDefinitionMtime(). That method is a no-op since the switch
  to version hashing.
* Remove remaining use of mtime in getModifiedTime(). This is now covered by
  hashing the title info in getDefinitionSummary().

Also refactor the code to be more readable. No intended change in behaviour.

Bug: T98087
Change-Id: Id46740db04c0c42bc5ca87d1487230a32feb34df
---

diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php
index 86d59a14aa..264af5bafe 100644
--- a/includes/resourceloader/ResourceLoaderWikiModule.php
+++ b/includes/resourceloader/ResourceLoaderWikiModule.php
@@ -26,8 +26,21 @@
  * Abstraction for resource loader modules which pull from wiki pages
  *
  * This can only be used for wiki pages in the MediaWiki and User namespaces,
- * because of its dependence on the functionality of
- * Title::isCssJsSubpage.
+ * because of its dependence on the functionality of Title::isCssJsSubpage.
+ *
+ * This module supports being used as a placeholder for a module on a remote wiki.
+ * To do so, getDB() must be overloaded to return a foreign database object that
+ * allows local wikis to query page metadata.
+ *
+ * Safe for calls on local wikis are:
+ * - Option getters:
+ *   - getGroup()
+ *   - getPosition()
+ *   - getPages()
+ * - Basic methods that strictly involve the foreign database
+ *   - getDB()
+ *   - isKnownEmpty()
+ *   - getTitleInfo()
  */
 class ResourceLoaderWikiModule extends ResourceLoaderModule {
 	/** @var string Position on the page to load this module at */
@@ -36,7 +49,7 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 	// Origin defaults to users with sitewide authority
 	protected $origin = self::ORIGIN_USER_SITEWIDE;
 
-	// In-object cache for title info
+	// In-process cache for title info
 	protected $titleInfo = array();
 
 	// List of page names that contain CSS
@@ -116,13 +129,13 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 	}
 
 	/**
-	 * Get the Database object used in getTitleMTimes(). Defaults to the local slave DB
-	 * but subclasses may want to override this to return a remote DB object, or to return
-	 * null if getTitleMTimes() shouldn't access the DB at all.
+	 * Get the Database object used in getTitleInfo().
+	 *
+	 * Defaults to the local slave DB. Subclasses may want to override this to return a foreign
+	 * database object, or null if getTitleInfo() shouldn't access the database.
 	 *
-	 * NOTE: This ONLY works for getTitleMTimes() and getModifiedTime(), NOT FOR ANYTHING ELSE.
-	 * In particular, it doesn't work for getting the content of JS and CSS pages. That functionality
-	 * will use the local DB irrespective of the return value of this method.
+	 * NOTE: This ONLY works for getTitleInfo() and isKnownEmpty(), NOT FOR ANYTHING ELSE.
+	 * In particular, it doesn't work for getContent() or getScript() etc.
 	 *
 	 * @return IDatabase|null
 	 */
@@ -131,10 +144,15 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 	}
 
 	/**
-	 * @param Title $title
+	 * @param string $title
 	 * @return null|string
 	 */
-	protected function getContent( $title ) {
+	protected function getContent( $titleText ) {
+		$title = Title::newFromText( $titleText );
+		if ( !$title || $title->isRedirect() ) {
+			return null;
+		}
+
 		$handler = ContentHandler::getForTitle( $title );
 		if ( $handler->isSupportedFormat( CONTENT_FORMAT_CSS ) ) {
 			$format = CONTENT_FORMAT_CSS;
@@ -169,11 +187,7 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 			if ( $options['type'] !== 'script' ) {
 				continue;
 			}
-			$title = Title::newFromText( $titleText );
-			if ( !$title || $title->isRedirect() ) {
-				continue;
-			}
-			$script = $this->getContent( $title );
+			$script = $this->getContent( $titleText );
 			if ( strval( $script ) !== '' ) {
 				$script = $this->validateScriptFile( $titleText, $script );
 				$scripts .= ResourceLoader::makeComment( $titleText ) . $script . "\n";
@@ -192,12 +206,8 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 			if ( $options['type'] !== 'style' ) {
 				continue;
 			}
-			$title = Title::newFromText( $titleText );
-			if ( !$title || $title->isRedirect() ) {
-				continue;
-			}
 			$media = isset( $options['media'] ) ? $options['media'] : 'all';
-			$style = $this->getContent( $title );
+			$style = $this->getContent( $titleText );
 			if ( strval( $style ) === '' ) {
 				continue;
 			}
@@ -214,26 +224,6 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 		return $styles;
 	}
 
-	/**
-	 * @param ResourceLoaderContext $context
-	 * @return int
-	 */
-	public function getModifiedTime( ResourceLoaderContext $context ) {
-		$modifiedTime = 1;
-		$titleInfo = $this->getTitleInfo( $context );
-		if ( count( $titleInfo ) ) {
-			$mtimes = array_map( function ( $value ) {
-				return $value['timestamp'];
-			}, $titleInfo );
-			$modifiedTime = max( $modifiedTime, max( $mtimes ) );
-		}
-		$modifiedTime = max(
-			$modifiedTime,
-			$this->getMsgBlobMtime( $context->getLanguage() )
-		);
-		return $modifiedTime;
-	}
-
 	/**
 	 * @param ResourceLoaderContext $context
 	 * @return array
@@ -242,6 +232,8 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 		$summary = parent::getDefinitionSummary( $context );
 		$summary[] = array(
 			'pages' => $this->getPages( $context ),
+			// Includes SHA1 of content
+			'titleInfo' => $this->getTitleInfo( $context ),
 		);
 		return $summary;
 	}
@@ -251,33 +243,29 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 	 * @return bool
 	 */
 	public function isKnownEmpty( ResourceLoaderContext $context ) {
-		$titleInfo = $this->getTitleInfo( $context );
-		// Bug 68488: For modules in the "user" group, we should actually
-		// check that the pages are empty (page_len == 0), but for other
-		// groups, just check the pages exist so that we don't end up
-		// caching temporarily-blank pages without the appropriate
-		// <script> or <link> tag.
-		if ( $this->getGroup() !== 'user' ) {
-			return count( $titleInfo ) === 0;
-		}
+		$revisions = $this->getTitleInfo( $context );
 
-		foreach ( $titleInfo as $info ) {
-			if ( $info['length'] !== 0 ) {
-				// At least one non-0-lenth page, not empty
-				return false;
+		// For user modules, don't needlessly load if there are no non-empty pages
+		if ( $this->getGroup() === 'user' ) {
+			foreach ( $revisions as $revision ) {
+				if ( $revision['rev_len'] > 0 ) {
+					// At least one non-empty page, module should be loaded
+					return false;
+				}
 			}
+			return true;
 		}
 
-		// All pages are 0-length, so it's empty
-		return true;
+		// Bug 68488: For other modules (i.e. ones that are called in cached html output) only check
+		// page existance. This ensures that, if some pages in a module are temporarily blanked,
+		// we don't end omit the module's script or link tag on some pages.
+		return count( $revisions ) === 0;
 	}
 
 	/**
-	 * Get the modification times of all titles that would be loaded for
-	 * a given context.
-	 * @param ResourceLoaderContext $context Context object
-	 * @return array Keyed by page dbkey. Value is an array with 'length' and 'timestamp'
-	 *               keys, where the timestamp is a UNIX timestamp
+	 * Get the information about the wiki pages for a given context.
+	 * @param ResourceLoaderContext $context
+	 * @return array Keyed by page name. Contains arrays with 'rev_len' and 'rev_sha1' keys
 	 */
 	protected function getTitleInfo( ResourceLoaderContext $context ) {
 		$dbr = $this->getDB();
@@ -286,32 +274,36 @@ class ResourceLoaderWikiModule extends ResourceLoaderModule {
 			return array();
 		}
 
-		$hash = $context->getHash();
-		if ( isset( $this->titleInfo[$hash] ) ) {
-			return $this->titleInfo[$hash];
-		}
+		$pages = $this->getPages( $context );
+		$key = implode( '|', array_keys( $pages ) );
+		if ( !isset( $this->titleInfo[$key] ) ) {
 
-		$this->titleInfo[$hash] = array();
-		$batch = new LinkBatch;
-		foreach ( $this->getPages( $context ) as $titleText => $options ) {
-			$batch->addObj( Title::newFromText( $titleText ) );
-		}
+			$this->titleInfo[$key] = array();
+			$batch = new LinkBatch;
+			foreach ( $pages as $titleText => $options ) {
+				$batch->addObj( Title::newFromText( $titleText ) );
+			}
 
-		if ( !$batch->isEmpty() ) {
-			$res = $dbr->select( 'page',
-				array( 'page_namespace', 'page_title', 'page_touched', 'page_len' ),
-				$batch->constructSet( 'page', $dbr ),
-				__METHOD__
-			);
-			foreach ( $res as $row ) {
-				$title = Title::makeTitle( $row->page_namespace, $row->page_title );
-				$this->titleInfo[$hash][$title->getPrefixedDBkey()] = array(
-					'timestamp' => wfTimestamp( TS_UNIX, $row->page_touched ),
-					'length' => $row->page_len,
+			if ( !$batch->isEmpty() ) {
+				$res = $dbr->select( array( 'page', 'revision' ),
+					array( 'page_namespace', 'page_title', 'rev_len', 'rev_sha1' ),
+					$batch->constructSet( 'page', $dbr ),
+					__METHOD__,
+					array(),
+					array( 'revision' => array( 'INNER JOIN', array( 'page_latest=rev_id' ) ) )
 				);
+				foreach ( $res as $row ) {
+					// Avoid including ids or timestamps of revision/page tables so
+					// that versions are not wasted
+					$title = Title::makeTitle( $row->page_namespace, $row->page_title );
+					$this->titleInfo[$key][$title->getPrefixedText()] = array(
+						'rev_len' => $row->rev_len,
+						'rev_sha1' => $row->rev_sha1,
+					);
+				}
 			}
 		}
-		return $this->titleInfo[$hash];
+		return $this->titleInfo[$key];
 	}
 
 	public function getPosition() {
diff --git a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php
index 7974ee9ddd..8cefec7558 100644
--- a/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php
+++ b/tests/phpunit/includes/resourceloader/ResourceLoaderWikiModuleTest.php
@@ -109,39 +109,27 @@ class ResourceLoaderWikiModuleTest extends ResourceLoaderTestCase {
 			array( array(), 'test1', true ),
 			// 'site' module with a non-empty page
 			array(
-				array(
-					'MediaWiki:Common.js' => array(
-						'timestamp' => 123456789,
-						'length' => 1234
-					)
-				), 'site', false,
+				array( 'MediaWiki:Common.js' => array( 'rev_sha1' => 'dmh6qn', 'rev_len' => 1234 ) ),
+				'site',
+				false,
 			),
 			// 'site' module with an empty page
 			array(
-				array(
-					'MediaWiki:Monobook.js' => array(
-						'timestamp' => 987654321,
-						'length' => 0,
-					),
-				), 'site', false,
+				array( 'MediaWiki:Foo.js' => array( 'rev_sha1' => 'phoi', 'rev_len' => 0 ) ),
+				'site',
+				false,
 			),
 			// 'user' module with a non-empty page
 			array(
-				array(
-					'User:FooBar/common.js' => array(
-						'timestamp' => 246813579,
-						'length' => 25,
-					),
-				), 'user', false,
+				array( 'User:Example/common.js' => array( 'rev_sha1' => 'j7ssba', 'rev_len' => 25 ) ),
+				'user',
+				false,
 			),
 			// 'user' module with an empty page
 			array(
-				array(
-					'User:FooBar/monobook.js' => array(
-						'timestamp' => 1357924680,
-						'length' => 0,
-					),
-				), 'user', true,
+				array( 'User:Example/foo.js' => array( 'rev_sha1' => 'phoi', 'rev_len' => 0 ) ),
+				'user',
+				true,
 			),
 		);
 	}