DiscussionTools is making duplicate DB requests back to back
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Ladsgroup
	Dec 6 2021, 5:27 PM

Description

I'm checking queries made by a purge and I saw this:

It needs to have some cache.
Here is the call graph and call count for that code in DiscussionTools: https://performance.wikimedia.org/xhgui/run/symbol?id=61ae46afebb32a41e58d4f13&symbol=MediaWiki%5CExtension%5CDiscussionTools%5CHooks%5CHookUtils%3A%3AisAvailableForTitle

Fixing this would remove something on 1k query/sec at least. Let me know if you have reproducing the problem

Details

	Subject	Repo	Branch	Lines +/-
	Cache page properties in memory to avoid extra queries	mediawiki/extensions/DiscussionTools	wmf/1.38.0-wmf.12	+25 -7
	Cache page properties in memory to avoid extra queries	mediawiki/extensions/DiscussionTools	master	+25 -7

Customize query in gerrit

Related Objects

Mentioned In: T364693: DiscussionTools isFeatureEnabled check is taking 5% of all requests
T297669: Noticeable increase in db load after wmf.12 roll out
T297300: PageProps doesn't cache result when it is false
Mentioned Here: T297669: Noticeable increase in db load after wmf.12 roll out
T297300: PageProps doesn't cache result when it is false

Event Timeline

Ladsgroup created this task.Dec 6 2021, 5:27 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 6 2021, 5:27 PM

Change 744889 had a related patch set uploaded (by Esanders; author: Esanders):

[mediawiki/extensions/DiscussionTools@master] Cache page properties in memory to avoid extra queries

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

gerritbot added a project: Patch-For-Review.Dec 7 2021, 10:32 PM

Esanders added projects: Performance Issue, Editing-team (Kanban Board).Dec 7 2021, 10:33 PM

Esanders moved this task from Incoming to Code Review on the Editing-team (Kanban Board) board.

On my local test page this fix results in DT making 8 queries instead of 22. Thanks.

Change 744889 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Cache page properties in memory to avoid extra queries

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

I think this is actually a bug in the core PageProps class, which we are using. In PageProps::getProperties(), there's a ton of code that does caching, but only when the property is set – nothing is cached if the property we're querying for isn't present on the page. This seems unexpected to me.

matmarex moved this task from Code Review to Ready for Sign Off on the Editing-team (Kanban Board) board.Dec 8 2021, 12:31 AM

matmarex edited projects, added Skipped QA; removed Patch-For-Review.

ReleaseTaggerBot added a project: MW-1.38-notes (1.38.0-wmf.13; 2021-12-13).Dec 8 2021, 1:00 AM

In T297132#7555212, @matmarex wrote:

I think this is actually a bug in the core PageProps class, which we are using. In PageProps::getProperties(), there's a ton of code that does caching, but only when the property is set – nothing is cached if the property we're querying for isn't present on the page. This seems unexpected to me.

Thanks for fixing it quickly. if you want to make a patch against that in core, I'd be more than happy to review it. I think the general attitude of core's caching is that if the value is falsy, it doesn't cache them. e.g. getWithSetCallback doesn't cache if the callback returns false. This was not my idea and I don't even like these implicit logic bits but it is the current situation.

ppelberg closed this task as Resolved.Dec 8 2021, 5:05 PM

ppelberg claimed this task.

Filed as T297300

Change 746920 had a related patch set uploaded (by Ladsgroup; author: Esanders):

[mediawiki/extensions/DiscussionTools@wmf/1.38.0-wmf.12] Cache page properties in memory to avoid extra queries

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

gerritbot added a project: Patch-For-Review.Dec 14 2021, 4:23 AM

Change 746920 merged by jenkins-bot: