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

resourceloader: Remove isFileModule() overhead from web requests

Browse source 
About 1.5% of load.php wall-time is spent in isFileModule() calls
during the ServiceWiring/getResourceLoader/register call early on.

Reduce the overhead of this cost by moving that logic to the Module
class.

There are two costs that we save this way:

1. The inherent cost of applying the skinStyles.

   This is now limited to only the modules that are constructed in a
   given web request. Thus, apart from the startup response (which
   constructs all modules), for regular load.php requests and all
   index.php page views, the vast majority of modules will never need
   to be constructed, and thus won't pay this cost.

2. The overhead of predicting (and class-loading) for whether a module
   is (or will become) a FileModule object.

   This is what isFileModule() does and is the main reason I wrote
   this patch. It involves class loading, and checks and conditions that
   run 1000+ times at WMF. This is eliminated now because we no longer
   have to calculate this decision. Instead, the logic applies when
   it needs to (due to FileModule implementing it), and doesn't
   when it doesn't!

Change-Id: Ia2db14f930800c96e767b94ef62fb00e9d52725b
This commit is contained in:
Timo Tijhof 2021-08-11 19:53:15 +01:00 committed by Catrope
parent 76245d70ff
commit 1367e356e7
4 changed files with 49 additions and 117 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

57
includes/resourceloader/ResourceLoader.php
View file

