Page MenuHomePhabricator

Avoid significant "subjectpageheader" parsing on every pageview
Closed, ResolvedPublic

Description

[…]
10% ~40ms : Article::showNamespaceHeader uncached wikitext parsing and Scribunto calls. This was enabled on article pages last year in T151682 as a way to achieve "PageNotice" functionality (before that, this function was limited to talk pages).
[…]

A few ideas:

  • Disable this feature in the main namespace in WMF production. This is similar to other optimizations in core, and could be conditional on $wgMiserMode.
  • Split this feature by namespace ID. Instead of reading MediaWiki:Subjectpageheader we could read MediaWiki:Subjectpageheader-1 where 1 is the namespace ID. This is similar to how we handle Editnotice. This would mean the expensive code only runs in namespaces where a given community has chosen to enable it. This could be combined with the first idea, e.g. split by namespace to avoid the overhead in most namespaces, and as defense in depth explicitly disallow the NS0 (MediaWiki:Subjectpageheader-0) under $wgMiserMode.
  • Split this feature by content model (per original use case T151682#10065952 and T355159). Instead of reading MediaWiki:Subjectpageheader we could read MediaWiki:Subjectpageheader-css, MediaWiki:Subjectpageheader-Scribunto etc where the last segment is the content model ID ($wgContentHandlers key) . We can then cleanly exclude "wikitext" from the feature, because wikitext pages can and should should simply place any shared template in their wikitext slot instead. This would not need to involve wgMiserMode or WMF-specific configuration.

Another idea:

  • Move the computation from the uncached skin layer, to behind the ParserCache, e.g. in setExtensionData or some other extra field that the skin can pull out afterward.

Afaik the return value does not vary by user or request, only by title.

See also:

  • T408264 which proposes ParserCache for this message for a different reason (to allow categories). This task is orthogonal from that. We can compute this in the ParserOutput object while continuing to process it as an interface message without any metadata exports, which I suggest we keep that way within the scope of this task.
  • T151682#7623800 which talks about why we can't do per-namespace caching

Event Timeline

Krinkle added subscribers: Zabe, matmarex.

@matmarex @Zabe As consultants/reviewers of the original implementation at T151682, do you have any insights to inform which choice we should make or otherwise have a preference for one approach vs another?

Summary of audit

The feature is used on 4 wikis: enwiki, hrwiki, thwiki, dewiki

  • ✅ enwiki: varies cleanly by content model, unused on wikitext or NS0 pages.
  • ❌ hrwiki: used on one namspace (Draft, NS 118) which is wikitext. Unclear why because enwiki has a different solution already which predates this one.
  • ✅ thwiki: varies cleanly by content model unused on wikitext or NS0 pages.
  • ✅ dewiki: varies cleanly by content model unused on wikitext or NS0 pages.

Audit details

https://global-search.toolforge.org/?q=.*&regex=1&namespaces=8&title=Subjectpageheader

enwiki usage: https://en.wikipedia.org/wiki/MediaWiki:Subjectpageheader calls https://en.wikipedia.org/wiki/Module:Subject_page_header which is basically:

if title.contentModel == 'json' and  ..:
  ...
elseif title.contentModel == 'SecurePoll' and ..:
  ...
else
  return ""

https://hr.wikipedia.org/wiki/MediaWiki:Subjectpageheader

{{Zaštićen}}<!--
-->{{#switch:{{NAMESPACENUMBER}}|
|118={{Nacrt}}
}}
  • NS 118 is the Draft namespace. This unconditionally prepends a message above all such pages. Other wikis solve this by adding the template directly as part of the wizard that creates the draft, with possibly an AbuseFilter rule to prevent removal. That might be preferred, given these are wikitext, which is not what the feature was developed for.
  • Padlock icon on protected articles. Parsed on all articles, and if it's protected, it tries to add a page status indicator. This doesn't appear to work, because it requires page metadata to be merged, which we purposefully don't, because it would break caching and fall out of sync with link tables. Other wikis solve this with a gadget, or with a template on the page (added to at least all protected by convention or bot, and naturally no-ops after protection expires or is removed). I checked https://hr.wikipedia.org/wiki/Special:Log/protect for articles that are recently fully/admin protected such as https://hr.wikipedia.org/wiki/Ivan_Anušić and indeed this doesn't actually work. EDIT: It does work in terms of showing the icon, but not in terms of categories or link tables. I mistakenly thought it didn't work because my account is set to English and this feature parses MediaWiki:Subjectpageheader in the interface langauge instead of the content language, and so was actually parsing MediaWiki:Subjectpageheader/en. It works at https://hr.wikipedia.org/wiki/Ivan_Anu%C5%A1i%C4%87?useskin=vector&safemode=on&uselang=hr

https://th.wikipedia.org/wiki/MediaWiki:Subjectpageheader -> https://th.wikipedia.org/wiki/Module:Subject_page_header. This was copied from enwiki and does the same thing.

if title.contentModel == 'json' and  ..:
  ...
elseif title.contentModel == 'SecurePoll' and ..:
  ...
else
  return ""

https://de.wikipedia.org/wiki/MediaWiki:Subjectpageheader

Splits cleanly by content model and namespace. Either approach works. If we split by one, then they switch by the other. Or vice versa.

{{#switch: {{NAMESPACENUMBER}}
  |2|4|8|10|100|828 = {{#switch: {{#contentmodel:canonical}}
    |css
    |javascript
    |json
    |sanitized-css=…
    |text=…
  }}
}}
Krinkle triaged this task as High priority.Wed, Aug 12, 6:17 PM

Change #1324806 had a related patch set uploaded (by Jforrester; author: Jforrester):

[mediawiki/core@master] Article:showNamespaceHeader: Disable in miser mode for non-talk pages

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

Change #1326351 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] Article: Split subjectpageheader by model and disable for wikitext

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

@matmarex @Zabe As consultants/reviewers of the original implementation at T151682, do you have any insights to inform which choice we should make or otherwise have a preference for one approach vs another?

It makes sense to me to split the message based on content model (or by namespace ID). We handle 'editnotice-...' messages similarly for similar reasons.

Change #1326351 merged by jenkins-bot:

[mediawiki/core@master] Article: Split subjectpageheader by model and disable for wikitext

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

This is based on analyzing a pageview (logged-out, cache miss via WikimediaDebug, mw-experimental-eqiad with 1 warmup for opcache and ParserCache, instrumenting a hard refresh): https://performance.wikimedia.org/excimer/profile/b630781ba01d857c.

The total pre-send latency is 470ms (/w/index.php, sans postOutputShutdown), and that's logged-out, with a ParserCache hit, on a warmed-up server.

[…]

  • [@25%] 10% ~40ms : Article::showNamespaceHeader uncached wikitext parsing and Scribunto calls. This was enabled on article pages last year in T151682 as a way to achieve "PageNotice" functionality (before that, this function was limited to talk pages).

Updated profile, same settings, same scenario: https://performance.wikimedia.org/excimer/profile/00849b73732ea83e

Total pre-send latency is now 403ms. There's been several other changes rolled out since then as well (see parent task).

The segment attributed to Article::showNamespaceHeader no longer exists in the latest profile, which means it is below the sampling interval (0.1ms).

It's nearest parent (MediaWiki\Page\Article::view) went from 159ms to 126ms (-33ms). There's some natural variance from run to run.