Page MenuHomePhabricator

Fix the most common "Module not loadable on target mobile" warnings (Oct 2019)
Open, Needs TriagePublic

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
xext.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
xnoscript783,604gerrit
ext.globalCssJs.user783,179T138727
ext.globalCssJs.user.styles783,177T138727
ext.uls.compactlinks747,326T237061
xmmv.head378,848gerrit
xmmv.bootstrap.autostart378,845gerrit
wikibase.client.init200,084
mw.MediaWikiPlayer.loader103,683
mw.PopUpMediaTransform102,323
ext.tmh.thumbnail.styles102,322
wikibase.ui.entitysearch102,041
ext.wikidata-org.badges101,481
wikibase.common97,583
jquery.wikibase.toolbar.styles85,611
jquery.ui.core.styles85,610
wikibase.ui.entityViewInit85,610
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
xmediawiki.special.search21,810gerrit
ext.scribunto.logs16,021
mediawiki.special.userlogin.login.styles12,910
ext.charinsert12,849
ext.charinsert.styles12,849
mediawiki.action.edit.collapsibleFooter12,345
ext.CodeMirror12,332
mediawiki.special.userlogin.signup.js11,808
mediawiki.special.userlogin.signup.styles11,808
ext.TemplateWizard11,804
mediawiki.action.edit11,804
ext.wikiEditor11,793

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:

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.
}

On client

if ( mw.config.get('wgMFMode')  === undefined ) {
   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']

  1. Performance strategies
  2. 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/master/includes/skins/Skin.php#L219

  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/master/modules/ext.echo.init.js#L247

  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/master/resources/ext.popups/index.js

Details

Related Gerrit Patches:
mediawiki/core : masterEnable special page modules for search
mediawiki/extensions/MultimediaViewer : masterDo not queue MMV modules on the mobile domain
mediawiki/core : masterLoad noscript module on mobile

Related Objects

Event Timeline

Krinkle created this task.Oct 16 2019, 9:33 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 16 2019, 9:33 PM
Krinkle updated the task description. (Show Details)Oct 16 2019, 9:33 PM
Jdlrobson updated the task description. (Show Details)Mon, Oct 21, 5:55 PM
Jdlrobson updated the task description. (Show Details)Mon, Oct 21, 5:59 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptMon, Oct 21, 5:59 PM
Jdlrobson added a subscriber: Jdlrobson.

I've added some important criteria from our team's perspective about the "how" we do this. Hopefully it's clear and actionable.

I've opened a specific task for Popups which my team owns. For the other issues I defer to the maintainers of those extensions and am happy to help consult and code review patches for specific problems. The reading web team as part of desktop refresh may choose to speed up those transitions as and where necessary.

 try {
  $ctx = MediaWikiServices::getInstance()->getService( 'MobileFrontend.Context' );
  if ( !$ctx->shouldDisplayMobileView() ) {
     // add desktop specific modules
  } 
} catch ( Wikimedia\Services\NoSuchServiceException $ex ) {
   // MobileFrontend is not installed.
}

This think we generally use ExtensionRegistry::isLoaded() here to avoid swallowing exceptions when there is a mistake in the code or breaking change with the name of the main service and/or one of its dependencies (eg. any getService call from this service and/or any getService call from any code within shouldDisplayMobileView).

  • If the module is over 0.5kb it should use a bootstrap module before the targets is changed.

What the intended impact of this change? If the code is loaded on many pages and we add a 1-line bootstrap that calls mw.loader on those same pages, we've basically added bandwidth and memory overhead to the startup module, and delayed to "time to interactive" for the feature. What does it improve? (In particular because our pipeline is already non-blocking/asynchronous).

What the intended impact of this change?

Basically, I'm very concerned in this task about maintaining the status quo for mobile performance and improving desktop performance as a result of carrying out this task. Right now mobile site ships 225kb of JS for the desktop site vs 179kb for the mobile site on the same article. CSS is much more scary - we ship, 14.38kb of CSS on desktop compared to 7.98kb on mobile. The best result of this task is that those values on mobile remains around the same, and desktop decreases.

My concern is that if we just blindly enable all these modules the startup cost of JS on mobile is going to increase considerably per T235797 (compare Minerva desktop payload via Minerva mobile payload to get a rough idea). Obviously there are cases where that code will be warranted but I wanted an easy to understand litmus test to make sure we have the right conversations. I did pick an arbitrary number here but hoped it would just trigger a conversation. The worst possible thing that could happen as part of this task is that we get random patch submissions that simply change the 'targets' property of ResourceLoader modules.

If the code is loaded on many pages and we add a 1-line bootstrap that calls mw.loader on those same pages, we've basically added bandwidth and memory overhead to the startup module, and delayed to "time to interactive" for the feature. What does it improve? (In particular because our pipeline is already non-blocking/asynchronous).

When building Popups we were advised to use requestIdleCallback (see T176211) to reduce JavaScript execution "during the critical rendering path.". I figured we'll want to apply this approach to other modules too. Are you saying we no longer need to do that?

Feel free to amend the recommendations in the description as the performance expert - I just wanted to provide some perspective from our side!

[1] https://grafana.wikimedia.org/d/000000205/mobile-2g?

Jdlrobson updated the task description. (Show Details)Tue, Oct 22, 12:04 AM

If the code is loaded on many pages and we add a 1-line bootstrap […]. What does it improve? (In particular because our pipeline is already non-blocking/asynchronous).

When building Popups we were advised to use requestIdleCallback (see T176211) to reduce JavaScript execution "during the critical rendering path.". I figured we'll want to apply this approach to other modules too. Are you saying we no longer need to do that?

Ah, forgot about that. Yeah. That change helped rule out a number of things during the review, but I don't think it applies anymore. The idle-callback behaves a bit differently from what we initially thought. See also T190950 from last year which is essentially about removing this indirection for Popups.

As a general rule, less overhead and indirection is better for the system as a whole, as it allows ResourceLoader to better do its orchestration logic for JS execution. On the macro scale of 1 Popups module I don't expect to see improvements, but on the whole fewer modules and indirection should help in the long run, and it shouldn't make things worse at least.

Having said that, Popups is significant enough in terms of size that it's worth rolling out as its own change so that we can monitor the change closely with high confidence in any changes we observe (e.g. not part of the train). In isolation, it should make no difference. But, still worth monitoring because in the real world the competition for bandwidth and CPU resources can always shift in surprising ways.

If we find that it does make things worse, I'll think about making mw.loader's internal rIC use more aligned with the Popups experiment.

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)Wed, Oct 30, 7:16 PM
Jdlrobson updated the task description. (Show Details)Wed, Oct 30, 7:20 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Wed, Oct 30, 7:32 PM
Jdlrobson updated the task description. (Show Details)Thu, Oct 31, 7:54 PM
Jdlrobson updated the task description. (Show Details)Thu, Oct 31, 10:31 PM

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

Jdlrobson updated the task description. (Show Details)Thu, Oct 31, 10:40 PM

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)Mon, Nov 4, 10:08 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Inbox to OKR Backlog on the User-Jdlrobson board.

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.

Jdlrobson updated the task description. (Show Details)Tue, Nov 5, 7:22 PM

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

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

Jdlrobson updated the task description. (Show Details)Tue, Nov 5, 11:16 PM
Jdlrobson updated the task description. (Show Details)Tue, Nov 12, 7:06 PM