Page MenuHomePhabricator

Performance regression from Apcu/ExtensionRegistry::loadFromQueue on PHP7
Open, HighPublic

Description

In reviewing gerrit change 409498, I noticed code for wgExtensionInfoMTime, which is an optimisation to avoid run-time overhead for checking mtimes on all extension.json files.

It seems this optimisation isn't enabled in production. If we don't need it, we should consider removing it (or adding tests for it). Alternatively, if we do need it, we should figure out how we want to enable it, and what it means for our deployment workflow.

Status

Details

ProjectBranchLines +/-Subject
mediawiki/coremaster+62 -24
mediawiki/coremaster+32 -34
mediawiki/extensions/Mathmaster+46 -22
mediawiki/extensions/Echomaster+3 -1
mediawiki/extensions/SecurePollmaster+883 -516
mediawiki/extensions/CirrusSearchmaster+42 -12
mediawiki/extensions/CirrusSearchmaster+42 -46
mediawiki/extensions/EventBusmaster+146 -51
mediawiki/extensions/MobileFrontendmaster+1 -2
mediawiki/extensions/Babelmaster+4 -7
mediawiki/skins/MinervaNeuemaster+3 -7
mediawiki/extensions/MassMessagemaster+72 -60
mediawiki/extensions/LocalisationUpdatemaster+21 -25
mediawiki/extensions/LocalisationUpdatemaster+13 -4
mediawiki/extensions/MassMessagemaster+3 -11
mediawiki/skins/Vectormaster+1 -4
mediawiki/extensions/WikimediaEventsmaster+5 -4
mediawiki/extensions/Collectionmaster+3 -8
mediawiki/extensions/PageImagesmaster+5 -5
mediawiki/extensions/PageAssessmentsmaster+4 -10
mediawiki/extensions/GettingStartedmaster+4 -14
mediawiki/extensions/InterwikiSortingmaster+3 -5
mediawiki/extensions/TemplateWizardmaster+3 -3
mediawiki/extensions/ExternalGuidancemaster+2 -3
mediawiki/extensions/JsonConfigmaster+2 -23
mediawiki/extensions/FileExportermaster+3 -3
mediawiki/extensions/GlobalUserPagemaster+2 -7
mediawiki/extensions/SiteMatrixmaster+3 -12
mediawiki/extensions/Graphmaster+2 -6
mediawiki/extensions/Flowmaster+3 -317
mediawiki/extensions/TocTreemaster+15 -50
mediawiki/coremaster+1 -0
mediawiki/coremaster+6 -7
Show related patches Customize query in gerrit

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

The patch for moving TestAutoloadClasses and TestAutoloadNamespaces to be lazy-loaded was rejected on the grounds that "we need to fix T240535: Clean up ExtensionRegistry autoloading mess first" for code cleanliness, but it feels like an obvious win to not load test code in production…

Krinkle added a comment.EditedFeb 20 2020, 4:48 AM

Source: P10669

Most common keys
184 url
184 name
184 MessagesDirs
184 manifest_version
183 descriptionmsg
182 license-name
182 author
174 type
168 Hooks
159 requires.MediaWiki
159 requires
152 author.0
146 config
139 AutoloadClasses
123 author.1
116 version
116 ResourceModules
110 ResourceFileModulePaths.localBasePath
110 ResourceFileModulePaths
109 ResourceFileModulePaths.remoteExtPath
 94 ExtensionMessagesFiles
 82 author.2
 74 SpecialPages
 54 Hooks.BeforePageDisplay
 54 author.3
 53 AutoloadNamespaces
 49 APIModules
 44 Hooks.LoadExtensionSchemaUpdates
 41 Hooks.ParserFirstCallInit
 41 Hooks.GetPreferences
 37 AvailableRights.0
 37 AvailableRights
 35 author.4
 32 GroupPermissions
 28 ConfigRegistry
 27 DefaultUserOptions
 26 callback
 25 requires.extensions
 25 JobClasses
 24 author.5
 23 TestAutoloadClasses
 23 load_composer_autoloader
 23 Hooks.ResourceLoaderTestModules
 22 LogActionsHandlers
 22 attributes
 22 APIListModules
 20 LogTypes.0
 20 LogTypes
 20 Hooks.ResourceLoaderRegisterModules
 20 Hooks.PageContentSaveComplete
 20 AvailableRights.1
 19 ServiceWiringFiles.0
 19 ServiceWiringFiles
 19 Hooks.ListDefinedTags
 19 Hooks.ChangeTagsListActive
 18 Hooks.ResourceLoaderGetConfigVars
 16 GroupPermissions.sysop
 16 author.6
 15 TrackingCategories.0
 15 TrackingCategories

