Page MenuHomePhabricator
Create Task
Maniphest T304813

Mapdata API should use the FlaggedRevs stable revision ParserCache when appropriate
Closed, ResolvedPublic8 Estimated Story Points
Actions

Description

FlaggedRevs intercepts stable revision pageviews and stores ParserOutput in the ParserCache under a different key than the latest revisions. APIs on the other hand will use RevisionOutputCache, which will cause duplicated parsing work and lower cache hit rates. We want to fix this for at least the mapdata API, since we know it will be called for every page with a map.

Quick fix

  • Add code to the mapdata API to switch to the FlaggedRevs cache for stable pages.

Proper fix

  • Add a new hook to ParserOutputAccess to allow extensions to provide a ParserOutput when appropriate.
  • Implement this hook in FlaggedRevs, to provide stable-pcache content if it exists and store there after parsing.

Details

SubjectRepoBranchLines +/-
[DNM] Remove stable cache workaround loggingmediawiki/extensions/Kartographermaster+0 -8
Duplicate "latest revision may be special" logic from FlaggedRevsmediawiki/extensions/Kartographerwmf/1.39.0-wmf.10+6 -2
Duplicate "latest revision may be special" logic from FlaggedRevsmediawiki/extensions/Kartographermaster+6 -2
Watch for mapdata cache misses in productionoperations/mediawiki-configmaster+1 -1
Update TODO with new task numbermediawiki/extensions/Kartographermaster+1 -1
Untangle dependencies between PoolWorkArticleView subclassesmediawiki/coremaster+39 -56
New hook to divert ParserOutputAccessmediawiki/coremaster+82 -5
Implement multi-cache hookmediawiki/extensions/FlaggedRevsmaster+168 -0
[beta] Stash all logs for the Kartographer extensionoperations/mediawiki-configmaster+1 -0
Workaround for FlaggedRevs stable-revision cachingmediawiki/extensions/Kartographermaster+118 -4
Change ParserOutputAccess workers to work with Status objectsmediawiki/coremaster+83 -120
[WIP] Experimenting with pooled parse workersmediawiki/coremaster+33 -210
Move "dirty" logic to PoolWorkArticleView subclass that uses itmediawiki/coremaster+21 -38
Rearrange PoolWorkArticleViewCurrent::fallback() for claritymediawiki/coremaster+11 -14
Kartographer tests depend on FlaggedRevsintegration/configmaster+3 -3
Test for PoolWorkArticleViewCurrent::fallbackmediawiki/coremaster+64 -1
Make old vs. latest revision more obvious in ParserOutputAccessmediawiki/coremaster+8 -16
Extract cache handling into a new classmediawiki/extensions/FlaggedRevsmaster+118 -16
Show related patches Customize query in gerrit

Related Objects
Search...

Event Timeline

awight created this task.Mar 28 2022, 8:34 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 28 2022, 8:34 AM
gerritbot added a comment.Mar 28 2022, 9:51 AM

Change 773446 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/FlaggedRevs@master] [WIP] Implement multi-cache hooks

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

gerritbot added a project: Patch-For-Review.Mar 28 2022, 9:51 AM

Change 774419 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/FlaggedRevs@master] Extract cache handling into a new class

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

gerritbot added a comment.Mar 28 2022, 9:54 AM

Change 773361 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/core@master] New hook to divert ParserOutputAccess

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

awight claimed this task.Mar 28 2022, 11:31 AM
awight moved this task from Sprint Backlog to Doing on the WMDE-TechWish-Sprint-2022-03-16 board.
awight added a parent task: T293905: Performance review of proposed Kartographer/Kartotherian revid support.Mar 29 2022, 8:45 AM
awight mentioned this in T293905: Performance review of proposed Kartographer/Kartotherian revid support.Mar 29 2022, 8:57 AM
awight added a project: WMDE-TechWish-Sprint-2022-03-30.Mar 30 2022, 8:09 AM
thiemowmde moved this task from Sprint Backlog to Doing on the WMDE-TechWish-Sprint-2022-03-30 board.Mar 30 2022, 8:10 AM
thiemowmde set the point value for this task to 8.Mar 30 2022, 8:42 AM
thiemowmde set Final Story Points to 13.
awight moved this task from Backlog to In sprint on the WMDE-GeoInfo-FocusArea board.Mar 30 2022, 3:02 PM
awight added a parent task: T300712: Deploy versioned maps support to production.Mar 31 2022, 8:12 AM
awight added subscribers: Krinkle, Ladsgroup.Mar 31 2022, 9:09 AM
awight moved this task from Doing to Tech Review on the WMDE-TechWish-Sprint-2022-03-30 board.Mar 31 2022, 1:20 PM
gerritbot added a comment.Apr 1 2022, 3:44 PM

