Page MenuHomePhabricator

Duplicate queries on page views (SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Current_user')
Closed, ResolvedPublic

Description

On Special:BlankPage I see 24 queries (many to objectcache), but what caught my attention is that this simple query is repeated 4 times:

18	localhost: SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Developer' LIMIT 1	1.0000ms	User::load
21	localhost: SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Developer' LIMIT 1	0.0000ms	User::load
22	localhost: SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Developer' LIMIT 1	0.0000ms	User::load
23	localhost: SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Developer' LIMIT 1	0.0000ms	User::load

This seems wasteful. Why isn't the user object caching this properly?

--- a/includes/user/User.php
+++ b/includes/user/User.php
@@ -374,7 +374,7 @@ class User implements IDBAccessObject, UserIdentity {
                    'actor',
                    [ 'actor_id', 'actor_user', 'actor_name' ],
                    $this->mFrom === 'name' ? [ 'actor_name' => $this->mName ] : [ 'actor_id' => $this->mActorId ],
-                   __METHOD__,
+                   wfGetAllCallers( 6 ),
Queries log
=> SkinTemplate->prepareQuickTemplate/Skin->bottomScripts/OutputPage->getBottomScripts/OutputPage->getRlClient/ResourceLoaderWikiModule::preloadTitleInfo/ResourceLoaderUserStylesModule->getPages/User->isAnon/User->isRegistered/User->getId/User->load

41	SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Admin' LIMIT 1
	User::load
42	SELECT user_id,user_name,user_real_name,user_email,user_touched,user_token,
	user_email_authenticated,user_email_token,user_email_token_expires,user_registration,user_editcount,user_actor.actor_id
	FROM `user` JOIN `actor` `user_actor` ON ((user_actor.actor_user = user_id)) WHERE user_id = 1 LIMIT 1
	User::loadFromDatabase
43	SELECT ug_user,ug_group,ug_expiry FROM `user_groups` WHERE ug_user = 1
	UserGroupMembership::getMembershipsForUser

=> OutputPage->getBottomScripts/ResourceLoaderClientHtml->getBodyHtml/ResourceLoaderClientHtml->getData/ResourceLoaderWikiModule->isKnownEmpty/ResourceLoaderWikiModule->getTitleInfo/ResourceLoaderUserModule->getPages/User->isAnon/User->isRegistered/User->getId/User->load
45	SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Admin' LIMIT 1
	User::load
46	SELECT user_id,user_name,user_real_name,user_email,user_touched,user_token,
	user_email_authenticated,user_email_token,user_email_token_expires,user_registration,user_editcount,user_actor.actor_id 
	FROM `user` JOIN `actor` `user_actor` ON ((user_actor.actor_user = user_id)) WHERE user_id = 1 LIMIT 1
	User::loadFromDatabase
47	SELECT ug_user,ug_group,ug_expiry FROM `user_groups` WHERE ug_user = 1
	UserGroupMembership::getMembershipsForUser

=> OutputPage->headElement/ResourceLoaderClientHtml->getHeadHtml/ResourceLoaderClientHtml->getData/ResourceLoaderWikiModule->isKnownEmpty/ResourceLoaderWikiModule->getTitleInfo/ResourceLoaderUserModule->getPages/User->isAnon/User->isRegistered/User->getId/User->load
48	SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Admin' LIMIT 1
	User::load
49	SELECT user_id,user_name,user_real_name,user_email,user_touched,user_token,
	user_email_authenticated,user_email_token,user_email_token_expires,user_registration,user_editcount,user_actor.actor_id
	FROM `user` JOIN `actor` `user_actor` ON ((user_actor.actor_user = user_id)) WHERE user_id = 1 LIMIT 1
	User::loadFromDatabase
50	SELECT ug_user,ug_group,ug_expiry FROM `user_groups` WHERE ug_user = 1
	UserGroupMembership::getMembershipsForUser

=> ResourceLoader->makeModuleResponse/ResourceLoaderModule->getModuleContent/ResourceLoaderModule->buildContent/ResourceLoaderUserOptionsModule->getScript/User->getEditToken/User->getEditTokenObject/User->isAnon/User->isRegistered/User->getId/User->load
51	SELECT actor_id,actor_user,actor_name FROM `actor` WHERE actor_name = 'Admin' LIMIT 1
	User->load
52	SELECT user_id,user_name,user_real_name,user_email,user_touched,user_token,
	user_email_authenticated,user_email_token,user_email_token_expires,user_registration,user_editcount,user_actor.actor_id
	FROM `user` JOIN `actor` `user_actor` ON ((user_actor.actor_user = user_id)) WHERE user_id = 1 LIMIT 1
	User::loadFromDatabase

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptApr 2 2020, 7:11 AM
Anomie moved this task from Inbox to Triage Meeting Inbox on the Core Platform Team board.
Anomie added a subscriber: Anomie.

It's more likely 4 separate User objects. We don't have a "UserFactory" that tries to return the same User object each time a user is requested, and User's static methods don't try to do that either.[1]

When I try this locally with some additional logging, I find one call during Session setup (checking if the user name in the session actually exists), and six from ResourceLoader during page output. Regarding the latter, it looks like ResourceLoaderContext does try to cache the User but that doesn't help much when ResourceLoaderClientHtml::getData() creates a new ResourceLoaderContext for each module group (and also never actually caches the data it calculates into $this->data).

[1]: Note there's a bunch of other work going on with respect to User, and tasks with otherwise non-obvious requirements on User object reuse like T218674 and T231930. If someone wants to try making a UserFactory, they should make themselves aware of all that first.

Restricted Application added a project: Performance-Team. · View Herald TranscriptApr 2 2020, 2:46 PM
Gilles assigned this task to Krinkle.Apr 6 2020, 7:58 PM
Gilles moved this task from Inbox to Doing on the Performance-Team board.
Krinkle triaged this task as Low priority.Apr 7 2020, 12:54 AM
CCicalese_WMF added a subscriber: CCicalese_WMF.

Untagging Core Platform Team for now. Please feel free to re-tag if you need review.

Change 588516 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Derive from existing Context object in ClientHtml

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

Krinkle updated the task description. (Show Details)Apr 14 2020, 2:28 AM
Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptApr 14 2020, 2:28 AM

I've added the Queries log from MWDebug toolbar to the task description, and including a 6-frame call stack from each User::load call.

Looks like they're not coming from the ad-hoc ResourceLoaderContext objects in ResourceLoaderClientHtml. But I've fixed those regardless in the above patch.

Krinkle updated the task description. (Show Details)Apr 14 2020, 2:35 AM

Found it :)

Change 588520 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[mediawiki/core@master] resourceloader: Let derivative context inherit getUserObj() object

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

Change 588516 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Derive from existing Context object in ClientHtml

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

Change 588520 merged by jenkins-bot:
[mediawiki/core@master] resourceloader: Let derivative context inherit getUserObj() object

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

Krinkle closed this task as Resolved.Apr 18 2020, 11:05 PM