Based on this, anything that isn't needed during WebStart/Setup.php, and isn't needed when doing common (read) operations with Title/User/Message objects, seems like a good candidate to try and make into a lazy-loaded attribute.

Note that there's virtually no run-time cost to making a key lazy-loaded. Even if we were to make all keys "lazy" loaded, it might still be a net-win performance wise as it would naturally mean we only fetch from APC what we actually need.

The reason I'm not suggesting that is because, except for a handful of these, for most it requires manual effort to actually convert first to a normal/propert attribute (e.g. where we no longer write it out to a global as well). And whether that change is safe is very much a judgement call on a case-by-case basis. Starting with the most frequently used keys, means carving out bigger chunks from the APC blob.

Krinkle updated the task description. (Show Details)Mar 9 2020, 6:22 PM
Krinkle updated the task description. (Show Details)
Krinkle added a comment.EditedMar 9 2020, 6:53 PM

Keys, sub keys, and attributes from extensions.json files in WMF production, listed by frequency (see T187154#5900039):

  • url
  • name (required for ExtensionRegistry->isLoaded)
  • MessagesDirs
  • manifest_version (hard-required)
  • descriptionmsg
  • license-name
  • author, author.0, author.1
  • type
  • Hooks, Hooks.* (required for wgHooks/Setup.php)
  • requires, requires.* (hard-required)
  • config (keep)
  • AutoloadClasses, AutoloadNamespaces (required for Setup.php)
  • version
  • ResourceModules, ResourceFileModulePaths – See T247265
  • ExtensionMessagesFiles
  • SpecialPages
  • APIModules
  • AvailableRights
  • GroupPermissions
  • ConfigRegistry
  • DefaultUserOptions
  • callback (required for Setup.php)
  • JobClasses
  • TestAutoloadClasses – See T240535
  • load_composer_autoloader (required for Setup.php)
  • LogActionsHandlers
  • attributes
  • APIListModules
  • LogTypes
  • ServiceWiringFiles (required for Setup.php)
  • TrackingCategories (Done) Lazy as of https://gerrit.wikimedia.org/r/559650/

With T247265 and various others above fixed and deployed, time to take another look:

For load.php (2019-09-29 samples):

  • 38.25% is spent within WebStart.php:
    • 17.28% in ExtensionRegistry::loadFromQueue.
    • 2.63% in MediaWikiServices::getDBLoadBalancer. – T228895

For api.php (2019-09-29 samples):

  • 24.67% is spent within WebStart.php:
    • 9.58% in ExtensionRegistry::loadFromQueue.
    • 1.46% in MediaWikiServices::getDBLoadBalancer.
    • 3.99% in SessionManager::getGlobalSession.

For index.php (2019-09-29 samples):

  • 13.19% is spent within WebStart.php:
    • 5.07% in ExtensionRegistry::loadFromQueue.
    • 0.72% in MediaWikiServices::getDBLoadBalancer.
    • 1.97% in SessionManager::getGlobalSession.

For load.php (2020-04-04):

  • 24.28% is spent within WebStart.php:
    • 11.45% in ExtensionRegistry::loadFromQueue .
    • 3.21% in MediaWikiServices::getDBLoadBalancer.

For api.php (2020-04-04):

  • 26.31% is spent within WebStart.php:
    • 9.84% in ExtensionRegistry::loadFromQueue .
    • 2.20% in MediaWikiServices::getDBLoadBalancer.
    • 3.99% in SessionManager::getGlobalSession.

For index.php (2020-04-04):

  • 12.98% is spent within WebStart.php:
    • 5.07% in ExtensionRegistry::loadFromQueue .
    • 1.08% in MediaWikiServices::getDBLoadBalancer.
    • 1.44% in SessionManager::getGlobalSession.
Krinkle added a comment.EditedApr 6 2020, 10:24 PM

Keys, sub keys, and attributes from extensions.json files in WMF production, listed by frequency (see T187154#5900039):

I dumped enwiki's "main" ExtensionRegistry cache value from APCu (from mwdebug1001, after a warmup).

You can browse it on deploy1001.eqiad.wmnet:/home/krinkle/enwiki-T187154-extregistry-main-apcu.json.

It is about 894K in size (as minified JSON).

In JSON pretty-print it is 29,204 lines of code, which I've used as a rough way to measure which keys take up the most space:

line of pretty json | keys

2231 wgExtensionCredits
1748 credits
1593 wgHooks
1018 wgAutoloadClasses
461 wgMessagesDirs
443 wgEcho*/wgDefaultNotify*/wgNotifyType*
329 wgCirrusSearch*
151 wgPageTriageTagsOptionsMessages
124 wgCentralNoticeCampaignMixins
111 wgLinterCategories
97 wgRawHtmlMessages
96 wgGroupPermissions
91 wgSpecialPages
79 wgAPIModules
71 wgExtensionMessagesFiles
64 wgMwEmbedModuleConfig
60 wgGrantPermissions
60 wgPageTriageDeletionTagsOptionsMessages
50 wgDefaultUserOptions

Of special note:

  • The vast majority of the blob is the default configurations from all extensions (as read from extension.json).
  • Much of the default configuration is stored twice, not sure why. Once as wg-key, and a second time in more verbose form under config. Presumably we only read one of them at run-time so we probably shouldn't store the unprocessed form. Might be as easy as an unset() somewhere.
  • The pattern "providedby": "…" is repeated nearly a thousand times through the verbose config format, but does not appear to be used for anything.

By far the biggest is 2231 wgExtensionCredits, 1748 credits. I'll try to tackle this one next. It's both something we presumably only need on Special:Version (where it can be fetched and cached as its own cache key), and also an interesting case of the same values being stored twice, which I haven't quite figured out why yet either.

Anomie added a comment.Apr 8 2020, 7:50 PM

It's both something we presumably only need on Special:Version (where it can be fetched and cached as its own cache key),

Looks like it's also read in ApiQuerySiteInfo (for output) and ApiBase (for mapping files to source extensions for the auto-generated help).

I see a lot of references to $wgExtensionCredits in extensions, most of which appear to be writing entries instead of using extension.json but a few are doing other things. Among Wikimedia-deployed extensions, I see only two:

None of that argues against it being lazy-loaded, IMO.

and also an interesting case of the same values being stored twice, which I haven't quite figured out why yet either.

Looks like ->credits came first, then T108269: $wgExtensionCredits handling in ExtensionProcessor/ExtensionRegistry is weird duplicated it into ->globals.

Change 587609 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/extensions/TocTree@master] Remove duplication of extension.json from TocTree (require MW 1.32+)

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

Change 587609 merged by jenkins-bot:
[mediawiki/extensions/TocTree@master] Remove duplication of extension.json from TocTree (require MW 1.32+)

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

and also an interesting case of the same values being stored twice, which I haven't quite figured out why yet either.

Looks like ->credits came first, then T108269: $wgExtensionCredits handling in ExtensionProcessor/ExtensionRegistry is weird duplicated it into ->globals.

Not really sure what 2015 me was thinking in that task.

Based on your and Krinkle's analysis, I think we should have ExtensionRegistry stop adding credits to the global but continue to support allowing legacy PHP entrypoints to write/append to $wgExtensionCredits. Then we introduce a PHP function somewhere that combines the two, which can be called from Special:Version and the API. Trying to read from $wgExtensionCredits outside of that function is discouraged/deprecated and becomes hit or miss basically.

Change 589577 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Flow@master] Move most Flow files to AutoloadNamespaces

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

