Page MenuHomePhabricator

Fix the most common "Module not loadable on target mobile" warnings (Oct 2019)
Closed, ResolvedPublic

Description

When we enabled this warning mechanism in 2018, the most common violation was the Gadgets extension. Its noise was hiding pretty much all else.

This was fixed last week per T171180. I've re-enabled the detection logging with https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/431816/, but today during the group1 deployment, it became obvious there's a few more we missed that happen on basically all page views (warning detection disabled now again in prod per T235711).

These will need to be fixed before we can continue.

For each of these, the code currently calls addModules() on both mobile and desktop, but the lack of a target declaration is making the module get skipped client-side. We will need to set targets=> [mobile, desktop] on these and then decide either to let it load like the code tries already, or explicitly forego loading in some way.

fixedmodule4h total countBug
ext.uls.interlanguage809,041T236943
ext.visualEditor.desktopArticleTarget.init809,039T236944
ext.visualEditor.desktopArticleTarget.noscript809,037T236944
ext.3d.styles809,036T235796
ext.uls.interface809,035T237036, T237061
ext.wikimediaBadges807,320T236946
site783,608T237050
site.styles783,606T237050
noscript783,604gerrit
ext.globalCssJs.user783,179T138727, T259047
ext.globalCssJs.user.styles783,177T138727, T259047
ext.uls.compactlinks747,326T237061
mmv.head378,848gerrit
mmv.bootstrap.autostart378,845gerrit
wikibase.client.init200,084T259048
mw.MediaWikiPlayer.loader103,683T217085
mw.PopUpMediaTransform102,323T217085
ext.tmh.thumbnail.styles102,322T217085
wikibase.ui.entitysearch102,041T259049
ext.wikidata-org.badges101,481T259050
wikibase.common97,583T259049
jquery.wikibase.toolbar.styles85,611T259049
jquery.ui.core.styles85,610
wikibase.ui.entityViewInit85,610T259049
ext.popups82,738T236097
ext.citoid.wikibase.init82,524
wikibase.quality.constraints.suggestions80,423
propertySuggester.suggestions80,093
ext.3d76,091
filepage75,845
mediawiki.action.view.filepage75,844
ext.cx.eventlogging.campaigns74,886
ext.cite.ux-enhancements74,406
mediawiki.action.view.metadata53,789
ext.flaggedRevs.advanced40,314
mw.TMHGalleryHook.js34,020
ext.advancedSearch.init21,810
ext.advancedSearch.initialstyles21,810
mediawiki.special.search21,810gerrit
ext.scribunto.logs16,021
mediawiki.special.userlogin.login.styles12,910T211439
ext.charinsert12,849
ext.charinsert.styles12,849
mediawiki.action.edit.collapsibleFooter12,345
ext.CodeMirror12,332
mediawiki.special.userlogin.signup.js11,808T211439
mediawiki.special.userlogin.signup.styles11,808T211439
ext.TemplateWizard11,804
mediawiki.action.edit11,804
ext.wikiEditor11,793
    1. Acceptance criteria
  • When enabling modules ensure that they are needed by all pages. For example if code only runs on the login page, it should only run on the login page.
  • The new code should be compatible with the newer skins Minerva and Timeless. If not, it should check the skin and not add the module to these skins.
  • If the module is over 0.5kb it should use a bootstrap module before the targets is changed.
  • The correct loading strategy has been used
  • The correct performance strategy has been used
  • There is no (or a minor < 0.5kb) increase in JS or CSS on the critical path for an article page on the mobile site when enabling the module reported in the graphs on https://grafana.wikimedia.org/d/000000205/mobile-2g?orgId=1

Loading strategies

  1. If module should not run on mobile domain

Example would be mmv.head and ext.popups:

Server side
if (  ExtensionRegistry::getInstance()->isLoaded( 'MobileFrontend' ) && MediaWikiServices::getInstance()->getService( 'MobileFrontend.Context' )->shouldDisplayMobileView() ) {
     // Add mobile specific modules
} else {
   // MobileFrontend is not installed or you are in desktop so add desktop specific modules safely.
}
Client-side
if ( !mw.config.get('wgMFMode') ) {
   mw.loader.using('desktopmodule');
} else {
   mw.loader.using('mobilemodule');
}
  1. If module should run only on mobile domain

Use the hook MobileSiteOutputPageBeforeExec

  1. If module should run on all pages

Check the dependencies of the module.
Set the target to ['mobile', 'desktop']

Performance strategies

  1. Load module only on the page that needs it.

Certain modules only run on certain pages. For example, code may only be needed if a certain query string parameter is present or the user is on a certain special page. Where possible check for these things.

  1. Load module on pages which have the feature using the strpos pattern

