Page MenuHomePhabricator

GrowthExperiments adds 2.1K cost to Wikipedia pageviews [alert: Startup threshold exceeded]
Closed, ResolvedPublic

Description

Noticed during routine review of Grafana: startup-manifest-size, on 5 May (2021-05-05) there was a sudden increase of 2.1 KB on all page views for all users, due to the startup registry cost added by the deployment of GrowthExperiments, which currently registers 35 distinct module bundles.

Startup payload size (last 6 months)
a.png (836×1 px, 59 KB)
b.png (914×2 px, 108 KB)
c.png (749×2 px, 367 KB)

At 2.1KB for a single extension, that places it right up with ContentTranslation and VisualEditor, which have upcoming mitigations of their own that are difficult for legacy reasons, but this does not apply to new extensions which should generally not need more than three (3) modules. I regretfully did not notice this during our team's perf review in Nov 2020 (ref T240201). At the time there were 25 modules bundldes, but this has since then increased to 35. That suggests module bundles are used for something other than logical entry points or performance optimisations, but for code organisation, which is not what bundles are for.

From a quick look at the codebase, it seems like these should be easy to consolidate. If this is not easy to do for some reason, I think we need to work together on a viable solution and to block deployment until that is done.

Related Objects

Event Timeline

Urbanecm_WMF subscribed.

Boldly triaging this one.

$ diff <(cat extension.json | jq --raw-output '.ResourceModules | keys []' | sort) <(git show `git rev-list -1 --before="Nov 1 2020" master`:extension.json | jq --raw-output '.ResourceModules | keys []' | sort)
1,6d0
< ext.growthExperiments.AddLink
< ext.growthExperiments.AddLink.desktop
< ext.growthExperiments.AddLink.icons
< ext.growthExperiments.AddLink.mobile
< ext.growthExperiments.AddLink.onboarding
< ext.growthExperiments.AddLink.onboardingContent
31d24
< ext.growthExperiments.MultiPaneDialog

AddLink is a new major feature, has a different workflow, loads on different pages. AddLink.desktop and AddLink.mobile are probably necessary since the VE integration needs to happen differently in those two cases. Plus, we might not want to load desktop-only dependencies on mobile and vice versa. (There aren't many of them so maybe we could just handle the registration split by checking OO.ui.isMobile()?) AddLink.icons is necessary because icons and other assets cannot share a module in ResourceLoader. AddLink.onboarding is something only shown once to a user so it doesn't really make sense to load it all the time. AddLink.onboardingContent and MultiPaneDialog probably shouldn't be separate modules, they can be merged to AddLink.onboarding.

Change 693191 had a related patch set uploaded (by MewOphaswongse; author: MewOphaswongse):

[mediawiki/extensions/GrowthExperiments@master] Add a link: only include AddLinkOnboarding module if it's needed

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

Change 693202 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/GrowthExperiments@master] Inline AddLink.onboardingContent and MultiPaneDialog RL modules

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

Tgr removed Tgr as the assignee of this task.May 20 2021, 8:06 PM
Tgr moved this task from In Progress to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.

The patch removes two of the seven modules that have been added since November. I'm not sure what else we could do, short of merging some unrelated modules. Maybe migrate module registration to a hook and drop the ones which are related to disabled feature flags? (AddLink is only enabled on four production wikis.)

Change 693202 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Inline AddLink.onboardingContent and MultiPaneDialog RL modules

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

The patch removes two of the seven modules that have been added since November. I'm not sure what else we could do, short of merging some unrelated modules. Maybe migrate module registration to a hook and drop the ones which are related to disabled feature flags? (AddLink is only enabled on four production wikis.)

@Krinkle do you have further suggestions on what we should do?

ext.growthExperiments.AddLink
ext.growthExperiments.AddLink.desktop
ext.growthExperiments.AddLink.icons
ext.growthExperiments.AddLink.mobile
ext.growthExperiments.AddLink.onboarding
ext.growthExperiments.Help
ext.growthExperiments.HelpPanel
ext.growthExperiments.HelpPanel.icons
ext.growthExperiments.HelpPanel.init
ext.growthExperiments.HelpPanelCta.styles
ext.growthExperiments.Homepage.ConfirmEmail
ext.growthExperiments.Homepage.ConfirmEmail.styles
ext.growthExperiments.Homepage.Discovery.scripts
ext.growthExperiments.Homepage.Discovery.styles
ext.growthExperiments.Homepage.Impact
ext.growthExperiments.Homepage.Logger
ext.growthExperiments.Homepage.Logging
ext.growthExperiments.Homepage.Mentorship
ext.growthExperiments.Homepage.Mobile
ext.growthExperiments.Homepage.RecentQuestions
ext.growthExperiments.Homepage.StartEditing
ext.growthExperiments.Homepage.SuggestedEdits
ext.growthExperiments.Homepage.Topics
ext.growthExperiments.Homepage.contribs.styles
ext.growthExperiments.Homepage.icons
ext.growthExperiments.Homepage.styles
ext.growthExperiments.PostEdit
ext.growthExperiments.SuggestedEditSession
ext.growthExperiments.confirmEmail.createAccount
ext.growthExperiments.confirmEmail.createAccount.styles
ext.growthExperiments.mobileMenu.icons
ext.growthExperiments.welcomeSurvey.styles
ext.growthExperiments.welcomeSurveyLanguage

