Page MenuHomePhabricator

Performance regression from Apcu/ExtensionRegistry::loadFromQueue on PHP7
Closed, ResolvedPublic

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
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

Krinkle closed this task as Resolved.Jun 9 2020, 6:52 PM

Unfortunately ExtensionRegistry::loadFromQueue and its APCUBagOStuff::doGet call are still taking up prettuy much the same 8-10% of load.php as they did before we started.

I'm gonna close this for now as I've spent a lot of time on this and I'm just accepting for now that php-apcu is significantly slower than hhvm-apc. We know this already and it make sense considering that it has to copy these deep and wide arrays recursively into the current request threads whereas HHVM was cheating a bit by not doing any kind of GC or LRU on its APC memory, thus allowing threads to just read the shared memory directly without worrying about things falling out mid-request.

We've optimised it a bit, and it may've made a small improvement but not enough to have high confidence, and definitely not enough to have recovered from the regression. For now I'll shift my focus on other tasks that affect MW WebStart performance and regain our response times that way instead.

Examples:

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:40 PM