If a module progressively enhances an element that is sometimes and sometimes not in the page e.g.<div class="feature"></div>` using strpos check for the existence of the class before calling addModules
https://github.com/wikimedia/mediawiki/blob/fe598a9600/includes/skins/Skin.php#L210

  1. Load larger modules on pages based on an interaction.

If code is only needed when something happens - e.g. if you click on X the rest of the code is loaded, the click pattern may be helpful.
This might be mixed with the bootstrap pattern (pattern #4) to prefetch code.

Echo does this to load code when the notifications icon is clicked:
https://github.com/wikimedia/mediawiki-extensions-Echo/blob/30dbae4c02/modules/ext.echo.init.js#L223-L244

  1. Bootstrap pattern using mw.requestIdleCallback

For code which cannot be reduced and needs to be loaded on every page, you may want to follow the pattern of ext.popups and move JavaScript loading into an idle callback however the performance team there may be little to no benefit of doing this:
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/9d72260851/resources/ext.popups/index.js

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Could the performance team document these frontend best performance practices on https://www.mediawiki.org/wiki/ResourceLoader/Developing_with_ResourceLoader ?

I don't think it's worth the risk of changing how we load Popups now, but it's worth noting one of our most used feature is deprioritised in the queue for arguably less used things. I would expect Popups to have the same loading strategy as ULS and MMV for example.

Jdlrobson updated the task description. (Show Details)

Change 547672 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MultimediaViewer@master] Do not try to load MMV on the mobile domain.

https://gerrit.wikimedia.org/r/547672

Change 547674 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Load noscript module on mobile

https://gerrit.wikimedia.org/r/547674

Change 547674 merged by jenkins-bot:
[mediawiki/core@master] Load noscript module on mobile

https://gerrit.wikimedia.org/r/547674

Jdlrobson updated the task description. (Show Details)

Change 547672 merged by jenkins-bot:
[mediawiki/extensions/MultimediaViewer@master] Do not queue MMV modules on the mobile domain

https://gerrit.wikimedia.org/r/547672

Change 547673 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Enable special page modules for search

https://gerrit.wikimedia.org/r/547673

If the code is loaded on many pages and we add a 1-line bootstrap […]. What does it improve?

When building Popups we were advised to use requestIdleCallback […]. Are you saying we no longer need to do that?

Ah, forgot about that. Yeah. […] As a general rule, less overhead and indirection is better for the system as a whole, […]

Could the performance team document these frontend best performance practices on https://www.mediawiki.org/wiki/ResourceLoader/Developing_with_ResourceLoader ?

I've recently updated that page to reflect the current best practices regarding module registration overhead and bundling. However, I didn't mention to not use requestIdleCallback because it wasn't a a common pattern in the past. Rather, it was a one-off experiment for Popups to better understand an issue we observed during its deployment review.

Change 547673 merged by jenkins-bot:
[mediawiki/core@master] Enable special page modules for search

https://gerrit.wikimedia.org/r/547673

Change 604184 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/UniversalLanguageSelector@master] Define unsupported skins

https://gerrit.wikimedia.org/r/604184

Change 604184 merged by jenkins-bot:
[mediawiki/extensions/UniversalLanguageSelector@master] Define unsupported skins

https://gerrit.wikimedia.org/r/604184

Jdlrobson updated the task description. (Show Details)

@Krinkle would it be possible to rerun the script. I think enough has changed here to warrant resolving this and creating a new task to see where we're at. A lot has changed since October 2019.

@Krinkle would it be possible to rerun the script. I think enough has changed here to warrant resolving this and creating a new task to see where we're at. A lot has changed since October 2019.

I collected it from Logstash. To enable that data flow, re-apply this patch:

Change 431816 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Bump severity of targets violation to WARNING

https://gerrit.wikimedia.org/r/431816

It was reverted after a week (patch) per T235711. We can run this again for a week to gather new data, and possibly keep it there are zero entries on simple/most page views. So long as it won't trigger alerts or confuse deployers on mwdebug, where we generally expect no errors/warnigns under normal conditions.

Change 859643 had a related patch set uploaded (by Jdlrobson; author: Krinkle):

[mediawiki/core@master] resourceloader: Lower severity of targets violation back to DEBUG

https://gerrit.wikimedia.org/r/859643

Change 859643 merged by jenkins-bot:

[mediawiki/core@master] resourceloader: Raise severity of targets violation to WARNING

https://gerrit.wikimedia.org/r/859643

Change 864722 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/core@master] Revert "resourceloader: Raise severity of targets violation to WARNING"

https://gerrit.wikimedia.org/r/864722

Change 865229 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Wikibase@master] Do not add desktop targeted code directly to page

https://gerrit.wikimedia.org/r/865229

Change 865229 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Make wikibase.client.init module target mobile

https://gerrit.wikimedia.org/r/865229

Change 866470 had a related patch set uploaded (by Ladsgroup; author: Jdlrobson):

[mediawiki/extensions/Wikibase@wmf/1.40.0-wmf.13] Make wikibase.client.init module target mobile

https://gerrit.wikimedia.org/r/866470

Change 866470 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@wmf/1.40.0-wmf.13] Make wikibase.client.init module target mobile

https://gerrit.wikimedia.org/r/866470

Mentioned in SAL (#wikimedia-operations) [2022-12-08T23:23:32Z] <ladsgroup@deploy1002> Started scap: Backport for [[gerrit:866470|Make wikibase.client.init module target mobile (T235712)]]

Mentioned in SAL (#wikimedia-operations) [2022-12-08T23:25:22Z] <ladsgroup@deploy1002> ladsgroup and ladsgroup: Backport for [[gerrit:866470|Make wikibase.client.init module target mobile (T235712)]] synced to the testservers: mwdebug1002.eqiad.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2001.codfw.wmnet, mwdebug2002.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2022-12-08T23:32:15Z] <ladsgroup@deploy1002> Finished scap: Backport for [[gerrit:866470|Make wikibase.client.init module target mobile (T235712)]] (duration: 08m 42s)

Change 864722 merged by jenkins-bot:

[mediawiki/core@master] Revert "resourceloader: Raise severity of targets violation to WARNING"

https://gerrit.wikimedia.org/r/864722