I suppose we could combine the ext.growthExperiments.Homepage.* modules into a single one. In theory some of them are not relevant to certain configurations of the homepage (e.g. maybe the wiki has mentorship or sugested edits disabled).

PostEdit and SuggestedEditSession are pretty closely related, so maybe PostEdit should get folded into the SuggestedEditSession module.

The styles modules have to exist separately AIUI since they use ResourceLoaderFileModuleWithLessVars, so I don't see how we could down to the 3 module limit proposed by @Krinkle.

Also, just to clarify, we are talking about two separate issues here, right? One is the amount of JS getting shipped (2.1 kb) and the other is the number of startup modules. If we consolidate modules, we'll still end up at ~2.1 KB (maybe more) for the JS getting sent.

Also, just to clarify, we are talking about two separate issues here, right? One is the amount of JS getting shipped (2.1 kb) and the other is the number of startup modules. If we consolidate modules, we'll still end up at ~2.1 KB (maybe more) for the JS getting sent.

It's the same issue AIUI. The startup module contains a list of all module names with some metadata; GrowthExperiments contributed about 2100 characters to that list (example - about 1.8K now).

I wonder how much we should care about the uncompressed size though. If we do, just renaming the ext.growthExperiments to ext.ge would reduce it to about half; it wouldn't really affect bandwidth usage though.

It's hard to report compressed metrics and still break it down by source component. The main metric here is the module count of 35 modules, not the 2.1KB.

But, point taken, I had forgotten this number was after decompression, and indeed, the length of individual module names is not the concern here. I'll see what I can do to make this more clear on the dashboard. A possible compromise would be to simulate per-component compression in the PHP script that generates these metrics to approximate more closely the actual bandwidth consumed, at the cost of being more difficult to understand what the number means in the real world (neither actual bandwidth, nor decompressed size).

In general, I encourage JS cost measuring in decompressed sizes since that a more realistic proxy for actual cost in terms of parsing, compilation and execution. This is a trend I see elsewhere in the industry as well. The only place really where we use compressed sizes is in RUM/Synthetic metrics about complete page loads and user journeys as a measure of combined bandwidth cost (in addition to a per content-type breakdown of decompressed sizes).

Change 693191 abandoned by MewOphaswongse:

[mediawiki/extensions/GrowthExperiments@master] Add a link: only include AddLinkOnboarding module if it's needed

Reason:

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

As a thought excercise, it might help to think about what the code would look like if you had only a single bundle for the extension as a whole. (Apart from the core limitation for icon and style modules being disctinct.) Then thinking backwards from there one could think about how the code would be organised internally, triggering only what's needed, and potentially splitting off one or two chunks that you think are especially large in self-size or dependencies that are especially unlikely to be needed at all for a given week where they do engage with some of the other features.

Moving this back to Ready for Development. I'll start a spreadsheet to audit how the modules are used where to determine which ones can be bundled.

Hi @Tgr @kostajh — I went through all the modules and made some notes of their usage and came up with a plan to cut the number of modules down to 9. Let me know what you think.

  • Inline modules that are currently used for code organization but are used in the same end-user scenario (T287218, T287219)
  • [new] ext.growthExperiments.icons --- All icons except mobile menu (T287209)
  • [new] ext.growthExperiments.styles --- All CSS that are needed prior to JS loads (or for no js views). Might not be ideal since this CSS blocks the initial render so we might want to keep them separate so each is as small as possible
  • [new] ext.growthExperiments.Homepage --- Modules that are only used on the homepage (maybe this can be broken down to modules that are more commonly bundled together)
  • [new] ext.growthExperiments.Editing --- Modules that are used on the article page -- help, structured tasks etc
  • [new] ext.growthExperiments.Homepage.mobile --- ext.growthExperiments.Homepage + mobile
  • [new] ext.growthExperiments.Editing.mobile --- ext.growthExperiments.Editing + mobile
  • [new] ext.growthExperiments.Editing.desktop --- ext.growthExperiments.Editing + desktop
  • [keep] ext.growthExperiments.SuggestedEditSession --- I considered inlining this but I think it might be worth keeping separate so that places that need to just check for SE session don't have to load unnecessary code.
  • [keep] ext.growthExperiments.mobileMenu.icons --- Not sure if there's a good way to include this with the rest of the icons since this is specific to Minerva icons

More details can be found under the "Consolidation" tab in this spreadsheet.

