Krinkle has uploaded a new change for review.
https://gerrit.wikimedia.org/r/282505
Change subject: [WIP] resourceloader: Avoid request overhead for legacy modules
......................................................................
[WIP] resourceloader: Avoid request overhead for legacy modules
Follows-up 0ac4f998.
After d790562, legacy modules (added to the top queue) were no longer
consistently loaded before the bottom queue due to the top queue being async.
The implied dependency was made explicit by 0ac4f998 by awaiting all modules
that aren't themselves a legacy module (or a dependency thereof) on the
readiness of the legacy modules.
This had a negative side-effect of causing an extra HTTP request because we
wrongly assumed that the execute() function would not be called before a module
from the top queue request finished loading. Thus we wrongly assumed that this
waiting would only re-arrange execution (since the legacy modules were already
requested in the top queue, it was intended to just ride along the request for
that).
Instead, what really happened is that the embedded mw.loader.implement() call
for user options and tokens naturally triggers the execute() call before the
top queue is processed. It thus forces the loader to, at that point, start
a request just for the legacy modules (not knowing there is about to be a
request
for them already as part of the top queue).
This could be fixed by moving the top queue to be before the embedded modules
and
enforcing the embed in a different way. It could also be fixed by debouncing
module
load calls so they naturally end up as a single request.
However for now I'm addressing this by adding legacy modules to the list of
modules
in the initial load request from the startup module. This naturally makes them
available before all others without much overhead.
Change-Id: I54f087655e1cde1b8ff1ca5fe56e82f7f7d80965
---
M includes/resourceloader/ResourceLoaderStartUpModule.php
M includes/skins/Skin.php
M resources/src/mediawiki/mediawiki.js
3 files changed, 23 insertions(+), 36 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/core
refs/changes/05/282505/1
diff --git a/includes/resourceloader/ResourceLoaderStartUpModule.php
b/includes/resourceloader/ResourceLoaderStartUpModule.php
index 34866f3..a2cf193 100644
--- a/includes/resourceloader/ResourceLoaderStartUpModule.php
+++ b/includes/resourceloader/ResourceLoaderStartUpModule.php
@@ -109,7 +109,6 @@
'wgLegalTitleChars' =>
Title::convertByteClassToUnicodeClass( Title::legalChars() ),
'wgResourceLoaderStorageVersion' => $conf->get(
'ResourceLoaderStorageVersion' ),
'wgResourceLoaderStorageEnabled' => $conf->get(
'ResourceLoaderStorageEnabled' ),
- 'wgResourceLoaderLegacyModules' =>
self::getLegacyModules(),
'wgForeignUploadTargets' => $conf->get(
'ForeignUploadTargets' ),
'wgEnableUploads' => $conf->get( 'EnableUploads' ),
];
@@ -308,7 +307,7 @@
*/
public static function getStartupModulesUrl( ResourceLoaderContext
$context ) {
$rl = $context->getResourceLoader();
- $moduleNames = self::getStartupModules();
+ $moduleNames = array_merge( self::getStartupModules(),
self::getLegacyModules() );
$query = [
'modules' => ResourceLoader::makePackedModulesString(
$moduleNames ),
diff --git a/includes/skins/Skin.php b/includes/skins/Skin.php
index e5b9cb9..3e47583 100644
--- a/includes/skins/Skin.php
+++ b/includes/skins/Skin.php
@@ -163,8 +163,6 @@
'content' => [
'mediawiki.page.ready',
],
- // modules that exist for legacy reasons
- 'legacy' =>
ResourceLoaderStartUpModule::getLegacyModules(),
// modules relating to search functionality
'search' => [],
// modules relating to functionality relating to
watching an article
diff --git a/resources/src/mediawiki/mediawiki.js
b/resources/src/mediawiki/mediawiki.js
index 9d799db..a5417ff 100644
--- a/resources/src/mediawiki/mediawiki.js
+++ b/resources/src/mediawiki/mediawiki.js
@@ -1250,10 +1250,7 @@
registry[ module ].state = 'executing';
runScript = function () {
- var script, markModuleReady,
nestedAddScript, legacyWait,
- // Expand to include
dependencies since we have to exclude both legacy modules
- // and their dependencies from
the legacyWait (to prevent a circular dependency).
- legacyModules = resolve(
mw.config.get( 'wgResourceLoaderLegacyModules', [] ) );
+ var script, markModuleReady,
nestedAddScript;
try {
script = registry[ module
].script;
markModuleReady = function () {
@@ -1273,40 +1270,33 @@
nestedAddScript( arr, callback, i + 1 );
} );
};
+ if ( $.isArray( script ) ) {
+ nestedAddScript(
script, markModuleReady, 0 );
+ } else if ( $.isFunction(
script ) ) {
+ // Pass jQuery twice so
that the signature of the closure which wraps
+ // the script can bind
both '$' and 'jQuery'.
+ script( $, $,
mw.loader.require, registry[ module ].module );
+ markModuleReady();
- legacyWait = ( $.inArray(
module, legacyModules ) !== -1 )
- ? $.Deferred().resolve()
- : mw.loader.using(
legacyModules );
-
- legacyWait.always( function () {
- if ( $.isArray( script
) ) {
-
nestedAddScript( script, markModuleReady, 0 );
- } else if (
$.isFunction( script ) ) {
- // Pass jQuery
twice so that the signature of the closure which wraps
- // the script
can bind both '$' and 'jQuery'.
- script( $, $,
mw.loader.require, registry[ module ].module );
-
markModuleReady();
-
- } else if ( typeof
script === 'string' ) {
- // Site and
user modules are legacy scripts that run in the global scope.
- // This is
transported as a string instead of a function to avoid needing
- // to use
string manipulation to undo the function wrapper.
- if ( module ===
'user' ) {
- //
Implicit dependency on the site module. Not real dependency because
- // it
should run after 'site' regardless of whether it succeeds or fails.
-
mw.loader.using( 'site' ).always( function () {
-
$.globalEval( script );
-
markModuleReady();
- } );
- } else {
+ } else if ( typeof script ===
'string' ) {
+ // Site and user
modules are legacy scripts that run in the global scope.
+ // This is transported
as a string instead of a function to avoid needing
+ // to use string
manipulation to undo the function wrapper.
+ if ( module === 'user'
) {
+ // Implicit
dependency on the site module. Not real dependency because
+ // it should
run after 'site' regardless of whether it succeeds or fails.
+
mw.loader.using( 'site' ).always( function () {
$.globalEval( script );
markModuleReady();
- }
+ } );
} else {
- // Module
without script
+ $.globalEval(
script );
markModuleReady();
}
- } );
+ } else {
+ // Module without script
+ markModuleReady();
+ }
} catch ( e ) {
// This needs to NOT use mw.log
because these errors are common in production mode
// and not in debug mode, such
as when a symbol that should be global isn't exported
--
To view, visit https://gerrit.wikimedia.org/r/282505
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I54f087655e1cde1b8ff1ca5fe56e82f7f7d80965
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Krinkle <krinklem...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
