Page MenuHomePhabricator

Skip Expensive MobileFormatter Transformations On Pages With Extremely High Image/Heading counts
Closed, ResolvedPublic5 Estimated Story Points

Description

Per T204606, analysis was done on a subset of the Fatal error: entire web request took longer than 60 seconds errors found in logstash.

Most (but not all) of these errors were found to correlate to pages with either high image counts (> 1000) or high heading counts (> 1000). Therefore, if we want these pages to not timeout anymore, we could try skipping the MobileFormatter altogether under these scenarios.

Caveats:

  • Each of the cases I looked at had extremely high total element counts (> 50,000 total elements on the page). The suggested fix may remedy the majority of the timeout errors, however client side performance on mobile devices could still be pretty abysmal (e.g. slow/unuseable repaints, scroll freezing, etc). Therefore, it's probably not wise to spend a lot of time implementing this fix as it's really just a bandaid for the majority of cases.

Dev notes

  • Per Timo, image count might already be accessible from the ParserOutput object. This would be preferred over a regex since it would be a cheaper check.
  • For section count, using [[ https://www.php.net/manual/en/function.preg-match-all.php | preg_match_all ]] on the HTML string might work well. The downside of doing the regex is that there will be a cost added even for pages that are within the limits (majority of pages). Be sure to measure the cost of adding this regex! If the cost is too high, consider just checking the image count and foregoing the section check.

Acceptance Criteria

  • When a page has > 1000 images or > 1000 headings, don't run the MobileFormatter. Be conservative. Check these numbers with Nick- make as high as possible to start with.
  • The number of images/headings should be configurable.
  • Document the savings of execution time on big pages

QA steps

  • Click through some pages on mobile on the beta cluster. Check pages with headings have collapsible headings. After verifying this for a few pages move to sign off.

Sign off steps

  • A developer needs to check logstash and the impact on "time out" errors after this has been deployed. If it's significant report it to wikitech.

Event Timeline

The ParserOutput object might have some relevant information cheaply available as well, as stored while Parser ran (such as image links as pure data). That might forego the need for a regular expression. Worth checking out :)

Jdlrobson subscribed.

Looks ready for an estimate and discussion to me. Thanks for looking into this @nray !

Jdlrobson updated the task description. (Show Details)
nray updated the task description. (Show Details)

Change 539213 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Restrict the MobileFormatter to certain pages

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

Change 539213 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Restrict the MobileFormatter to certain pages

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

Change 539937 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] BREAKING: Don't run section collapsing on h1s

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

Change 539977 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Change ordering of MobileFormatter canApply logic.

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

Change 539977 abandoned by Jdlrobson:
Change ordering of MobileFormatter canApply logic.

Reason:
Okay now I remember why I didn't do this... :) A lot more complicated to unit test.

Piotr, if you're interested please feel free to restore and fix the unit test. It's not my priority right now.

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

Change 540107 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[operations/mediawiki-config@master] Set new MFMobileFormatterOptions config using old config

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

Change 540123 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[operations/mediawiki-config@master] Remove old and unused MFMobileFormatterHeadings config

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

Change 539937 abandoned by Jdlrobson:
BREAKING: Don't run section collapsing on h1s

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

Change 540107 merged by jenkins-bot:
[operations/mediawiki-config@master] Set new MFMobileFormatterOptions config using old config

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

Mentioned in SAL (#wikimedia-operations) [2019-10-02T11:20:50Z] <pmiazga@deploy1001> Synchronized wmf-config/InitialiseSettings.php: SWAT: [[gerrit:540107|Set new MFMobileFormatterOptions config using old config (T232690)]] (duration: 01m 01s)

I'll sign off the logstash check as I'm on chores duty on thursday and we need to get the impact on logstash.

Jdlrobson closed this task as Resolved.EditedOct 3 2019, 11:27 PM

Errors:
https://logstash.wikimedia.org/goto/447f5ffcb2ceb585b5b46c052e9baa7f

Screenshot 2019-10-03 at 4.49.58 PM.png (322×751 px, 21 KB)

We'll keep monitoring the situation in chores. I've unfiltered "timeout" errors from the results.

In last 24 hrs I've seen 1 pages timing out:
https://en.m.wikipedia.org/wiki/Wikipedia:Top_5000_pages

Change 540123 merged by jenkins-bot:
[operations/mediawiki-config@master] Remove old and unused MFMobileFormatterHeadings config

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

Mentioned in SAL (#wikimedia-operations) [2019-10-10T22:26:57Z] <jforrester@deploy1001> Synchronized wmf-config/InitialiseSettings.php: Stop setting wgMFMobileFormatterHeadings, unread T232690 (duration: 00m 51s)