Page MenuHomePhabricator

Reduce db queries in load.php calls
Open, In Progress, MediumPublic

Description

load.php is a critical path that needs to respond as quickly as possible to avoid breaking user experience and given that they are being called a lot (23% of all requests are to load.php = ~2B req/day), this redactions will have a major impact.

Looking at the queries logged by verbose logging:

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Marostegui triaged this task as Medium priority.Jan 7 2022, 6:24 AM
Marostegui moved this task from Triage to Refine on the DBA board.

I added both of these recently for T284097. If there is a better way to have some localisation messages that support full wikitext syntax, and have the ResourceLoader cache invalidated when the
messages change, I'm all ears.

(Maybe we could reduce the number of queries by doing one big query manually instead of using getTouched() for each title, like ResourceLoaderWikiModule does – not sure if that would be much better?)

  • DiscussionTools is triggering several db queries in load.php due to getting called in HookUtils::isAvailableForTitle which sorta doesn't make sense and it seems it's due to the hook handler onParserAfterTidy which gets triggered in every parse including parsing of mediawiki messages (6 of them happen in the load.php I examined) so if this needs to be on parsing of the page, it probably need to hook somewhere else in mediawiki not on ParserAfterTidy

This probably means some other code is parsing wikitext localisation messages without setting the interface flag. Can you track down which ResourceLoader modules are causing that?

(We previously discussed this in T291601, and I fixed this problem for a majority of cases.)

This probably means some other code is parsing wikitext localisation messages without setting the interface flag. Can you track down which ResourceLoader modules are causing that?

Well, I guessed one, maybe that is all: T298822: Copyright messages in ext.visualEditor.data (VisualEditorDataModule) are parsed without the interface flag

For context, I believe use of page_touched here was likely based on my recommendation at: T189229: VisualEditorDataModule spends significant time in hot startup module

Basically, it avoids the cost that's worse than a one cheap db query, which is a parser run and a dozen more queries.

This probably means some other code is parsing wikitext localisation messages without setting the interface flag. Can you track down which ResourceLoader modules are causing that?

I can give you the queries made in load.php requests. Is that useful? https://logstash.wikimedia.org/goto/fad5ac21e85939eebc78761b44dcf582

I added both of these recently for T284097. If there is a better way to have some localisation messages that support full wikitext syntax, and have the ResourceLoader cache invalidated when the
messages change, I'm all ears.

My thinking is that having an APCu cache on it for an hour should be okay, they don't take much space and being distributed in 200 appservers make the actual ttl much smaller and I don't think having outdated information for an hour is that big of a deal (I'm biased though). For example site lookup has APCu with ttl of an hour and when we add a new wiki, it takes an hour to show up in lists.

I can give you the queries made in load.php requests. Is that useful? https://logstash.wikimedia.org/goto/fad5ac21e85939eebc78761b44dcf582

Kind of useful, but I haven't managed to find any other modules doing parsing wrong like ext.visualEditor.data. So maybe it is the only one, which would be nice.

I learned a few other things from it though:

  • WikiLove and PageTriage are doing some queries from load.php, because they use ResourceLoaderWikiModule – I think this is expected
  • GrowthExperiments is doing some queries from load.php for the startup module, because it parses some messages in a packageFiles callback, e.g. when generating GEHelpPanelLinks in a few modules – this is a similar issue to T189229

GuidedTours is making one/two db queries caused by onResourceLoaderGetConfigVars T264817#7601839

At a glance the only nontrivial logic there is Title::exists() which has its internal cache mechanism (via the LinkCache, which stores titles in the WAN cache for one day). Since the check is always done for the same title, the DB traffic should be minimal (and I don't see how that call could ever result in two queries). I guess APC could speed it up marginally, but wouldn't change the number of DB queries. Could something be broken with the tracking? Or with the LinkCache mechanism? (Or maybe there is some TitleExists hook handler doing nontrivial work? But that wouldn't necessarily be related to GuidedTours.)

GrowthExperiments is making several db queries in unloading package files, caused by getSuggestedEditsConfigJson. These need APCu cache: https://performance.wikimedia.org/xhgui/run/symbol?id=61d700876ddcc011535c3a61&symbol=GrowthExperiments%5CHomepageHooks%3A%3AgetSuggestedEditsConfigJson

getSuggestedEditsConfigJson uses GrowthWikiConfig, which is cached (for one day in memcached + in in-process cache). APC would speed it up a little, but 4ms seems way too much for a memcached call. Not sure what's going on there...

Flow is doing similar to GT in getTermsOfUseMessagesVersion (but instead of calling Title::exists(), it calls Title::getTouched() triggering a db query: https://performance.wikimedia.org/xhgui/run/symbol?id=61d700876ddcc011535c3a61&symbol=Title%3A%3AgetTouched)

That should also be handled by LinkCache.

GrowthExperiments is triggering several queries through HelpPanelHooks https://performance.wikimedia.org/xhgui/run/symbol?id=61d700876ddcc011535c3a61&symbol=GrowthExperiments%5CHelpPanelHooks%3A%3AgetModuleData

Also uses GrowthWikiConfig.

  • GrowthExperiments is doing some queries from load.php for the startup module, because it parses some messages in a packageFiles callback, e.g. when generating GEHelpPanelLinks in a few modules – this is a similar issue to T189229

Hm, that's possible, we do parse a configuration variable to allow using {{#time}} in page name patterns. I guess that would need its own caching logic?
In this case we have an easy way out since we don't need that variable anymore - we only use it if $wgGEHelpPanelAskMentor is false and it is always true in production. We just need to make sure the message parsing doesn't happen in that case.

ppelberg added a subscriber: ppelberg.

Note: the Editing Team does not understand the issues raised with Discussion Tools to be urgent nor do they see a clear way of optimizing the code referenced in this task without sacrificing important functionality (e.g. localizing interface messages). As such, we're not going to prioritize work on this. If there is any information that leads you to a different conclusion from the one I've shared above, please ping us!

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

[mediawiki/extensions/GuidedTour@master] Do not load help/guider URL when the accompanying tour is not used

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

Change 756104 merged by jenkins-bot:

[mediawiki/extensions/GuidedTour@master] Do not load help/guider URL when the accompanying tour is not used

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

This probably means some other code is parsing wikitext localisation messages without setting the interface flag. Can you track down which ResourceLoader modules are causing that?

Well, I guessed one, maybe that is all: T298822: Copyright messages in ext.visualEditor.data (VisualEditorDataModule) are parsed without the interface flag

The patch for this is merged now.

I went looking, and found another one in Wikibase with this search: https://codesearch.wmcloud.org/search/?q=wfMessage&i=nope&files=.*Module.*&excludeFiles=&repos= (other results look like false positives)

Change 757449 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/Wikibase@master] SitesModule: Replace wfMessage() with use of MessageLocalizer

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

Change 757449 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] SitesModule: Replace wfMessage() with use of MessageLocalizer

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

Urbanecm_WMF changed the task status from Open to In Progress.Jan 31 2022, 11:33 AM

A somewhat related issue (but one that cannot be solved with caching) is that these DB calls block T155147: Do not initialise the database in tests when not needed; there should be some way to disable them in tests which are not that extension's hook handler's own tests.