Page MenuHomePhabricator

2% of /w/load.php spent in WebStart/onBeforeCreateEchoEvent for Wikibase
Closed, ResolvedPublic



  • EchoHooks::initEchoExtension (3.05%)
    • Wikibase\Client\Hooks\EchoNotificationsHandlers::onBeforeCreateEchoEvent
    • Wikibase\Client\Hooks\EchoNotificationsHandlers::newFromGlobalState (2.63%)
	public static function onBeforeCreateEchoEvent(
		array &$notifications,
		array &$notificationCategories,
		array &$icons
	) {
		$self = self::newFromGlobalState();
		$self->doBeforeCreateEchoEvent( $notifications, $notificationCategories, $icons );

For example:

The doBeforeCreateEchoEvent() method call itself is trivial. Most time is spent in newFromGlobalState(), which requires numerous services that are not used.

  • NamespaceChecker (unused)
  • RepoLinker (unused)
  • WikibaseClient (mostly unused, except for Settings)
    • WikibaseClient//SiteLookup (unused)
    • WikibaseClient//DataTypeDefinitions (unused)
    • WikibaseClient//Settings (used)

The most expensive here is SiteLookup and NamespaceChecker, which are both created here for the unrelated hook onWikibaseHandleChange - which is not called on most requests and not worth blocking WebStart on all web requests.

From a brief look it seems that the main problem is that all hook handlers share the same singleton for injection (instead of obtaining their own services as needed), which means calling one hook ends up also requiring services for other hooks.

A deeper problem is that these services have expensive constructors, which is unexpected, and likely the reason why this approach was assumed to be unproblematic.


See also: T160678: Reduce ChronologyProtector init cost for load.php

Event Timeline

Krinkle created this task.Oct 3 2017, 6:35 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 3 2017, 6:35 PM
Gilles moved this task from Inbox to Radar on the Performance-Team board.Oct 4 2017, 7:06 PM
Gilles edited projects, added Performance-Team (Radar); removed Performance-Team.
Gilles moved this task from Limbo to Perf issue on the Performance-Team (Radar) board.
Krinkle added a comment.EditedOct 4 2017, 11:47 PM

The above flame graph is from HHVM profiling samples over 24 hours from production traffic. While useful to see the overall impact on average, it does not provide timings (due to being sampled). Here two profiles from XHGui for an individual request (including flamegraph). (total: 1.3s)

  • EchoNotificationsHandlers::onBeforeCreateEchoEvent (111ms, 8%) (total: 45s)

  • EchoNotificationsHandlers::onBeforeCreateEchoEvent (231ms, 1%)
hoo added a subscriber: hoo.Oct 5 2017, 12:03 AM

This should probably indeed be split up into different handlers, which might solve some of the problems here.

Also we could probably make WikibaseClient faster by lazy-creating the SiteLookup.

Regarding the NamespaceChecker construction: This is in parts so expensive due to the closure in mediawiki-config ($wgWBClientSettings['excludeNamespaces'] = function () { … } in wmf-config/Wikibase.php). We should try to avoid getting this setting if possible, but I don't see a nice way to make this faster.

Change 382383 had a related patch set uploaded (by Aude; owner: Aude):
[mediawiki/extensions/Wikibase@master] Split Echo hook handlers based on services needed

Change 382386 had a related patch set uploaded (by Aude; owner: Aude):
[mediawiki/extensions/Wikibase@master] Split hook handler that runs on Echo extension setup

Change 382386 abandoned by Aude:
Split hook handler that runs on Echo extension setup

aude claimed this task.Oct 5 2017, 8:40 AM

Change 382383 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Split Echo hook handlers based on services needed

thiemowmde triaged this task as Low priority.Oct 5 2017, 10:03 AM
thiemowmde moved this task from incoming to in progress on the Wikidata board.
thiemowmde added a subscriber: Lydia_Pintscher.
Krinkle added a comment.EditedOct 5 2017, 9:18 PM

Change 382383 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Split Echo hook handlers based on services needed

@aude Thanks for the quick patch!

	public static function newFromGlobalState() {
-		$wikibaseClient = WikibaseClient::getDefaultInstance();
-		$settings = $wikibaseClient->getSettings();
+		$settings = WikibaseSettings::getClientSettings();

		return new self(
-			$wikibaseClient->newRepoLinker(),
-			$wikibaseClient->getNamespaceChecker(),
			$settings->getSetting( 'echoIcon' )

Very nice. I didn't know WikibaseSettings::getClientSettings existed. That also avoids creating WikibaseClient's services.

aude added a comment.Oct 5 2017, 11:02 PM

@Krinkle is it okay for this to wait to go out w/ the train next week or should we consider a backport?

(while I'm sure the patch works good, I would prefer the train, so the code has time on test wikis etc.)

Krinkle added a comment.EditedOct 5 2017, 11:17 PM

@aude Next week is fine. This isn't a regression from current week's branch.

thiemowmde updated the task description. (Show Details)

@aude, @Krinkle, can we consider this done? Do you have new numbers to compare with the previous state as it is described in this tickets description?

For the moment I'm moving this ticket back, since it seems nothing happened for about a month.

Krinkle closed this task as Resolved.Oct 26 2017, 4:31 PM

Yep, this is resolved. It's no longer part of the trace for load.php. Thanks!.