Change 774419 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Extract cache handling into a new class

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.6; 2022-04-04).Apr 1 2022, 4:00 PM
Tobi_WMDE_SW subscribed.Apr 4 2022, 10:44 AM
gerritbot added a comment.Apr 4 2022, 11:05 AM

Change 776901 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Don't cache old revisions in ParserOutputAccess' local cache

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

gerritbot added a comment.Apr 4 2022, 11:05 AM

Change 776900 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Make old vs. latest revision more obvious in ParserOutputAccess

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

gerritbot added a comment.Apr 4 2022, 12:02 PM

Change 776937 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Untangle dependencies between PoolWorkArticleView subclasses

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

gerritbot added a comment.Apr 4 2022, 12:13 PM

Change 776900 merged by jenkins-bot:

[mediawiki/core@master] Make old vs. latest revision more obvious in ParserOutputAccess

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

gerritbot added a comment.Apr 5 2022, 2:30 PM

Change 777360 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/core@master] [WIP] Experimenting with pooled parse workers

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

gerritbot added a comment.Apr 5 2022, 3:49 PM

Change 777376 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Rearrange PoolWorkArticleViewCurrent::fallback() for clarity

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

gerritbot added a comment.Apr 5 2022, 4:02 PM

Change 777382 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Move "dirty" logic to PoolWorkArticleView subclass that uses it

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

gerritbot added a comment.Apr 5 2022, 4:42 PM

Change 777408 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/core@master] Change ParserOutputAccess workers to work with Status objects

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

gerritbot added a comment.Apr 6 2022, 8:40 AM

Change 777750 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/core@master] Test for PoolWorkArticleViewCurrent::fallback

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

gerritbot added a comment.Apr 7 2022, 11:45 AM

Change 777750 merged by jenkins-bot:

[mediawiki/core@master] Test for PoolWorkArticleViewCurrent::fallback

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

gerritbot added a comment.Apr 7 2022, 11:55 AM

Change 778279 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Workaround for FlaggedRevs stable-revision caching

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.7; 2022-04-11); removed MW-1.39-notes (1.39.0-wmf.6; 2022-04-04).Apr 7 2022, 12:00 PM
gerritbot added a comment.Apr 7 2022, 1:07 PM

Change 778287 had a related patch set uploaded (by Awight; author: Awight):

[integration/config@master] Kartographer tests depend on FlaggedRevs

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

awight removed awight as the assignee of this task.Apr 7 2022, 1:15 PM
awight updated Other Assignee, added: thiemowmde.
awight updated Other Assignee, removed: thiemowmde.
awight updated the task description. (Show Details)Apr 8 2022, 9:28 AM
gerritbot added a comment.Apr 8 2022, 10:56 AM

Change 778287 merged by jenkins-bot:

[integration/config@master] Kartographer tests depend on FlaggedRevs

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

gerritbot added a comment.Apr 8 2022, 11:10 AM

Change 777376 merged by jenkins-bot:

[mediawiki/core@master] Rearrange PoolWorkArticleViewCurrent::fallback() for clarity

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

gerritbot added a comment.Apr 8 2022, 12:06 PM

Change 777382 merged by jenkins-bot:

[mediawiki/core@master] Move "dirty" logic to PoolWorkArticleView subclass that uses it

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

gerritbot added a comment.Apr 8 2022, 1:10 PM

Change 777360 abandoned by Awight:

[mediawiki/core@master] [WIP] Experimenting with pooled parse workers

Reason:

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

gerritbot added a comment.Apr 8 2022, 2:10 PM