@ -359,42 +359,6 @@ class ResourceLoader implements LoggerAwareInterface {
// Attach module
$this->moduleInfos[$name] = $info;
// Last-minute changes
// Apply custom skin-defined styles to existing modules.
if ( $this->isFileModule( $name ) ) {
foreach ( $this->moduleSkinStyles as $skinName => $skinStyles ) {
// If this module already defines skinStyles for this skin, ignore ResourceModuleSkinStyles.
if ( isset( $this->moduleInfos[$name]['skinStyles'][$skinName] ) ) {
continue;
}
// If $name is preceded with a '+', the defined style files will be added to 'default'
// skinStyles, otherwise 'default' will be ignored as it normally would be.
if ( isset( $skinStyles[$name] ) ) {
$paths = (array)$skinStyles[$name];
$styleFiles = [];
} elseif ( isset( $skinStyles['+' . $name] ) ) {
$paths = (array)$skinStyles['+' . $name];
$styleFiles = isset( $this->moduleInfos[$name]['skinStyles']['default'] ) ?
(array)$this->moduleInfos[$name]['skinStyles']['default'] :
[];
} else {
continue;
}
// Add new file paths, remapping them to refer to our directories and not use settings
// from the module we're modifying, which come from the base definition.
list( $localBasePath, $remoteBasePath ) =
ResourceLoaderFileModule::extractBasePaths( $skinStyles );
foreach ( $paths as $path ) {
$styleFiles[] = new ResourceLoaderFilePath( $path, $localBasePath, $remoteBasePath );
}
$this->moduleInfos[$name]['skinStyles'][$skinName] = $styleFiles;
}
}
}
}
@ -538,6 +502,7 @@ class ResourceLoader implements LoggerAwareInterface {
[ $this, 'loadModuleDependenciesInternal' ],
[ $this, 'saveModuleDependenciesInternal' ]
);
$object->setSkinStylesOverride( $this->moduleSkinStyles );
$this->modules[$name] = $object;
}
@ -615,26 +580,6 @@ class ResourceLoader implements LoggerAwareInterface {
}
}
/**
* Whether the module is a ResourceLoaderFileModule or subclass thereof.
*
* @param string $name Module name
* @return bool
*/
protected function isFileModule( $name ) {
if ( !isset( $this->moduleInfos[$name] ) ) {
return false;
}
$info = $this->moduleInfos[$name];
return !isset( $info['factory'] ) && (
// The implied default for 'class' is ResourceLoaderFileModule
!isset( $info['class'] ) ||
// Explicit default
$info['class'] === ResourceLoaderFileModule::class ||
is_subclass_of( $info['class'], ResourceLoaderFileModule::class )
);
}
/**
* Get the list of sources.
*

35
includes/resourceloader/ResourceLoaderFileModule.php
View file

@ -837,6 +837,41 @@ class ResourceLoaderFileModule extends ResourceLoaderModule {
return [];
}
public function setSkinStylesOverride( array $moduleSkinStyles ): void {
$moduleName = $this->getName();
foreach ( $moduleSkinStyles as $skinName => $overrides ) {
// If a module provides overrides for a skin, and that skin also provides overrides
// for the same module, then the module has precedence.
if ( isset( $this->skinStyles[$skinName] ) ) {
continue;
}
// If $moduleName in ResourceModuleSkinStyles is preceded with a '+', the defined style
// files will be added to 'default' skinStyles, otherwise 'default' will be ignored.
if ( isset( $overrides[$moduleName] ) ) {
$paths = (array)$overrides[$moduleName];
$styleFiles = [];
} elseif ( isset( $overrides['+' . $moduleName] ) ) {
$paths = (array)$overrides['+' . $moduleName];
$styleFiles = isset( $this->skinStyles['default'] ) ?
(array)$this->skinStyles['default'] :
[];
} else {
continue;
}
// Add new file paths, remapping them to refer to our directories and not use settings
// from the module we're modifying, which come from the base definition.
list( $localBasePath, $remoteBasePath ) = self::extractBasePaths( $overrides );
foreach ( $paths as $path ) {
$styleFiles[] = new ResourceLoaderFilePath( $path, $localBasePath, $remoteBasePath );
}
$this->skinStyles[$skinName] = $styleFiles;
}
}
/**
* Get a list of file paths for all styles in this module, in order of proper inclusion.
*

13
includes/resourceloader/ResourceLoaderModule.php
View file

@ -126,6 +126,19 @@ abstract class ResourceLoaderModule implements LoggerAwareInterface {
$this->name = $name;
}
/**
* Provide overrides for skinStyles to modules that support that.
*
* This MUST be called after self::setName().
*
* @since 1.37
* @see $wgResourceModuleSkinStyles
* @param array $moduleSkinStyles
*/
public function setSkinStylesOverride( array $moduleSkinStyles ): void {
// Stub, only supported by FileModule currently.
}
/**
* Inject the functions that load/save the indirect file path dependency list from storage
*

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

@ -150,67 +150,6 @@ class ResourceLoaderTest extends ResourceLoaderTestCase {
);
}
public function provideTestIsFileModule() {
$fileModuleObj = $this->createMock( ResourceLoaderFileModule::class );
return [
'factory ignored' => [ false,
[
'factory' => static function () {
return new ResourceLoaderTestModule();
}
]
],
'factory ignored (actual FileModule)' => [ false,
[
'factory' => static function () use ( $fileModuleObj ) {
return $fileModuleObj;
}
]
],
'simple empty' => [ true,
[]
],
'simple scripts' => [ true,
[ 'scripts' => 'example.js' ]
],
'simple scripts with targets' => [ true, [
'scripts' => [ 'a.js', 'b.js' ],
'targets' => [ 'desktop', 'mobile' ],
] ],
'FileModule' => [ true,
[ 'class' => ResourceLoaderFileModule::class, 'scripts' => 'example.js' ]
],
'TestModule' => [ false,
[ 'class' => ResourceLoaderTestModule::class, 'scripts' => 'example.js' ]
],
'SkinModule (FileModule subclass)' => [ true,
[ 'class' => ResourceLoaderSkinModule::class, 'scripts' => 'example.js' ]
],
'WikiModule' => [ false, [
'class' => ResourceLoaderWikiModule::class,
'scripts' => [ 'MediaWiki:Example.js' ],
] ],
];
}
/**
* @dataProvider provideTestIsFileModule
* @covers ResourceLoader::isFileModule
*/
public function testIsFileModule( $expected, $module ) {
$rl = TestingAccessWrapper::newFromObject( new EmptyResourceLoader() );
$rl->register( 'test', $module );
$this->assertSame( $expected, $rl->isFileModule( 'test' ) );
}
/**
* @covers ResourceLoader::isFileModule
*/
public function testIsFileModuleUnknown() {
$rl = TestingAccessWrapper::newFromObject( new EmptyResourceLoader() );
$this->assertSame( false, $rl->isFileModule( 'unknown' ) );
}
/**
* @covers ResourceLoader::isModuleRegistered
*/