Page MenuHomePhabricator

Reduce module registration overhead from CentralNotice
Open, Needs TriagePublic

Description

See also T202154: Audit modules 2018: Reduce registry overhead


This extension currently registers 23 modules on all wikis.

  • jquery.ui.multiselect
  • ext.centralNotice.adminUi
  • ext.centralNotice.adminUi.campaignPager
  • ext.centralNotice.adminUi.bannerManager
  • ext.centralNotice.adminUi.bannerEditor
  • ext.centralNotice.adminUi.campaignManager
  • ext.centralNotice.startUp
  • ext.centralNotice.geoIP
  • ext.centralNotice.choiceData
  • ext.centralNotice.display
  • ext.centralNotice.kvStore
  • ext.centralNotice.kvStoreMaintenance
  • ext.centralNotice.bannerHistoryLogger
  • ext.centralNotice.impressionDiet
  • ext.centralNotice.largeBannerLimit
  • ext.centralNotice.legacySupport
  • ext.centralNotice.bannerSequence
  • ext.centralNotice.adminUi.bannerSequence
  • ext.centralNotice.freegeoipLookup
  • ext.centralNotice.bannerController
  • ext.centralNotice.bannerController.mobile
  • ext.centralNotice.impressionEventsSampleRate
  • ext.centralNotice.cspViolationAlert
  • Identify which logical entry points exists (e.g. queued via addModules or lazy-loaded).
  • Identify whether there are files without side effects (e.g. classes) that are needed by multiple entry points, and define zero or more shared bundles for those.
  • Merge the rest.

Event Timeline

Krinkle created this task.Apr 24 2019, 8:25 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 24 2019, 8:25 PM

Change 506317 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CentralNotice@master] Move kvStoreMaintenance to 'ext.centralNotice.startUp'

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

Change 506317 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Move kvStoreMaintenance to 'ext.centralNotice.startUp'

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

Krinkle updated the task description. (Show Details)May 20 2019, 9:57 PM
Krinkle edited projects, added Fundraising-Backlog; removed Patch-For-Review.
Krinkle edited projects, added Wikimedia-Fundraising; removed Fundraising-Backlog.

Of these modules, the following 7 modules are only used in the administration UI interface on Meta:

  • ext.centralNotice.adminUi
  • ext.centralNotice.adminUi.campaignPager
  • ext.centralNotice.adminUi.bannerManager
  • ext.centralNotice.adminUi.bannerEditor
  • ext.centralNotice.adminUi.campaignManager
  • ext.centralNotice.adminUi.bannerSequence
  • ext.centralNotice.cspViolationAlert

Maybe a first step would be to consolidate these? The Admin UI interface impacts only CN administrators. I imagine a small tradeoff of increased RL payload to them would be worth an also small decrease in load time for millions of users.

I guess there isn't a way to selectively register modules based on configuration (to avoid even registering the Admin UI ones on other wikis)?

ext.centralNotice.bannerController and ext.centralNotice.bannerController.mobile are empty and should just be reoved. Here's a task for that: T224034

ext.centralNotice.freegeoipLookup is currently broken, but something like it is useful for development and possibly 3rd party wikis. If we wish to not worry about 3rd party wiki support for geotargeting, we could also probably bundle it in with Admin UI stuff. Here's a task: T224038.

Regarding the modules that run on subscribing wikis, the whole design of the system was made with performance in mind. I think the overhead caused by module registration was not taken into account, however.

On that point, the following modules are "mix-ins" that provide optional features on all wikis ("subscribing" wikis) in the process of banner display. They are only added as dependencies to choiceData as needed.

  • ext.centralNotice.bannerHistoryLogger
  • ext.centralNotice.impressionDiet
  • ext.centralNotice.largeBannerLimit
  • ext.centralNotice.legacySupport
  • ext.centralNotice.bannerSequence
  • ext.centralNotice.impressionEventsSampleRate

At least two of the above, ext.centralNotice.bannerHistoryLogger and ext.centralNotice.largeBannerLimit are used almost exclusively for fundraising and are normally used together, so those might be consolidated.

Two, ext.centralNotice.legacySupport and ext.centralNotice.impressionEventsSampleRate, are very small, so I imagine payload for module registration overhead for them is greater than the code payload itself. One feature of ext.centralNotice.legacySupport will be removed pretty soon, too. So perhaps the best option for those would be to just integrate the remaining functionality into the ext.centralNotice.display.

Yet another, ext.centralNotice.geoIP, shouldn't really be part of CentralNotice at all. (See T102848.)

Is there documentation or notes somewhere about how much each module adds from registration overhead?

