Page MenuHomePhabricator

Adjust ExtensionRegistry to avoid the slowness of array_key_exists on `$GLOBALS` in PHP 8.1+
Closed, ResolvedPublic

Description

See T317951 for more context

FYI: PHP 8.1 made major changes to how $GLOBALS works, and one of the side effects is that using array_key_exists on $GLOBALS is now a linear-time operation instead of constant-time. MediaWiki uses this construct pretty extensively in GlobalVarConfig, which is ~1000 times slower in 8.1 than it is in 8.0, due to $GLOBALS having 1000-2000 entries in it.

The GlobalVarConfig usage was fixed in T317951 by @Jdforrester-WMF, but while playing around with the (excellent) Excimer+Speedscope tooling, I noticed ExtensionRegistry::exportExtractedData was taking about 6ms in PHP 8.1 compared to 0.1-0.2ms in PHP 7.4. The culprit once again was array_key_exists on $GLOBALS - the relevant code in exportExtractedData calls array_key_exists at least once for every global registered by extensions, which in our case is about 700, but I think for WMF may be higher. Each one of these copies $GLOBALS, which contains about 1000-2000 entries. AFAICT this function is called on every api.php/load.php/index.php request, so I suspect it's quietly adding ~5-10ms to every request on any 8.1 wikis.

So, what's the fix? I filed a bug against PHP core about this 2 years ago, but the general agreement was that it was intended behavior. The type of fix

isset( $GLOBALS[$var] ) || array_key_exists( $var, $GLOBALS );

done in T317951 doesn't work very well here – the distinction is that GlobalVarConfig was almost always dealing with entries where isset($GLOBALS[$var]) was true, which meant the array_key_exists branch is never hit. Comparatively, most of the merging in exportExtractedData is done on keys that are not yet in $GLOBALS, which means that (at least in our case) about 80% of the keys returned false for isset, and needed to hit array_key_exists anyway.

One alternative is to make a copy of $GLOBALS at the beginning of exportExtractedData and then call array_key_exists($key, $fakeGlobals). I believe this will always give correct results, but it's fairly hard to reason about since $GLOBALS is continuing to mutate, and it would be easy in the future to accidentally introduce additional logic to extension registration such that the "optimization" becomes incorrect.

What do you think?

Event Timeline

Change #1040245 had a related patch set uploaded (by Umherirrender; author: Umherirrender):

[mediawiki/core@master] ExtensionRegistry: Avoid array_key_exists on $GLOBALS in PHP 8.1+

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

Change #1043064 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Optimize globals processing in ExtensionRegistry for performance

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

Change #1040245 merged by jenkins-bot:

[mediawiki/core@master] ExtensionRegistry: Avoid array_key_exists on $GLOBALS in PHP 8.1+

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

Change #1043064 merged by jenkins-bot:

[mediawiki/core@master] Optimize globals processing in ExtensionRegistry for performance

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

Umherirrender claimed this task.