From d3bdda32211425d2ecd2deb8eee5101dc068ee25 Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 18 Oct 2013 15:34:50 +0200 Subject: [PATCH] resourceloader: Add definition hashing to improve cache invalidation Currently we invalidate module caches based on timestamps from various places (including message blob, file mtimes and more). This meant that when a module changes such that the maximum detectable timestamp is still the same, the cache would not invalidate. Module classes can now implement a method to build a summary. In most cases this should be (a normalised version of) the definition array originally given to ResourceLoader::register (such as $wgResourceModules items). The most common scenarios this addresses: * File lists (scripts, styles) being re-ordered. * Files being removed when more recently changed files remain part of the module (e.g. removing an older file). * Files or messages being added to a module while other more recently changed things are already part of the module. E.g. you create a message and use it in a js file, but forget to add the message to the module. Then later you add the msg key. This last update would be ignored, because the mtime of the message is no newer than the total which already included the (same) mtime of the js file added in the previous update. * Change the concatenation order of pages in a Gadget definition. Bug: 37812 Change-Id: I00cf086c981a84235623bf58fb83c9c23aa2d619 --- RELEASE-NOTES-1.23 | 2 + .../ResourceLoaderFileModule.php | 43 ++++++++- .../resourceloader/ResourceLoaderModule.php | 82 ++++++++++++++++- .../ResourceLoaderWikiModule.php | 18 +++- .../includes/ResourceLoaderModuleTest.php | 87 +++++++++++++++++++ 5 files changed, 226 insertions(+), 6 deletions(-) create mode 100644 tests/phpunit/includes/ResourceLoaderModuleTest.php diff --git a/RELEASE-NOTES-1.23 b/RELEASE-NOTES-1.23 index 2d89dd2f04..d4c413e916 100644 --- a/RELEASE-NOTES-1.23 +++ b/RELEASE-NOTES-1.23 @@ -53,6 +53,8 @@ production. * Classes TitleListDependency and TitleDependency have been removed, as they have been found unused in core and extensions for a long time. * (bug 57098) SpecialPasswordReset now obeys returnto parameter +* (bug 37812) ResourceLoader will notice when a module's definition changes and + recompile it accordingly. === API changes in 1.23 === * (bug 54884) action=parse&prop=categories now indicates hidden and missing diff --git a/includes/resourceloader/ResourceLoaderFileModule.php b/includes/resourceloader/ResourceLoaderFileModule.php index 8a4847802c..43330dabb4 100644 --- a/includes/resourceloader/ResourceLoaderFileModule.php +++ b/includes/resourceloader/ResourceLoaderFileModule.php @@ -241,7 +241,11 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { case 'dependencies': case 'messages': case 'targets': - $this->{$member} = (array)$option; + // Normalise + $option = array_values( array_unique( (array)$option ) ); + sort( $option ); + + $this->{$member} = $option; break; // Single strings case 'group': @@ -457,14 +461,49 @@ class ResourceLoaderFileModule extends ResourceLoaderModule { wfProfileIn( __METHOD__ . '-filemtime' ); $filesMtime = max( array_map( array( __CLASS__, 'safeFilemtime' ), $files ) ); wfProfileOut( __METHOD__ . '-filemtime' ); + $this->modifiedTime[$context->getHash()] = max( $filesMtime, - $this->getMsgBlobMtime( $context->getLanguage() ) ); + $this->getMsgBlobMtime( $context->getLanguage() ), + $this->getDefinitionMtime( $context ) + ); wfProfileOut( __METHOD__ ); return $this->modifiedTime[$context->getHash()]; } + /** + * Get the definition summary for this module. + * + * @return Array + */ + public function getDefinitionSummary( ResourceLoaderContext $context ) { + $summary = array( + 'class' => get_class( $this ), + ); + foreach ( array( + 'scripts', + 'debugScripts', + 'loaderScripts', + 'styles', + 'languageScripts', + 'skinScripts', + 'skinStyles', + 'dependencies', + 'messages', + 'targets', + 'group', + 'position', + 'localBasePath', + 'remoteBasePath', + 'debugRaw', + 'raw', + ) as $member ) { + $summary[$member] = $this->{$member}; + }; + return $summary; + } + /* Protected Methods */ /** diff --git a/includes/resourceloader/ResourceLoaderModule.php b/includes/resourceloader/ResourceLoaderModule.php index 11264fc87c..26092af324 100644 --- a/includes/resourceloader/ResourceLoaderModule.php +++ b/includes/resourceloader/ResourceLoaderModule.php @@ -398,7 +398,8 @@ abstract class ResourceLoaderModule { * Helper method for calculating when the module's hash (if it has one) changed. * * @param ResourceLoaderContext $context - * @return integer: UNIX timestamp or 0 if there is no hash provided + * @return integer: UNIX timestamp or 0 if no hash was provided + * by getModifiedHash() */ public function getHashMtime( ResourceLoaderContext $context ) { $hash = $this->getModifiedHash( $context ); @@ -425,8 +426,10 @@ abstract class ResourceLoaderModule { } /** - * Get the last modification timestamp of the message blob for this - * module in a given language. + * Get the hash for whatever this module may contain. + * + * This is the method subclasses should implement if they want to make + * use of getHashMTime() inside getModifiedTime(). * * @param ResourceLoaderContext $context * @return string|null: Hash @@ -435,6 +438,79 @@ abstract class ResourceLoaderModule { return null; } + /** + * Helper method for calculating when this module's definition summary was last changed. + * + * @return integer: UNIX timestamp or 0 if no definition summary was provided + * by getDefinitionSummary() + */ + public function getDefinitionMtime( ResourceLoaderContext $context ) { + wfProfileIn( __METHOD__ ); + $summary = $this->getDefinitionSummary( $context ); + if ( $summary === null ) { + return 0; + } + + $hash = md5( json_encode( $summary ) ); + + $cache = wfGetCache( CACHE_ANYTHING ); + + // Embed the hash itself in the cache key. This allows for a few nifty things: + // - During deployment, servers with old and new versions of the code communicating + // with the same memcached will not override the same key repeatedly increasing + // the timestamp. + // - In case of the definition changing and then changing back in a short period of time + // (e.g. in case of a revert or a corrupt server) the old timestamp and client-side cache + // url will be re-used. + // - If different context-combinations (e.g. same skin, same language or some combination + // thereof) result in the same definition, they will use the same hash and timestamp. + $key = wfMemcKey( 'resourceloader', 'moduledefinition', $this->getName(), $hash ); + + $data = $cache->get( $key ); + if ( is_int( $data ) && $data > 0 ) { + // We've seen this hash before, re-use the timestamp of when we first saw it. + return $data; + } + + wfDebugLog( 'resourceloader', __METHOD__ . ": New definition hash for module {$this->getName()} in context {$context->getHash()}: $hash." ); + + $timestamp = time(); + $cache->set( $key, $timestamp ); + + wfProfileOut( __METHOD__ ); + return $timestamp; + } + + /** + * Get the definition summary for this module. + * + * This is the method subclasses should implement if they want to make + * use of getDefinitionMTime() inside getModifiedTime(). + * + * Return an array containing values from all significant properties of this + * module's definition. Be sure to include things that are explicitly ordered, + * in their actaul order (bug 37812). + * + * Avoid including things that are insiginificant (e.g. order of message + * keys is insignificant and should be sorted to avoid unnecessary cache + * invalidation). + * + * Avoid including things already considered by other methods inside your + * getModifiedTime(), such as file mtime timestamps. + * + * Serialisation is done using json_encode, which means object state is not + * taken into account when building the hash. This data structure must only + * contain arrays and scalars as values (avoid object instances) which means + * it requires abstraction. + * + * @return Array|null + */ + public function getDefinitionSummary( ResourceLoaderContext $context ) { + return array( + 'class' => get_class( $this ), + ); + } + /** * Check whether this module is known to be empty. If a child class * has an easy and cheap way to determine that this module is diff --git a/includes/resourceloader/ResourceLoaderWikiModule.php b/includes/resourceloader/ResourceLoaderWikiModule.php index 3f10ae5332..2653f76497 100644 --- a/includes/resourceloader/ResourceLoaderWikiModule.php +++ b/includes/resourceloader/ResourceLoaderWikiModule.php @@ -180,10 +180,26 @@ abstract class ResourceLoaderWikiModule extends ResourceLoaderModule { if ( count( $mtimes ) ) { $modifiedTime = max( $modifiedTime, max( $mtimes ) ); } - $modifiedTime = max( $modifiedTime, $this->getMsgBlobMtime( $context->getLanguage() ) ); + $modifiedTime = max( + $modifiedTime, + $this->getMsgBlobMtime( $context->getLanguage() ), + $this->getDefinitionMtime( $context ) + ); return $modifiedTime; } + /** + * Get the definition summary for this module. + * + * @return Array + */ + public function getDefinitionSummary( ResourceLoaderContext $context ) { + return array( + 'class' => get_class( $this ), + 'pages' => $this->getPages( $context ), + ); + } + /** * @param $context ResourceLoaderContext * @return bool diff --git a/tests/phpunit/includes/ResourceLoaderModuleTest.php b/tests/phpunit/includes/ResourceLoaderModuleTest.php new file mode 100644 index 0000000000..46433191fb --- /dev/null +++ b/tests/phpunit/includes/ResourceLoaderModuleTest.php @@ -0,0 +1,87 @@ + 'false', + 'lang' => 'en', + 'modules' => 'startup', + 'only' => 'scripts', + 'skin' => 'vector', + ) ); + return new ResourceLoaderContext( $resourceLoader, $request ); + } + + /** + * @covers ResourceLoaderModule::getDefinitionSummary + * @covers ResourceLoaderFileModule::getDefinitionSummary + */ + public function testDefinitionSummary() { + $context = self::getResourceLoaderContext(); + + $baseParams = array( + 'scripts' => array( 'foo.js', 'bar.js' ), + 'dependencies' => array( 'jquery', 'mediawiki' ), + 'messages' => array( 'hello', 'world' ), + ); + + $module = new ResourceLoaderFileModule( $baseParams ); + + $jsonSummary = json_encode( $module->getDefinitionSummary( $context ) ); + + // Exactly the same + $module = new ResourceLoaderFileModule( $baseParams ); + + $this->assertEquals( + $jsonSummary, + json_encode( $module->getDefinitionSummary( $context ) ), + 'Instance is insignificant' + ); + + // Re-order dependencies + $module = new ResourceLoaderFileModule( array( + 'dependencies' => array( 'mediawiki', 'jquery' ), + ) + $baseParams ); + + $this->assertEquals( + $jsonSummary, + json_encode( $module->getDefinitionSummary( $context ) ), + 'Order of dependencies is insignificant' + ); + + // Re-order messages + $module = new ResourceLoaderFileModule( array( + 'messages' => array( 'world', 'hello' ), + ) + $baseParams ); + + $this->assertEquals( + $jsonSummary, + json_encode( $module->getDefinitionSummary( $context ) ), + 'Order of messages is insignificant' + ); + + // Re-order scripts + $module = new ResourceLoaderFileModule( array( + 'scripts' => array( 'bar.js', 'foo.js' ), + ) + $baseParams ); + + $this->assertNotEquals( + $jsonSummary, + json_encode( $module->getDefinitionSummary( $context ) ), + 'Order of scripts is significant' + ); + + // Subclass + $module = new ResourceLoaderFileModuleTestModule( $baseParams ); + + $this->assertNotEquals( + $jsonSummary, + json_encode( $module->getDefinitionSummary( $context ) ), + 'Class is significant' + ); + } +} + +class ResourceLoaderFileModuleTestModule extends ResourceLoaderFileModule {} -- 2.20.1