I think another issue here is the modernization of CN RL modules, as regards file structure and the way the JS API is set up? I guess maybe we could somehow fix that, too, together with the changes to reduce the number of modules? (That's the approach it seems you've used with changes proposed so far--definitely seems like the right way to go.) So, maybe we could bring all CN RL modules up-to-date, plan the overall dependency scheme, and consolidate as far as possible? We might also take into account data from Hive about whether and in what contexts the dynamic dependency system of choiceData is actually helpful. (I expect that it is useful, especially on mobile and on smaller languages and projects, but I don't have the data to back that up.)

Thoughts?

Thanks so much for working on this!!!!!!!!!

Krinkle added a comment.EditedMay 21 2019, 10:52 PM

[..] the following 7 modules are only used in the administration UI interface on Meta: [..]
Maybe a first step would be to consolidate these?

Yes!

I guess there isn't a way to selectively register modules based on configuration [..]

Actually, that's quite possible. We have the ResourceLoaderRegisterModules hook for this purpose. From there you can conditionally call $rl->register() based on site configuration. So the logic you have in CentralNotice for deciding whether to register the special page (e.g. detecting that you're on Meta-Wiki), could be re-used for registering its modules.

For an example, see the Cite extension, which conditionally registers a VE plugin.

Is there documentation or notes somewhere about how much each module adds from registration overhead?

It's effectively just about the array elements of the startup module. The cost of transferring these (bandwidth, and download time) and cost of holding these (parsing them, and allocating in memory) will be proportional to the number of bytes used for it as part of the startup response.

This isn't monitored currently on a per-module level, but can be extracted from load.php by searching for the relevant modules and copying the surrounding arrays. Note that this should include their dependencies, and any relations to them elsewhere. (For CentralNotice the latter is presumably none, given no other extensions depending on CN modules).

In this case:

Startup manifest (CN excerpt)
["jquery.ui.multiselect","0qwr195",[55,63,157]],["ext.centralNotice.adminUi","1ldgfzc",[52,390,126]],["ext.centralNotice.adminUi.campaignPager","1cwyb0x"],["ext.centralNotice.adminUi.bannerManager","1a7fxsd",[391,53]],["ext.centralNotice.adminUi.bannerEditor","0h852qy",[391,53,87]],["ext.centralNotice.adminUi.campaignManager","1rm7jgq",[391,45,53,62,84,250]],["ext.centralNotice.startUp","17iws46",[398,401,129]],["ext.centralNotice.geoIP","1svqygb",[27]],["ext.centralNotice.choiceData","125h5e6"],["ext.centralNotice.display","1tw8vil",[397,400,126]],["ext.centralNotice.kvStore","0un16jc",[401]],["ext.centralNotice.kvStoreMaintenance","1vmne68"],["ext.centralNotice.bannerHistoryLogger","18ci9bz",[399,127]],["ext.centralNotice.impressionDiet","1753l4t",[399]],["ext.centralNotice.largeBannerLimit","0fybqa8",[399,133]],[
"ext.centralNotice.legacySupport","0wctcxy",[399]],["ext.centralNotice.bannerSequence","131xgje",[399]],["ext.centralNotice.adminUi.bannerSequence","0i9zt39",[395]],["ext.centralNotice.freegeoipLookup","0orp3pu",[397]],["ext.centralNotice.bannerController","07j6l8d",[397,396]],["ext.centralNotice.impressionEventsSampleRate","1p3o9dw",[399]],["ext.centralNotice.cspViolationAlert","1aaale4"],
  • Total size: 1221 bytes (1.2 KB)

The download time, processing time, and memory are harder to measure and vary based on the user's network provider, device and browser engine.

I guess maybe we could somehow fix [file structure], too, together with the changes to reduce the number of modules?

Yeah, more about the motivation behind that at T193826 and at mw:RL/PKG.

We might also take into account data from Hive about whether and in what contexts the dynamic dependency system of choiceData is actually helpful.

I'd be very interested in such data, also for other modules and extensions. However, I'm not sure whether we can get that accurately from Hive. Remember that the fastest request is the request never made, and ResourceLoader's client-side cache means that quite a few composition bundles a page view, never make it onto the network because the cache already satisfies it. We unpack batches of modules client-side and re-use them between page views, even if the bundle composition differs on the other end. Read more at https://www.mediawiki.org/wiki/ResourceLoader/Architecture#Store.

another issue here is the modernization of CN RL modules, as regards file structure and the way the JS API is set up? I guess maybe we could somehow fix that, too

Just to note here, @Krinkle has a patch in review that addresses this for a large chunk client-side subscribing code: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/512429/

Possibly also relevant for reducing module cruft? T133434: CentralNotice: Use only one cookie library

Change 531223 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/CentralNotice@master] Only register adminUI modules on the CN infra wiki (e.g. Meta-Wiki)

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

Change 531223 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Only register adminUI modules on the CN infra wiki (e.g. Meta-Wiki)

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