Thijs/wiki.techinc.nl
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

resourceloader: Skip version hash calculation in debug mode

Browse source 
=== Why

* More speed

  In debug mode, the server should regenerate the startup manifest
  on each page view to ensure immediate effect of changes. But,
  this also means more version recomputation work on the server.

  For most modules, this was already quite fast on repeat views
  because of OS-level file caches, and our file-hash caches and
  LESS compile caches in php-apcu from ResourceLoader.
  But, this makes it even faster.

* Better integration with browser devtools.

  Breakpoints stay more consistently across browsers when the
  URL stays the same even after you have changed the file and
  reloaded the page. For static files, I believe most browsers ignore
  query parameters. But for package files that come from load.php,
  this was harder for browsers to guess correctly which old script URL
  is logically replaced by a different one on the next page view.

=== How

Change Module::getVersionHash to return empty strings in debug mode.

I considered approaching this from StartupModule::getModuleRegistrations
instead to make the change apply only to the client-side manifest.

I decided against this because we have other calls to getVersionHash
on the server-side (such as for E-Tag calculation, and formatting
cross-wiki URLs) which would then not match the version queries that
mw.loader formats in debug mode.

Also, those calls would still be incurring some the avoidable costs.

=== Notes

* The two test cases for verifying the graceful fallback in production
  if version hash computations throw an exception, were moved to a
  non-debug test case as no longer happen now during the debug
  (unminified) test cases.

* Avoid "PHP Notice: Undefined offset 0" in testMakeModuleResponseStartupError
  by adding a fallback to empty string so that if the test fails,
  it fails in a more useful way instead of aborting with this error
  before the assertion happens. (Since PHPUnit generally stops on the
  first error.)

* In practice, there are still "version" query parameters and E-Tag
  headers in debug mode. These are not module versions, but URL
  "combined versions" crafted by getCombinedVersion() in JS and PHP.
  These return the constant "ztntf" in debug mode, which is the hash
  of an empty string. We could alter these methods to special-case
  when all inputs are and join to a still-empty string, or maybe we
  just leave them be. I've done the latter for now.

Bug: T235672
Bug: T85805
Change-Id: I0e63eef4f85b13089a0aa3806a5b6f821d527a92
This commit is contained in:
Timo Tijhof 2021-08-28 03:53:36 +01:00 committed by Roan Kattouw
parent 7c6713b4d9
commit 008b6528b6
7 changed files with 136 additions and 128 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
includes/resourceloader/ResourceLoaderModule.php
View file