Change 555008 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/SecurePoll@master] Namespace-ification!

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

Change 589577 merged by jenkins-bot:
[mediawiki/extensions/Flow@master] Move most Flow files to AutoloadNamespaces

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

Change 589775 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/JsonConfig@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589777 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/SiteMatrix@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589778 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/PageAssessments@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589779 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/GettingStarted@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589780 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Graph@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589784 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/GlobalUserPage@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589785 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/FileExporter@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589787 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/InterwikiSorting@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589788 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/ExternalGuidance@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589790 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/TemplateWizard@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589780 merged by jenkins-bot:
[mediawiki/extensions/Graph@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589777 merged by jenkins-bot:
[mediawiki/extensions/SiteMatrix@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589784 merged by jenkins-bot:
[mediawiki/extensions/GlobalUserPage@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589775 merged by jenkins-bot:
[mediawiki/extensions/JsonConfig@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589785 merged by jenkins-bot:
[mediawiki/extensions/FileExporter@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589788 merged by jenkins-bot:
[mediawiki/extensions/ExternalGuidance@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589790 merged by jenkins-bot:
[mediawiki/extensions/TemplateWizard@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589787 merged by jenkins-bot:
[mediawiki/extensions/InterwikiSorting@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589779 merged by jenkins-bot:
[mediawiki/extensions/GettingStarted@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589778 merged by jenkins-bot:
[mediawiki/extensions/PageAssessments@master] Swap AutoloadClasses for AutoloadNamespaces

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

Change 589798 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/skins/Vector@master] Move some more classes to AutoloadNamespaces

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

Change 589799 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Babel@master] Move some classes to AutoloadNamespaces

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

Change 589801 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/MassMessage@master] Move some classes to AutoloadNamespaces

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

Change 589802 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Collection@master] Move some classes to AutoloadNamespaces

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

Change 589803 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CirrusSearch@master] Move some classes to AutoloadNamespaces

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

Change 589805 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/WikimediaEvents@master] Move some classes to AutoloadNamespaces

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

Change 589807 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/PageImages@master] Move some classes to AutoloadNamespaces

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

Change 589799 merged by jenkins-bot:
[mediawiki/extensions/Babel@master] Move some classes to AutoloadNamespaces

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

Change 589802 merged by jenkins-bot:
[mediawiki/extensions/Collection@master] Move some classes to AutoloadNamespaces

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

Change 589807 merged by jenkins-bot:
[mediawiki/extensions/PageImages@master] Move some classes to AutoloadNamespaces

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

Change 589801 merged by jenkins-bot:
[mediawiki/extensions/MassMessage@master] Move some classes to AutoloadNamespaces

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

Change 589805 merged by jenkins-bot:
[mediawiki/extensions/WikimediaEvents@master] Move some classes to AutoloadNamespaces

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

Change 589798 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Move some more classes to AutoloadNamespaces

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

Change 589863 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/MassMessage@master] Fixup namespacing in MassMessage

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

Change 589865 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/LocalisationUpdate@master] Namespace LocalisationUpdate class

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