Thanks for the thorough analysis, @mewoph!

I think there are a few modules that won't be easy to merge:

  • Homepage.Discovery is (somewhat confusingly) not used on the homepage, but on every other page, to discover the homepage (show a guider telling the user how to get there).
  • The poorly named ext.growthExperiments.Help is used by both the help panel and the homepage (as they share the functionality of sending messages to a mentor), and it is huge, so simply duplicating it would make the code hard to maintain.

Also, I think this would be quite a bit of work, not just a rearranging of module definitions.

  • Currently, loading a module is usually used as an "enable" mechanism (it's OK for the module to unconditionally make changes to the page because if those changes weren't needed, it wouldn't get loaded). If we load a bunch of modules that are expected to be passive most of the time, that would have to be reviewed, and some of the code might need to be made conditional (the server-side code which currently uses Outputpage::addModule as a signal to apply changes would have to set a JS variable or add a body class or something like that). Maybe most modules simply wouldn't apply since the classes / DOM subtrees they try to affect aren't there, so this is a non-issue - I'm not sure.
  • Also, some script fields would have to be converted to packageFiles fields which have different loading mechanics.

I think there is an easy and fairly large reduction here, which is merging all the homepage modules, and merging the account creation related modules (welcome survey, email confirmation, donor signup - although maybe that last one can be discarded now). The rest doesn't seem to be worth the effort IMO.

In the longer term, I'd wait for improvements to ResourceLoader for facilitating module count reduction (T225842: Allow ResourceLoader modules to be "private", bundle them with their dependent modules, ability to mark modules server-side-only) before putting much more effort into this.

Thanks for the context @Tgr! Only bundling all the homepage modules in the short term makes sense to me.

@kostajh -- could you please add comments for Performance team and create any new tasks, and move this task off our sprint board?

I think there is an easy and fairly large reduction here, which is merging all the homepage modules, and merging the account creation related modules (welcome survey, email confirmation, donor signup - although maybe that last one can be discarded now). The rest doesn't seem to be worth the effort IMO.

We're going to make a separate task for that, and do it sometime soon-ish. For the overall issue with startup registry cost, we'd like to revisit that after T225842: Allow ResourceLoader modules to be "private", bundle them with their dependent modules is done.

Change 755749 had a related patch set uploaded (by MewOphaswongse; author: MewOphaswongse):

[mediawiki/extensions/GrowthExperiments@master] Consolidate ext.growthExperiments.Homepage.contribs.styles into ext.growthExperiments.Homepage.styles

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

Change 755749 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Consolidate ext.growthExperiments.Homepage.contribs.styles into ext.growthExperiments.Homepage.styles

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

@Krinkle the size is now 1.7kB and module count is 27.

Is there anything else you'd like to see from Growth-Team on this?

Krinkle closed this task as Resolved.EditedAug 31 2022, 1:02 PM
Krinkle claimed this task.

@kostajh The current number suggests that there remains some signs of a misaligned overall approach. My main concern at this point would be what it might encourage/incentivize/makes-easy in future development by re-growing the registry. As counter example, I do note that Account.styles for example encourages bundling by default through its directory structure. This looks great.

Perhaps a compromise could be that going forward any addition is either avoided or paired with consolidation/removal of at least as many modules as are being added, so as to spread out the remaining debt as-needed over time rather than continuing the current sprint.

The ResourceLoader Bundle size dashboard can help to quickly assess at a glance the footprint of any given module. For example, Homepage.NewImpact appears to be ~0.3KB, and a quick look at the underlying structure suggests that the homepage feature has a structure that encourages creating a new RL bundle when adding new functionality. I don't know the Homepage feature very well, but if it seems likely that a given user will interact with multiple of its subfeatures within a span of a few days, then it might make sense to slowly move toward all of them being in one bundle that is async during the first view of that special page, and then future interacts have it all ready to go in local cache. I suspect this may currently be complicated by mixed use of OOUI and Vue, so perhaps this is something for T296646: [Epic] Migration of front-end modules to Vue.js components where e.g. a Homepage.Vue exists and any subfeatures using Vuew would not register an additional module.

I note that the directry structure is out-of-sync with some of the above merged refactors (ref T193826) which can sometimes make it harder to intuit what is loaded when, and where to start adding new functionality. I trust you'll keep an eye of this through code review, but FYI, this is an area where I find contributors often follow prior examples, which in the case of Account.styles makes it easy to add another file, but in the case of Homepage seems to make it easier to add a new top-level bundle.

Functionalty like DataStore and Logger might end up benefiting from a module like ext.growthExperiments.common to strike a balance if/when you find multiple modules on the same page are likely to contain the same files. Again, this would then be reflected on-disk as well through directory structure to aid in understanding dependencies, where to add new files with little resistence, and how to register those files.

Doc references: Page load perf guide, RL Development guide.