@ -857,6 +857,14 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface {
* @return string Hash formatted by ResourceLoader::makeHash
*/
final public function getVersionHash( ResourceLoaderContext $context ) {
if ( $context->getDebug() ) {
// In debug mode, make uncached startup module extra fast by not computing any hashes.
// Server responses from load.php for individual modules already have no-cache so
// we don't need them. This also makes breakpoint debugging easier, as each module
// gets its own consistent URL. (T235672)
return '';
}
// Cache this somewhat expensive operation. Especially because some classes
// (e.g. startup module) iterate more than once over all modules to get versions.
$contextHash = $context->getHash();

16
includes/resourceloader/ResourceLoaderStartUpModule.php
View file

@ -205,7 +205,7 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
try {
// The version should be formatted by ResourceLoader::makeHash and be of
// length ResourceLoader::HASH_LENGTH.
// length ResourceLoader::HASH_LENGTH (or empty string).
// The getVersionHash method is final and is covered by tests, as is makeHash().
$versionHash = $module->getVersionHash( $context );
} catch ( Exception $e ) {
@ -222,20 +222,6 @@ class ResourceLoaderStartUpModule extends ResourceLoaderModule {
$states[$name] = 'error';
}
if ( $versionHash !== '' && strlen( $versionHash ) !== ResourceLoader::HASH_LENGTH ) {
$e = new RuntimeException( "Badly formatted module version hash" );
$resourceLoader->outputErrorAndLog( $e,
"Module '{module}' produced an invalid version hash: '{version}'.",
[
'module' => $name,
'version' => $versionHash,
]
);
// Module implementation either broken or deviated from ResourceLoader::makeHash
// Asserted by tests/phpunit/structure/ResourcesTest.
$versionHash = ResourceLoader::makeHash( $versionHash );
}
$skipFunction = $module->getSkipFunction();
if ( $skipFunction !== null && !$context->getDebug() ) {
$skipFunction = ResourceLoader::filter( 'minify-js', $skipFunction );

2
resources/src/startup/mediawiki.loader.js
View file

@ -328,6 +328,8 @@
}
/**
* See also `ResourceLoader.php#makeVersionQuery` on the server.
*
* @private
* @param {string[]} modules List of module names
* @return {string} Hash of concatenated version hashes.

2
tests/phpunit/includes/resourceloader/ResourceLoaderFileModuleTest.php
View file

@ -571,7 +571,7 @@ class ResourceLoaderFileModuleTest extends ResourceLoaderTestCase {
* @covers ResourceLoaderFileModule::getFileHashes
*/
public function testGetVersionHash( $a, $b, $isEqual ) {
$context = $this->getResourceLoaderContext();
$context = $this->getResourceLoaderContext( [ 'debug' => 'false' ] );
$moduleA = new ResourceLoaderFileTestModule( $a );
$moduleA->setConfig( $context->getResourceLoader()->getConfig() );

6
tests/phpunit/includes/resourceloader/ResourceLoaderModuleTest.php
View file

@ -6,7 +6,7 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase {
* @covers ResourceLoaderModule::getVersionHash
*/
public function testGetVersionHash() {
$context = $this->getResourceLoaderContext();
$context = $this->getResourceLoaderContext( [ 'debug' => 'false' ] );
$baseParams = [
'scripts' => [ 'foo.js', 'bar.js' ],
@ -68,7 +68,7 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase {
* @covers ResourceLoaderModule::getVersionHash
*/
public function testGetVersionHash_length() {
$context = $this->getResourceLoaderContext();
$context = $this->getResourceLoaderContext( [ 'debug' => 'false' ] );
$module = new ResourceLoaderTestModule( [
'script' => 'foo();'
] );
@ -80,7 +80,7 @@ class ResourceLoaderModuleTest extends ResourceLoaderTestCase {
* @covers ResourceLoaderModule::getVersionHash
*/
public function testGetVersionHash_parentDefinition() {
$context = $this->getResourceLoaderContext();
$context = $this->getResourceLoaderContext( [ 'debug' => 'false' ] );
$module = $this->getMockBuilder( ResourceLoaderModule::class )
->onlyMethods( [ 'getDefinitionSummary' ] )->getMock();
$module->method( 'getDefinitionSummary' )->willReturn( [ 'a' => 'summary' ] );

224
tests/phpunit/includes/resourceloader/ResourceLoaderStartUpModuleTest.php
View file

@ -31,7 +31,7 @@ mw.loader.addSource({
mw.loader.register([
[
"test.blank",
"{blankVer}"
""
]
]);',
] ],
@ -62,7 +62,7 @@ mw.loader.addSource({
mw.loader.register([
[
"a",
"{blankVer}",
"",
[
1,
3
@ -70,18 +70,18 @@ mw.loader.register([
],
[
"b",
"{blankVer}",
"",
[
2
]
],
[
"c",
"{blankVer}"
""
],
[
"d",
"{blankVer}"
""
]
]);',
] ],
@ -108,7 +108,7 @@ mw.loader.addSource({
mw.loader.register([
[
"a",
"{blankVer}",
"",
[
1,
"x"
@ -116,7 +116,7 @@ mw.loader.register([
],
[
"b",
"{blankVer}",
"",
[
2,
"x"
@ -124,7 +124,7 @@ mw.loader.register([
],
[
"c",
"{blankVer}"
""
]
]);',
] ],
@ -160,7 +160,7 @@ mw.loader.addSource({
mw.loader.register([
[
"top",
"{blankVer}",
"",
[
1,
4
@ -168,7 +168,7 @@ mw.loader.register([
],
[
"middle1",
"{blankVer}",
"",
[
2,
4
@ -176,21 +176,21 @@ mw.loader.register([
],
[
"middle2",
"{blankVer}",
"",
[
3
]
],
[
"bottom",
"{blankVer}",
"",
[
0
]
],
[
"util",
"{blankVer}"
""
]
]);',
] ],
@ -214,7 +214,7 @@ mw.loader.addSource({
mw.loader.register([
[
"top",
"{blankVer}",
"",
[
1,
0
@ -222,70 +222,9 @@ mw.loader.register([
],
[
"util",
"{blankVer}"
""
]
]);',
] ],
[ [
'msg' => 'Version falls back gracefully if getModuleContent throws',
'modules' => [
'test.fail' => [
'factory' => function () {
$mock = $this->getMockBuilder( ResourceLoaderTestModule::class )
->onlyMethods( [ 'getModuleContent' ] )->getMock();
$mock->method( 'getModuleContent' )->will(
$this->throwException( new Exception )
);
return $mock;
}
]
],
'out' => '
mw.loader.addSource({
"local": "/w/load.php"
});
mw.loader.register([
[
"test.fail",
""
]
]);
mw.loader.state({
"test.fail": "error"
});',
] ],
[ [
'msg' => 'Version falls back gracefully if getDefinitionSummary throws',
'modules' => [
'test.fail' => [
'factory' => function () {
$mock = $this->getMockBuilder( ResourceLoaderTestModule::class )
->onlyMethods( [
'enableModuleContentVersion',
'getDefinitionSummary'
] )
->getMock();
$mock->method( 'enableModuleContentVersion' )->willReturn( false );
$mock->method( 'getDefinitionSummary' )->will(
$this->throwException( new Exception )
);
return $mock;
}
]
],
'out' => '
mw.loader.addSource({
"local": "/w/load.php"
});
mw.loader.register([
[
"test.fail",
""
]
]);
mw.loader.state({
"test.fail": "error"
});',
] ],
[ [
'msg' => 'Group signature',
@ -307,17 +246,17 @@ mw.loader.addSource({
mw.loader.register([
[
"test.blank",
"{blankVer}"
""
],
[
"test.group.foo",
"{blankVer}",
"",
[],
2
],
[
"test.group.bar",
"{blankVer}",
"",
[],
3
]
@ -339,7 +278,7 @@ mw.loader.addSource({
mw.loader.register([
[
"test.blank",
"{blankVer}"
""
]
]);'
] ],
@ -368,19 +307,19 @@ mw.loader.addSource({
mw.loader.register([
[
"test.blank",
"{blankVer}"
""
],
[
"test.core-generated",
"{blankVer}"
""
],
[
"test.sitewide",
"{blankVer}"
""
],
[
"test.user",
"{blankVer}"
""
]
]);'
] ],
@ -410,11 +349,11 @@ mw.loader.addSource({
mw.loader.register([
[
"test.blank",
"{blankVer}"
""
],
[
"test.core-generated",
"{blankVer}"
""
]
]);'
] ],
@ -440,7 +379,7 @@ mw.loader.addSource({
mw.loader.register([
[
"test.blank",
"{blankVer}",
"",
[],
null,
"example"
@ -480,11 +419,11 @@ mw.loader.addSource({
mw.loader.register([
[
"test.x.core",
"{blankVer}"
""
],
[
"test.x.polyfill",
"{blankVer}",
"",
[],
null,
null,
@ -492,7 +431,7 @@ mw.loader.register([
],
[
"test.y.polyfill",
"{blankVer}",
"",
[],
null,
null,
@ -500,7 +439,7 @@ mw.loader.register([
],
[
"test.z.foo",
"{blankVer}",
"",
[
0,
1,
@ -524,7 +463,7 @@ mw.loader.addSource({
mw.loader.register([
[
"test.es6",
"{blankVer}!"
"!"
]
]);',
] ],
@ -610,36 +549,36 @@ mw.loader.addSource({
mw.loader.register([
[
"test.blank",
"{blankVer}"
""
],
[
"test.x.core",
"{blankVer}"
""
],
[
"test.x.util",
"{blankVer}",
"",
[
1
]
],
[
"test.x.foo",
"{blankVer}",
"",
[
1
]
],
[
"test.x.bar",
"{blankVer}",
"",
[
2
]
],
[
"test.x.quux",
"{blankVer}",
"",
[
3,
4,
@ -648,32 +587,32 @@ mw.loader.register([
],
[
"test.group.foo.1",
"{blankVer}",
"",
[],
2
],
[
"test.group.foo.2",
"{blankVer}",
"",
[],
2
],
[
"test.group.bar.1",
"{blankVer}",
"",
[],
3
],
[
"test.group.bar.2",
"{blankVer}",
"",
[],
3,
"example"
],
[
"test.es6",
"{blankVer}!"
"!"
]
]);'
] ],
@ -708,6 +647,75 @@ mw.loader.register([
);
}
/**
* These test cases test behaviour that are specific to production mode.
*
* @see provideGetModuleRegistrations
*/
public function provideGetModuleRegistrationsProduction() {
yield 'Version falls back gracefully if getModuleContent throws' => [ [
'modules' => [
'test.fail' => [
'factory' => function () {
$mock = $this->getMockBuilder( ResourceLoaderTestModule::class )
->onlyMethods( [ 'getModuleContent' ] )->getMock();
$mock->method( 'getModuleContent' )->will(
$this->throwException( new Exception )
);
return $mock;
}
]
],
'out' => 'mw.loader.addSource({"local":"/w/load.php"});' . "\n"
. 'mw.loader.register([["test.fail",""]]);' . "\n"
. 'mw.loader.state({"test.fail":"error"});',
] ];
yield 'Version falls back gracefully if getDefinitionSummary throws' => [ [
'modules' => [
'test.fail' => [
'factory' => function () {
$mock = $this->getMockBuilder( ResourceLoaderTestModule::class )
->onlyMethods( [
'enableModuleContentVersion',
'getDefinitionSummary'
] )
->getMock();
$mock->method( 'enableModuleContentVersion' )->willReturn( false );
$mock->method( 'getDefinitionSummary' )->will(
$this->throwException( new Exception )
);
return $mock;
}
]
],
'out' => 'mw.loader.addSource({"local":"/w/load.php"});' . "\n"
. 'mw.loader.register([["test.fail",""]]);' . "\n"
. 'mw.loader.state({"test.fail":"error"});',
] ];
}
/**
* @dataProvider provideGetModuleRegistrationsProduction
* @covers ResourceLoaderStartUpModule
* @covers ResourceLoader
*/
public function testGetModuleRegistrationsProduction( array $case ) {
$context = $this->getResourceLoaderContext( [ 'debug' => 'false' ] );
$rl = $context->getResourceLoader();
$rl->register( $case['modules'] );
$module = new ResourceLoaderStartUpModule();
$module->setConfig( $rl->getConfig() );
$out = ltrim( $case['out'], "\n" );
// Tolerate exception logs for cases that expect getVersionHash() to throw.
$this->setLogger( 'exception', new Psr\Log\NullLogger() );
$this->assertEquals(
self::expandPlaceholders( $out ),
$module->getModuleRegistrations( $context )
);
}
public static function provideRegistrations() {
return [
[ [
@ -773,11 +781,11 @@ mw.loader.register([
mw.loader.register([
[
"test.blank",
"{blankVer}"
""
],
[
"test.min",
"{blankVer}",
"",
[
0
],
@ -819,7 +827,9 @@ mw.loader.register([
* @covers ResourceLoaderStartupModule
*/
public function testGetVersionHash_varyModule() {
$context1 = $this->getResourceLoaderContext();
$context1 = $this->getResourceLoaderContext( [
'debug' => 'false',
] );
$rl1 = $context1->getResourceLoader();
$rl1->register( [
'test.a' => [ 'class' => ResourceLoaderTestModule::class ],
@ -870,7 +880,7 @@ mw.loader.register([
* @covers ResourceLoaderStartUpModule
*/
public function testGetVersionHash_varyDeps() {
$context = $this->getResourceLoaderContext();
$context = $this->getResourceLoaderContext( [ 'debug' => 'false' ] );
$rl = $context->getResourceLoader();
$rl->register( [
'test.a' => [

6
tests/phpunit/includes/resourceloader/ResourceLoaderTest.php
View file

@ -788,7 +788,7 @@ END
],
'bar' => [ 'class' => ResourceLoaderTestModule::class ],
] );
$context = $this->getResourceLoaderContext( [], $rl );
$context = $this->getResourceLoaderContext( [ 'debug' => 'false' ], $rl );
$this->assertSame(
'',
@ -1008,6 +1008,8 @@ END
[
'modules' => 'startup',
'only' => 'scripts',
// No module build for version hash in debug mode
'debug' => 'false',
],
$rl
);
@ -1025,7 +1027,7 @@ END
$response = $rl->makeModuleResponse( $context, $modules );
$errors = $rl->getErrors();
$this->assertRegExp( '/Ferry not found/', $errors[0] );
$this->assertRegExp( '/Ferry not found/', $errors[0] ?? '' );
$this->assertCount( 1, $errors );
$this->assertRegExp(
'/isCompatible.*window\.RLQ/s',