Change 589866 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/LocalisationUpdate@master] Fix namespacing for rest of the classes

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

Change 589867 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/skins/MinervaNeue@master] Move some more classes to AutoloadNamespaces

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

Change 589870 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/EventBus@master] Correctly namespace Rest classes

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

Change 589863 merged by jenkins-bot:
[mediawiki/extensions/MassMessage@master] Fixup namespacing in MassMessage

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

Change 589865 merged by jenkins-bot:
[mediawiki/extensions/LocalisationUpdate@master] Namespace LocalisationUpdate class

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

Change 589866 merged by jenkins-bot:
[mediawiki/extensions/LocalisationUpdate@master] Fix namespacing for rest of the classes

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

Change 589867 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Move some more classes to AutoloadNamespaces

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

Change 590004 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/MobileFrontend@master] Move some classes to AutoloadNamespaces

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

Change 590006 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CirrusSearch@master] Namespace maintenance scripts and use AutoloadNamespaces

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

Change 590009 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/Echo@master] Move one class to AutoloadNamespaces

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

Change 590004 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move some classes to AutoloadNamespaces

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

Change 589870 merged by jenkins-bot:
[mediawiki/extensions/EventBus@master] Namespace EventBus extension

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

Change 589803 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] Move some classes to AutoloadNamespaces

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

Change 591126 had a related patch set uploaded (by Reedy; owner: Reedy):
[mediawiki/extensions/CirrusSearch@master] Use AutoloadNamespaces for maintenance scripts

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

Krinkle updated the task description. (Show Details)Apr 20 2020, 7:23 PM

Change 591126 merged by jenkins-bot:
[mediawiki/extensions/CirrusSearch@master] Use AutoloadNamespaces for maintenance scripts

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

Change 555008 merged by jenkins-bot:
[mediawiki/extensions/SecurePoll@master] Namespace-ification!

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

Change 590009 merged by jenkins-bot:
[mediawiki/extensions/Echo@master] Move one class to AutoloadNamespaces

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

Change 595982 had a related patch set uploaded (by Physikerwelt; owner: Physikerwelt):
[mediawiki/extensions/Math@master] Move checking code to a new namespace

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

Change 595982 merged by jenkins-bot:
[mediawiki/extensions/Math@master] Move checking code to a new namespace

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

Change 598522 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] SpecialVersion: Consolidate internal use of wgExtensionCredits

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

Change 598523 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] ExtensionRegistry: Remove exporting and caching of wgExtensionCredits

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

Change 598522 merged by jenkins-bot:
[mediawiki/core@master] SpecialVersion: Consolidate internal use of wgExtensionCredits

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

Change 598523 merged by jenkins-bot:
[mediawiki/core@master] ExtensionRegistry: Remove exporting and caching of wgExtensionCredits

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