Page MenuHomePhabricator

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

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.

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptDec 18 2019, 12:23 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 triaged this task as Medium priority.Jan 14 2020, 4:09 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 claimed this task.

Didn't mean to do that

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

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?

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)

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

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.

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

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

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:

(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.

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

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.

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 :)

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

Addshore claimed this task.

After deploying this fix:


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

This comment was removed by Ladsgroup.

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

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

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

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

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

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