Krinkle has uploaded a new change for review.
https://gerrit.wikimedia.org/r/90541
Change subject: resourceloader: Add definition hashing to improve cache
invalidation
......................................................................
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 remain.
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.
The most common scenarios this addresses:
* Files (scripts, styles) being re-ordered.
* Files being removed while more recently changed files remain
part of the module. (e.g. removing an older file)
* Files being added to a module containing files changed more
recently than the file being added. (e.g. you change 2 files,
one already part of the module, and then later add the second
file, the second update would be ignored).
Bug: 37812
Change-Id: I00cf086c981a84235623bf58fb83c9c23aa2d619
---
M includes/resourceloader/ResourceLoaderFileModule.php
M includes/resourceloader/ResourceLoaderModule.php
A tests/phpunit/includes/ResourceLoaderModuleTest.php
3 files changed, 161 insertions(+), 5 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/41/90541/1
diff --git a/includes/resourceloader/ResourceLoaderFileModule.php
b/includes/resourceloader/ResourceLoaderFileModule.php
index 9ed181e..2ff6dd3 100644
--- a/includes/resourceloader/ResourceLoaderFileModule.php
+++ b/includes/resourceloader/ResourceLoaderFileModule.php
@@ -241,7 +241,11 @@
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 @@
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()
+ );
wfProfileOut( __METHOD__ );
return $this->modifiedTime[$context->getHash()];
}
+ /**
+ * Get the definition summary for this module.
+ *
+ * @return Array
+ */
+ public function getDefinitionSummary() {
+ $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 11264fc..b4842f1 100644
--- a/includes/resourceloader/ResourceLoaderModule.php
+++ b/includes/resourceloader/ResourceLoaderModule.php
@@ -398,7 +398,8 @@
* 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 @@
}
/**
- * 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
@@ -436,6 +439,70 @@
}
/**
+ * Helper method for calculating when the module's definition changed.
+ * @return integer: UNIX timestamp or 0 if no definition summary was
provided
+ * by getDefinitionSummary()
+ */
+ public function getDefinitionMtime() {
+ $summary = $this->getDefinitionSummary( $context );
+ if ( $summary === null ) {
+ return 0;
+ }
+
+ $hash = md5( json_encode( $summary ) );
+
+ $cache = wfGetCache( CACHE_ANYTHING );
+ $key = wfMemcKey( 'resourceloader', 'moduleDefinition',
$this->getName() );
+
+ $data = $cache->get( $key );
+ if ( is_array( $data ) && $data['hash'] === $hash ) {
+ // Hash is still the same, re-use the timestamp of when
we first saw this hash.
+ return $data['timestamp'];
+ }
+
+ $timestamp = wfTimestamp();
+ $cache->set( $key, array(
+ 'hash' => $hash,
+ 'timestamp' => $timestamp,
+ ) );
+
+ 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 of all the significant values that should influence
the
+ * invalidation of its cache. For a module concatenating files, the
order matters.
+ * So it should include the list of file paths here in the order used.
That
+ * way, the module will expire when you re-order or remove files even
if the
+ * aggregated getModifiedTime() of all files remains the same (bug ##).
+ *
+ * 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() {
+ 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
* definitely going to be empty, it should override this method to
diff --git a/tests/phpunit/includes/ResourceLoaderModuleTest.php
b/tests/phpunit/includes/ResourceLoaderModuleTest.php
new file mode 100644
index 0000000..cf10c01
--- /dev/null
+++ b/tests/phpunit/includes/ResourceLoaderModuleTest.php
@@ -0,0 +1,50 @@
+<?php
+
+class ResourceLoaderModuleTest extends MediaWikiTestCase {
+
+ public function testDefinitionSummary() {
+ $module = new ResourceLoaderFileModule( array(
+ 'scripts' => array( 'foo.js', 'bar.js' ),
+ 'dependencies' => array( 'jquery', 'mediawiki' ),
+ 'messages' => array( 'hello', 'world' ),
+ ) );
+
+ $hash = json_encode( $module->getDefinitionSummary() );
+
+ $module = new ResourceLoaderFileModule( array(
+ 'scripts' => array( 'foo.js', 'bar.js' ),
+ 'dependencies' => array( 'mediawiki', 'jquery' ),
+ 'messages' => array( 'hello', 'world' ),
+ ) );
+
+ $this->assertEquals(
+ $hash,
+ json_encode( $module->getDefinitionSummary() ),
+ 'Order of dependencies is insignificant'
+ );
+
+ $module = new ResourceLoaderFileModule( array(
+ 'scripts' => array( 'foo.js', 'bar.js' ),
+ 'dependencies' => array( 'jquery', 'mediawiki' ),
+ 'messages' => array( 'world', 'hello' ),
+ ) );
+
+ $this->assertEquals(
+ $hash,
+ json_encode( $module->getDefinitionSummary() ),
+ 'Order of messages is insignificant'
+ );
+
+ $module = new ResourceLoaderFileModule( array(
+ 'scripts' => array( 'bar.js', 'foo.js' ),
+ 'dependencies' => array( 'jquery', 'mediawiki' ),
+ 'messages' => array( 'hello', 'world' ),
+ ) );
+
+ $this->assertNotEquals(
+ $hash,
+ json_encode( $module->getDefinitionSummary() ),
+ 'Order of scripts is significant'
+ );
+ }
+}
--
To view, visit https://gerrit.wikimedia.org/r/90541
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I00cf086c981a84235623bf58fb83c9c23aa2d619
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
