Page MenuHomePhabricator
Create Task
Maniphest T241050

Investigate if FormatterCache is actually used everywhere it should be when rendering an entity
Closed, ResolvedPublic
Actions

Description

In T237984#5747158 I stated that

This is worth investigating afterwards as per T201838 the properties should also be using the caching layer introduced during one of the previous wb_terms trailblazes.
My logging result was P9897 when using adhoc logging P9898...

While diving in I might have missed something or miss read something but we should definitely confirm that the Formatter cache is used where it is meant to be used.

We should confirm if the FormatterCache is used in the chain when formatting the properties for the snaks of statements when rendering items

Might also be vaugly related to T205194.

Details

ProjectBranchLines +/-Subject
mediawiki/extensions/Wikibasemaster+31 -5Cache item terms lookup the similar way they are cached in properties
mediawiki/extensions/Wikibasemaster+33 -13wbterms: Use caching for prefetching item lookup in PerRepositoryServiceWiring
mediawiki/extensions/WikibaseLexememaster+2 -0Allow terms caching in the diff visualizer integration test
mediawiki/extensions/Wikibasemaster+128 -14Pass term type and language to TermIdsResolver in PrefetchingTermLookup
mediawiki/extensions/Wikibasemaster+34 -8Narrow prefetching of the entities on two special pages
Customize query in gerrit

Related Objects

Event Timeline

• Addshore created this task.Dec 18 2019, 12:23 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 18 2019, 12:23 PM
• Addshore added a comment.Dec 18 2019, 12:26 PM

It could be that the left hand side of the rendered statements (where the property ID is), was never intended to use and thus doesnt use the FormatterCache..

The intent of the old trailblaze would have been to do that in T198868: Do not get entity data from wb_terms when "prefetching" data for displaying on pages which is now done in T236681: SqlEntityInfoBuilder reads and writes from the old term store regardless of the config

• Addshore added a project: Wikidata-Campsite.Dec 18 2019, 12:27 PM
• Addshore moved this task from Incoming to Ready to pick up on the Wikidata-Campsite board.Jan 7 2020, 10:30 AM
• Addshore moved this task from Ready to pick up to Wikidata-Campsite-Iteration-∞ (On Hold) on the Wikidata-Campsite board.Jan 7 2020, 1:30 PM
• Addshore edited projects, added Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)); removed Wikidata-Campsite.
Maintenance_bot moved this task from incoming to in progress on the Wikidata board.Jan 7 2020, 2:16 PM
• Addshore triaged this task as Medium priority.Jan 14 2020, 4:09 PM
• Addshore mentioned this in T242871: wbterms: Add a cache for PrefetchingPropertyTermLookup::prefetchTerms().Jan 15 2020, 4:11 PM
Ladsgroup added a subscriber: Lucas_Werkmeister_WMDE.EditedJan 15 2020, 5:53 PM

I looked at it a while back with @Lucas_Werkmeister_WMDE and the conclusion was that the new term store has BufferingTermLookup inside it natively (without the size limit BufferingTermLookup enforces) but I might be wrong here.

• Addshore closed this task as Resolved.Jan 16 2020, 10:27 AM
• Addshore claimed this task.
Restricted Application added a project: User-Addshore. · View Herald TranscriptJan 16 2020, 10:27 AM
• Addshore reopened this task as Open.Jan 16 2020, 10:27 AM

Didn't mean to do that

• Addshore removed • Addshore as the assignee of this task.Jan 16 2020, 10:27 AM
gerritbot added a comment.Jan 16 2020, 11:39 AM

Change 565258 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Pass term type and language to TermIdsResolver in PrefetchingTermLookup

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

gerritbot added a project: Patch-For-Review.Jan 16 2020, 11:39 AM
Ladsgroup claimed this task.Jan 16 2020, 11:43 AM
Ladsgroup moved this task from To Do (prioritised from top to bottom) to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
Restricted Application added a project: User-Ladsgroup. · View Herald TranscriptJan 16 2020, 11:43 AM
Maintenance_bot moved this task from Incoming to In progress on the User-Ladsgroup board.Jan 16 2020, 12:15 PM
WMDE-leszek moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 16 2020, 1:05 PM
Ladsgroup added a subscriber: WMDE-leszek.Jan 16 2020, 4:29 PM

The FormatterCache is being used for items but not properties (the left hand side of statements to be exact. If you render an entity with statement value of a property, it still uses the formatter cache). Is that intentional @WMDE-leszek?

• Addshore added a comment.Jan 16 2020, 4:35 PM
In T241050#5809929, @Ladsgroup wrote:

The FormatterCache is being used for items but not properties (the left hand side of statements to be exact. If you render an entity with statement value of a property, it still uses the formatter cache). Is that intentional @WMDE-leszek?

If that is so then I guess it will not be used for the left hand side of snaks in general (references, qualifiers included in that)

Ladsgroup moved this task from Doing to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 16 2020, 4:40 PM
• Addshore moved this task from Unsorted 💣 to Watching 👀 on the User-Addshore board.Jan 16 2020, 11:40 PM
• Addshore added a comment.EditedJan 20 2020, 2:09 PM

