From 6fa489392815c7c0139a8a2df6447dfff24004ed Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Thu, 14 May 2015 02:31:42 +0100 Subject: [PATCH] resourceloader: Don't cache minification of user.tokens As of b1e4006b4, the tokens are different on every request. Caching these is completely useless because the cache entry is simply unreachable and is extra overhead on every request for logged-in users to save content to Memcached. Whether they should be minified at all and whether they perhaps shouldn't change on every request is a separate matter. Bug: T84960 Change-Id: I6016e4b01e44e0acbfd6d49f7d99555e2290c9cb --- includes/OutputPage.php | 4 +- includes/resourceloader/ResourceLoader.php | 95 +++++++++++-------- .../ResourceLoaderStartUpModule.php | 4 +- 3 files changed, 62 insertions(+), 41 deletions(-) diff --git a/includes/OutputPage.php b/includes/OutputPage.php index a3a5a2730a..ba9fcba1e5 100644 --- a/includes/OutputPage.php +++ b/includes/OutputPage.php @@ -2981,8 +2981,10 @@ class OutputPage extends ContextSource { // Load embeddable private modules before any loader links // This needs to be TYPE_COMBINED so these modules are properly wrapped // in mw.loader.implement() calls and deferred until mw.user is available - $embedScripts = array( 'user.options', 'user.tokens' ); + $embedScripts = array( 'user.options' ); $links[] = $this->makeResourceLoaderLink( $embedScripts, ResourceLoaderModule::TYPE_COMBINED ); + // Separate user.tokens as otherwise caching will be allowed (T84960) + $links[] = $this->makeResourceLoaderLink( 'user.tokens', ResourceLoaderModule::TYPE_COMBINED ); // Scripts and messages "only" requests marked for top inclusion $links[] = $this->makeResourceLoaderLink( diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php index 5a9465e747..b4b5a2e2c2 100644 --- a/includes/resourceloader/ResourceLoader.php +++ b/includes/resourceloader/ResourceLoader.php @@ -169,58 +169,69 @@ class ResourceLoader { * * @param string $filter Name of filter to run * @param string $data Text to filter, such as JavaScript or CSS text - * @param string $cacheReport Whether to include the cache key report + * @param array $options For back-compat, can also be the boolean value for "cacheReport". Keys: + * - (bool) cache: Whether to allow caching this data. Default: true. + * - (bool) cacheReport: Whether to include the "cache key" report comment. Default: true. * @return string Filtered data, or a comment containing an error message */ - public function filter( $filter, $data, $cacheReport = true ) { + public function filter( $filter, $data, $options = array() ) { + // Back-compat + if ( is_bool( $options ) ) { + $options = array( 'cacheReport' => $options ); + } + // Defaults + $options += array( 'cache' => true, 'cacheReport' => true ); - // For empty/whitespace-only data or for unknown filters, don't perform - // any caching or processing - if ( trim( $data ) === '' || !in_array( $filter, array( 'minify-js', 'minify-css' ) ) ) { + // Don't filter empty content + if ( trim( $data ) === '' ) { return $data; } - // Try for cache hit - // Use CACHE_ANYTHING since filtering is very slow compared to DB queries - $key = wfMemcKey( 'resourceloader', 'filter', $filter, self::$filterCacheVersion, md5( $data ) ); - $cache = wfGetCache( CACHE_ANYTHING ); - $cacheEntry = $cache->get( $key ); - if ( is_string( $cacheEntry ) ) { - wfIncrStats( "rl-$filter-cache-hits" ); - return $cacheEntry; + if ( !in_array( $filter, array( 'minify-js', 'minify-css' ) ) ) { + wfDebugLog( 'resourceloader', __METHOD__ . ": Invalid filter: $filter" ); + return $data; } - $result = ''; - // Run the filter - we've already verified one of these will work - try { - wfIncrStats( "rl-$filter-cache-misses" ); + if ( !$options['cache'] ) { + $result = $this->applyFilter( $filter, $data ); + } else { + // Use CACHE_ANYTHING since filtering is very slow compared to DB queries + $key = wfMemcKey( 'resourceloader', 'filter', $filter, self::$filterCacheVersion, md5( $data ) ); + $cache = wfGetCache( CACHE_ANYTHING ); + $cacheEntry = $cache->get( $key ); + if ( is_string( $cacheEntry ) ) { + wfIncrStats( "rl-$filter-cache-hits" ); + return $cacheEntry; + } + $result = ''; + try { + wfIncrStats( "rl-$filter-cache-misses" ); + $result = $this->applyFilter( $filter, $data ); + if ( $options['cacheReport'] ) { + $result .= "\n/* cache key: $key */"; + } + $cache->set( $key, $result ); + } catch ( Exception $e ) { + MWExceptionHandler::logException( $e ); + wfDebugLog( 'resourceloader', __METHOD__ . ": minification failed: $e" ); + $this->errors[] = self::formatExceptionNoComment( $e ); + } + } + + return $result; + } + + private function applyFilter( $filter, $data ) { switch ( $filter ) { case 'minify-js': - $result = JavaScriptMinifier::minify( $data, + return JavaScriptMinifier::minify( $data, $this->config->get( 'ResourceLoaderMinifierStatementsOnOwnLine' ), $this->config->get( 'ResourceLoaderMinifierMaxLineLength' ) ); - if ( $cacheReport ) { - $result .= "\n/* cache key: $key */"; - } - break; case 'minify-css': - $result = CSSMin::minify( $data ); - if ( $cacheReport ) { - $result .= "\n/* cache key: $key */"; - } - break; + return CSSMin::minify( $data ); } - - // Save filtered text to Memcached - $cache->set( $key, $result ); - } catch ( Exception $e ) { - MWExceptionHandler::logException( $e ); - wfDebugLog( 'resourceloader', __METHOD__ . ": minification failed: $e" ); - $this->errors[] = self::formatExceptionNoComment( $e ); - } - - return $result; + return $data; } /* Methods */ @@ -1078,11 +1089,19 @@ MESSAGE; } } + $enableFilterCache = true; + if ( count( $modules ) === 1 && reset( $modules ) instanceof ResourceLoaderUserTokensModule ) { + // If we're building the embedded user.tokens, don't cache (T84960) + $enableFilterCache = false; + } + if ( !$context->getDebug() ) { if ( $context->getOnly() === 'styles' ) { $out = $this->filter( 'minify-css', $out ); } else { - $out = $this->filter( 'minify-js', $out ); + $out = $this->filter( 'minify-js', $out, array( + 'cache' => $enableFilterCache + ) ); } } diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php b/includes/resourceloader/ResourceLoaderStartUpModule.php index 06054eee93..6c078b0ab9 100644 --- a/includes/resourceloader/ResourceLoaderStartUpModule.php +++ b/includes/resourceloader/ResourceLoaderStartUpModule.php @@ -217,9 +217,9 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule { if ( $skipFunction !== null && !ResourceLoader::inDebugMode() ) { $skipFunction = $resourceLoader->filter( 'minify-js', $skipFunction, - // There will potentially be lots of these little string in the registrations + // There will potentially be lots of these little strings in the registrations // manifest, we don't want to blow up the startup module with - // "/* cache key: ... */" all over it in non-debug mode. + // "/* cache key: ... */" all over it. /* cacheReport = */ false ); } -- 2.20.1