Change 777408 merged by jenkins-bot:

[mediawiki/core@master] Change ParserOutputAccess workers to work with Status objects

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

awight added a comment.Apr 11 2022, 11:19 AM

I was able to reproduce the CI error quite simply, by commenting out all of my local settings and including DevelopmentSettings.php

awight added a comment.Apr 11 2022, 11:22 AM

It was $wgKartographerVersionedMapdata = true;. Of course... without this setting the mapdata API won't try to fetch data from the older revision.

thiemowmde added a project: WMDE-TechWish-Sprint-2022-04-13.Apr 13 2022, 12:21 PM
thiemowmde moved this task from Sprint Backlog to Tech Review on the WMDE-TechWish-Sprint-2022-04-13 board.
awight renamed this task from Integrate FlaggedRevs ParserCache into the ParserOutputAccess service when fetching the stable revision to Mapdata API should use the FlaggedRevs stable revision ParserCache when appropriate.Apr 13 2022, 12:22 PM
gerritbot added a comment.Apr 19 2022, 8:51 AM

Change 778279 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Workaround for FlaggedRevs stable-revision caching

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.9; 2022-04-25); removed MW-1.39-notes (1.39.0-wmf.7; 2022-04-11).Apr 19 2022, 9:00 AM
thiemowmde moved this task from Tech Review to Demo on the WMDE-TechWish-Sprint-2022-04-13 board.Apr 19 2022, 10:40 AM
thiemowmde subscribed.

Remaining patches are all optional or even WIP.

WMDE-Fisch subscribed.Apr 22 2022, 7:51 AM
In T304813#7863457, @gerritbot wrote:

Change 778279 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Workaround for FlaggedRevs stable-revision caching

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

I ran several tests on the beta cluster[1] with this to see if we ever get a Stable cache miss in the logs. Currently I got nothing there. So I assume it works.

[1] https://de.wikipedia.beta.wmflabs.org/w/index.php?title=Maptests

gerritbot added a comment.Apr 25 2022, 1:28 PM

Change 785852 had a related patch set uploaded (by Awight; author: Awight):

[operations/mediawiki-config@master] [beta] Stash all logs for the Kartographer extension

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

gerritbot added a comment.Apr 26 2022, 9:30 AM

Change 785852 merged by jenkins-bot:

[operations/mediawiki-config@master] [beta] Stash all logs for the Kartographer extension

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

gerritbot added a comment.Apr 26 2022, 10:24 AM

Change 786289 had a related patch set uploaded (by Awight; author: Awight):

[operations/mediawiki-config@master] Watch for mapdata cache misses in production

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

gerritbot added a comment.Apr 26 2022, 11:56 AM

Change 773446 abandoned by Awight:

[mediawiki/extensions/FlaggedRevs@master] Implement multi-cache hook

Reason:

easing out of this rabbit hole...

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

gerritbot added a comment.Apr 26 2022, 11:57 AM

Change 773361 abandoned by Awight:

[mediawiki/core@master] New hook to divert ParserOutputAccess

Reason:

Dropping in favor of a one-off workaround for our use case.

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

awight added a comment.Apr 26 2022, 12:10 PM
In T304813#7873387, @WMDE-Fisch wrote:

I ran several tests on the beta cluster[1] with this to see if we ever get a Stable cache miss in the logs. Currently I got nothing there. So I assume it works.

Once logging was enabled, we were also able to demonstrate the rare case when the stable cache is missed, and verified that the code path is executed and result looks correct.

gerritbot added a comment.Apr 26 2022, 12:23 PM

Change 776937 merged by jenkins-bot:

[mediawiki/core@master] Untangle dependencies between PoolWorkArticleView subclasses

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.10; 2022-05-02); removed MW-1.39-notes (1.39.0-wmf.9; 2022-04-25).Apr 26 2022, 1:01 PM
Krinkle added a comment.Apr 26 2022, 3:26 PM
In T304813#7880500, @gerritbot wrote:

Change 773446 abandoned by Awight:

[mediawiki/extensions/FlaggedRevs@master] Implement multi-cache hook

Reason:

easing out of this rabbit hole...

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

For what it's worth, I think you made the right call to post-pone refactoring of this for now. It's definitely something we'll want to circle back to at some point, but it seems more risk to me than is useful and relevant stakeholders are busy with other refactors already that this is fine to call directly for the time being.

WMDE-Fisch closed this task as Resolved.Apr 27 2022, 9:23 AM
WMDE-Fisch moved this task from Demo to Done on the WMDE-TechWish-Sprint-2022-04-13 board.
TheDJ subscribed.Apr 30 2022, 7:44 PM

Just a note that the code contains:

* FIXME: Once T304813 is resolved, MediaWiki core will be able to dynamically select the
* correct cache.  Until then, we're explicitly using the FlaggedRevs stable-revision cache to

Reading the above i'm guessing this part was actually not done as part of this ticket, so that comment isn't fully correct any longer ?

WMDE-Fisch added a comment.May 2 2022, 7:56 AM
In T304813#7893575, @TheDJ wrote:

Just a note that the code contains:

* FIXME: Once T304813 is resolved, MediaWiki core will be able to dynamically select the
* correct cache.  Until then, we're explicitly using the FlaggedRevs stable-revision cache to

Reading the above i'm guessing this part was actually not done as part of this ticket, so that comment isn't fully correct any longer ?

Good catch. We'll create an extra ticket for that solution and can alter the comment then.

awight mentioned this in T307342: MediaWiki should use the FlaggedRevs "stable parser cache" when handling a stable revision.May 2 2022, 10:03 AM
gerritbot added a comment.May 2 2022, 10:07 AM

Change 788301 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Update TODO with new task number

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

gerritbot added a comment.May 2 2022, 10:35 AM

Change 788301 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Update TODO with new task number

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

Krinkle mentioned this in T304480: ParserCache dashboard should show statistics for FlaggedRevs cache.May 2 2022, 5:51 PM
gerritbot added a comment.May 10 2022, 7:45 AM

Change 786289 merged by jenkins-bot:

[operations/mediawiki-config@master] Watch for mapdata cache misses in production

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

Stashbot added a comment.May 10 2022, 7:48 AM

Mentioned in SAL (#wikimedia-operations) [2022-05-10T07:48:24Z] <awight@deploy1002> Synchronized wmf-config: Config: [[gerrit:786289|Watch for mapdata cache misses in production (T304813 T300712)]] (duration: 00m 50s)

Stashbot mentioned this in T300712: Deploy versioned maps support to production.May 10 2022, 7:48 AM
gerritbot added a comment.May 11 2022, 12:02 PM

Change 791018 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@master] Duplicate "latest revision may be special" logic from FlaggedRevs

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

gerritbot added a comment.May 11 2022, 12:56 PM

Change 791027 had a related patch set uploaded (by Awight; author: Awight):

[mediawiki/extensions/Kartographer@master] Remove stable cache workaround logging

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

gerritbot added a comment.May 12 2022, 8:36 AM

Change 791248 had a related patch set uploaded (by Awight; author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/Kartographer@wmf/1.39.0-wmf.10] Duplicate "latest revision may be special" logic from FlaggedRevs

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

gerritbot added a comment.May 12 2022, 8:38 AM

Change 791018 merged by jenkins-bot:

[mediawiki/extensions/Kartographer@master] Duplicate "latest revision may be special" logic from FlaggedRevs

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

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.12; 2022-05-16); removed MW-1.39-notes (1.39.0-wmf.10; 2022-05-02).May 12 2022, 12:00 PM
gerritbot added a comment.May 17 2022, 1:50 PM

Change 791248 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/Kartographer@wmf/1.39.0-wmf.10] Duplicate "latest revision may be special" logic from FlaggedRevs

Reason:

wmf.12 will come this week anyway.

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

gerritbot added a comment.May 8 2024, 11:12 AM

Change #791027 abandoned by Thiemo Kreuz (WMDE):

[mediawiki/extensions/Kartographer@master] [DNM] Remove stable cache workaround logging

Reason:

Already happened via Ia6defcc.

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

Tacsipacsi removed a project: Patch-For-Review.May 9 2024, 11:43 PM
Tacsipacsi subscribed.
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and 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 · Credits