Will move to doing for T241050#5809929 to quickly be done / looked at / commented on by the @WMDE-leszek

• Addshore reassigned this task from Ladsgroup to WMDE-leszek.Jan 20 2020, 2:09 PM
• Addshore moved this task from Peer Review to Doing on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.
WMDE-leszek removed WMDE-leszek as the assignee of this task.Jan 20 2020, 3:27 PM
In T241050#5809929, @Ladsgroup wrote:

The FormatterCache is being used for items but not properties (the left hand side of statements to be exact. If you render an entity with statement value of a property, it still uses the formatter cache). Is that intentional @WMDE-leszek?

this has not been meant to be like this ultimately. The current state is just the intermediate state this code ended up being in Summer 2018.

Ladsgroup moved this task from Doing to Test (Verification) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 20 2020, 4:10 PM
• Addshore moved this task from Test (Verification) to Peer Review on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 21 2020, 10:21 AM
gerritbot added a comment.Jan 23 2020, 1:21 PM

Change 566730 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Narrow prefetching of the entities on two special pages

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

gerritbot added a comment.Jan 23 2020, 4:01 PM

Change 566730 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Narrow prefetching of the entities on two special pages

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

Ladsgroup added a comment.Jan 29 2020, 1:44 PM

Using XHGui to anlayze rendering of Q7251, it showed that 60% of time for rendering the page is being spent on PrefetchingPropertyTermsLookup, this is crazy:

image.png (863×1 px, 812 KB)

(From: https://performance.wikimedia.org/xhgui/run/callgraph?id=5e30a7403f3dfa0d2bf6b337 )

It also calls the the function 285 times while it should do it only once.

gerritbot added a comment.Jan 29 2020, 1:45 PM

Change 565258 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Pass term type and language to TermIdsResolver in PrefetchingTermLookup

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

Maintenance_bot removed a project: Patch-For-Review.Jan 29 2020, 2:10 PM
Ladsgroup added a comment.Jan 29 2020, 2:11 PM
In T241050#5838248, @gerritbot wrote:

Change 565258 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Pass term type and language to TermIdsResolver in PrefetchingTermLookup

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

I think this should be backported before turning on more reads.

• Addshore added a comment.Jan 29 2020, 2:47 PM
In T241050#5838312, @Ladsgroup wrote:
In T241050#5838248, @gerritbot wrote:

Change 565258 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Pass term type and language to TermIdsResolver in PrefetchingTermLookup

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

I think this should be backported before turning on more reads.

I think lets just not turn on more reads until it is deployed in the train :)

• Addshore moved this task from Peer Review to Test (Verification) on the Wikidata-Campsite (Wikidata-Campsite-Iteration-∞ (On Hold)) board.Jan 29 2020, 2:47 PM
WMDE-leszek added a comment.Jan 30 2020, 3:25 PM

Anything left do here? Or is the idea to wait with closing this until the change is deployed and live?

• Addshore closed this task as Resolved.Feb 6 2020, 9:50 AM
• Addshore claimed this task.
Maintenance_bot moved this task from In progress to Done on the User-Ladsgroup board.Feb 6 2020, 10:15 AM
Ladsgroup added a comment.Feb 10 2020, 6:49 PM

After deploying this fix:

image.png (890×1 px, 118 KB)

You can see similar drops in open connections, traffic, etc.

Ladsgroup added a comment.Feb 10 2020, 6:51 PM
This comment was removed by Ladsgroup.
gerritbot added a comment.Feb 19 2020, 11:31 AM

Change 573260 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Cache item terms lookup the similar way they are cached in properties

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

gerritbot added a project: Patch-For-Review.Feb 19 2020, 11:31 AM
gerritbot added a comment.Feb 19 2020, 1:32 PM

Change 573285 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/WikibaseLexeme@master] Make caching better in the diff visualizer integration test

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

gerritbot added a comment.Feb 19 2020, 3:45 PM

Change 573311 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] wbterms: Use caching for prefetching item lookup in PerRepositoryServiceWiring

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

gerritbot added a comment.Feb 19 2020, 4:08 PM

Change 573285 merged by jenkins-bot:
[mediawiki/extensions/WikibaseLexeme@master] Allow terms caching in the diff visualizer integration test

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

gerritbot added a comment.Feb 19 2020, 4:08 PM

Change 573260 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Cache item terms lookup the similar way they are cached in properties

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

gerritbot added a comment.Feb 19 2020, 4:46 PM

Change 573311 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] wbterms: Use caching for prefetching item lookup in PerRepositoryServiceWiring

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

ReleaseTaggerBot added a project: MW-1.35-notes (1.35.0-wmf.21; 2020-02-25).Feb 19 2020, 5:00 PM
Maintenance_bot removed a project: Patch-For-Review.Feb 19 2020, 5:11 PM
Content licensed under Creative Commons Attribution-ShareAlike 3.0 (CC-BY-SA) unless otherwise noted; code licensed under GNU General Public License (GPL) or other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL