NOTE: All this is only needed because the database backend of ParserCache does not do eviction or active expiry. If it did, we didn't need an extra cache, we could just reduce the TTL of the fast entries, and the storage backend would expire the obsolete entries.

That can be remedied in different ways for example, many parts of mediawiki toss a coin after doing some writes and clean up unrelated old rows in x% of the time. I'd argue we should actually move out of maint script to this model for ParserCache in general anyway.

Can we disable the optimization implemented as part of T346765. All things considered, I prefer keeping the infrastructure and architecture simple -- I am not yet convinced we need to layer more complexity on top. Do we have a sense of how much ParserCache capacity T346765 is saving?

In T391477#10727586, @ssastry wrote:

Can we disable the optimization implemented as part of T346765. All things considered, I prefer keeping the infrastructure and architecture simple -- I am not yet convinced we need to layer more complexity on top. Do we have a sense of how much ParserCache capacity T346765 is saving?

I agree with subbu, simpler infrastructure is much preferred. If T346765 is too much. Maybe either the turn the knob and save more or just get rid of it altogether.

In T391477#10728176, @Ladsgroup wrote:

I agree with subbu, simpler infrastructure is much preferred. If T346765 is too much. Maybe either the turn the knob and save more or just get rid of it altogether.

Currently, we skip cache writes for file descriptions on commons, and for item pages on Wikidata. Based on your comment on Slack my understanding was that enabling parser cache for thouse would cause the write rate on the DB to be too high, causing issues with the binlog.

To make poolcounter work correctly for these pages, we need to cache them - either short term or persistently in the database. I agree that the simplest solution would be to just cache everything in the same way... if you think that would be ok we can flip the switch. That would be the simoplest solution for the problem.

The number of filtered writes averages about 3K per minute, with spikes going up to 20K: https://grafana.wikimedia.org/goto/F6fpnx0Hg?orgId=1. That's the writes we'd be adding. Looks to be about 10% of total writes. And we would be giving up on the idea of making this tunable.

IIRC, we implemented this because the parser cache backend was having trouble keeping up?

In T391477#10728868, @daniel wrote:

In T391477#10728176, @Ladsgroup wrote:

I agree with subbu, simpler infrastructure is much preferred. If T346765 is too much. Maybe either the turn the knob and save more or just get rid of it altogether.

Currently, we skip cache writes for file descriptions on commons, and for item pages on Wikidata. Based on your comment on Slack my understanding was that enabling parser cache for thouse would cause the write rate on the DB to be too high, causing issues with the binlog.

I also said "it can be fine, I just need some numbers :D" :P

To make poolcounter work correctly for these pages, we need to cache them - either short term or persistently in the database.

I don't understand this statement. PoolCounter is a concurrency management service. ParserCache is a cache for the values. They are conceptually quite different systems and PoolCounter shouldn't need ParserCache at all (unless it would want to return stale data which I assume is not we are doing here). Can we make change not to require any cache there?

I agree that the simplest solution would be to just cache everything in the same way... if you think that would be ok we can flip the switch. That would be the simoplest solution for the problem.

The number of filtered writes averages about 3K per minute, with spikes going up to 20K: https://grafana.wikimedia.org/goto/F6fpnx0Hg?orgId=1. That's the writes we'd be adding. Looks to be about 10% of total writes. And we would be giving up on the idea of making this tunable.

The 10% is not much, we definitely can take it, the spike is a bit scary but that should be fine but I prefer not storing if we don't need to but if that's the only option: 1- We have expanded the clusters from 3 to 7 2- we are going to add one more 3- we can expand more and cache more aggressively if needed. PC now is much better horizontally scalable than four months ago.

IIRC, we implemented this because the parser cache backend was having trouble keeping up?

That's not really what happened. As I said in the ritual and other places: The main problem was that there was 1000 concurrent writes happening on one single row causing lock contention and writes piling up which made the database unresponsive. A lot of writes are okay as long as they are not on the same row over and over again which is the point of PoolCounter.

In T391477#10731649, @Ladsgroup wrote:

In T391477#10728868, @daniel wrote:

To make poolcounter work correctly for these pages, we need to cache them - either short term or persistently in the database.

I don't understand this statement. PoolCounter is a concurrency management service. ParserCache is a cache for the values. They are conceptually quite different systems and PoolCounter shouldn't need ParserCache at all (unless it would want to return stale data which I assume is not we are doing here). Can we make change not to require any cache there?

No, we need some cache. Poolcounter itself doesn't need a cache, it's pretty useless in the context of rendering if we don't have a cache: when the pool is exhausted, the process will wait for another process to finish parsing. The point is that we can then use the parser output generated by that other process. But we can only do that if we can get it somewhere - from a cache. If the output is not found in the cache, we have to parse anyway. We still reduce concurrency, but we don't really reduce load because all the parses still need to happen. And since all the clients are waiting, requests may start to pile up.

The 10% is not much, we definitely can take it, the spike is a bit scary but that should be fine but I prefer not storing if we don't need to but if that's the only option: 1- We have expanded the clusters from 3 to 7 2- we are going to add one more 3- we can expand more and cache more aggressively if needed. PC now is much better horizontally scalable than four months ago.

Ok. So shall we commit to putting everything into parsercache always, and remove the option to skip things that are unlikely to be read again, or fast to generate on the fly?

IIRC, we implemented this because the parser cache backend was having trouble keeping up?

That's not really what happened. As I said in the ritual and other places: The main problem was that there was 1000 concurrent writes happening on one single row causing lock contention and writes piling up which made the database unresponsive. A lot of writes are okay as long as they are not on the same row over and over again which is the point of PoolCounter.

I understand that this is what recently caused T387032. What I mean is the rationale for T346765 two years ago - IIRC that was supposed for a tunable replacement for a hack we had in place for disabling parser cache for commons and wikidata. Is that not correct?

I just realized that this came up before: T350669: Use short term cache when skipping ParserCache due to fast parse time

In T391477#10732370, @daniel wrote:

In T391477#10731649, @Ladsgroup wrote:

In T391477#10728868, @daniel wrote:

To make poolcounter work correctly for these pages, we need to cache them - either short term or persistently in the database.

I don't understand this statement. PoolCounter is a concurrency management service. ParserCache is a cache for the values. They are conceptually quite different systems and PoolCounter shouldn't need ParserCache at all (unless it would want to return stale data which I assume is not we are doing here). Can we make change not to require any cache there?

No, we need some cache. Poolcounter itself doesn't need a cache, it's pretty useless in the context of rendering if we don't have a cache: when the pool is exhausted, the process will wait for another process to finish parsing. The point is that we can then use the parser output generated by that other process. But we can only do that if we can get it somewhere - from a cache. If the output is not found in the cache, we have to parse anyway. We still reduce concurrency, but we don't really reduce load because all the parses still need to happen. And since all the clients are waiting, requests may start to pile up.

From database point of view pool counter without parser cache is still enough. Since lock contention is what brings down databases most of the time. In that specific incident this ticket is a subticket of, it was exactly lock contention over one row over and over again. That made writes pile up. The binlog, writes, or reads were not really the problem. As long as as they are not competing, it's fine.

The 10% is not much, we definitely can take it, the spike is a bit scary but that should be fine but I prefer not storing if we don't need to but if that's the only option: 1- We have expanded the clusters from 3 to 7 2- we are going to add one more 3- we can expand more and cache more aggressively if needed. PC now is much better horizontally scalable than four months ago.

Ok. So shall we commit to putting everything into parsercache always, and remove the option to skip things that are unlikely to be read again, or fast to generate on the fly?

IIRC, we implemented this because the parser cache backend was having trouble keeping up?

That's not really what happened. As I said in the ritual and other places: The main problem was that there was 1000 concurrent writes happening on one single row causing lock contention and writes piling up which made the database unresponsive. A lot of writes are okay as long as they are not on the same row over and over again which is the point of PoolCounter.

I understand that this is what recently caused T387032. What I mean is the rationale for T346765 two years ago - IIRC that was supposed for a tunable replacement for a hack we had in place for disabling parser cache for commons and wikidata. Is that not correct?

I thought you were talking about T387032 since this ticket is the subticket of that incident. As you said, T346765 happened two years ago and we have made massive changes since then, the cluster is more than doubled, we fixed mobile parser cache fragmentation and so on. If it's 10% on average, I think we will be fine.

Given that Parsoid will be slightly more expensive in terms of CPU and other criteria, I honestly vote for full storage of entries altogether (maaybe exclude commons and wikidata given their size). Worst case, we'll buy more pc hosts.

Change #1136151 had a related patch set uploaded (by Daniel Kinzler; author: Daniel Kinzler):

[mediawiki/core@master] [PoC] ParserOutputAccess: move filter logic from ParserCache

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

The proposed solution means that we always have to check both caches when reading, because we don't know the outcome the filter on save, so we don't know which cache it was written to.

If the filter wasn't conditional on the outcome (timing) of the parse but unconditional based on namespace (which it effectively currently is in production), we could just let the filter pick the cache.

The current conversation around applying a PoolCounter limiter to ParserOutput generation in the REST API afaik does not explicitly depend on ParserCache.

This conversation is afaik focussed on resource consumption by individual clients/services requesting a large of different objects. This is different from the PoolWorkArticleViewCurrent use case which focusses on a large range of clients requesting the same object.

Of course if/when a client is allowed to proceed, if the action involves wikitext parsing it make sense to consult ParserCache and use if where possible, but it should also be fine for certain pages to require wikitext parsing always. Its benefit from PoolCounter-based rate limits that deny clients above a certain level of concurrentcy does not depend on it getting a cache hit. That aspect is specific to scenarios where the interest is to coalesce a wide audience in its demand for a single object, and is (almost) always combined with a tolerance of stale objects since this use case has a desire to serve the entire audience whenever possible, since any given client has done nothing wrong.

We can look at these PoolCounter applications, which don't call or hard-require ParserCache:

• Special:Contributions in mediawiki-core via ContributionsSpecialPage.
• Special:Search in CirrusSearch via its CirrusSearch-Search keys via Util::doPoolCounterWork( …,, $this->user
• ApiParse
• other use cases with a key set in wmf-config/PoolCounterSettings.php.

Having said that, I am broadly supportive of the idea to simplify ParserCache logic, and explore a ParserCache that has entries for all pages. This would enable, among other things, a more consistently low latency for pageviews (T302623), and would remove one (but not the only) major obstacle toward reducing our 60s HTTP timeouts.

This conversation is afaik focussed on resource consumption by individual clients/services requesting a large of different objects. This is different from the PoolWorkArticleViewCurrent use case which focusses on a large range of clients requesting the same object.

The current work on the REST API is also focussde on multiple clients requesting the same content - typically, this is caused by multiple clients being notified about a change to the page, and trying to catch up on the new content.

We should probably also implement per-client concurrency limits, but that should likely be done across APIs, and possibly across services. I expect that this will be a central part of our work on the 5.1 KR in the upcoming FY.

it should also be fine for certain pages to require wikitext parsing always.

I agree that this isn't a problem, at least currently. But giving clients the option to opt into "no wait" semantics seems attractive: the client would specify that it wants to get a 504 if the result isn't cached (it could do that using cache-control: only-if-cached) and then retry later. The way, both client and server wouldn't have to hold on to a TCP connection and block a worker thread while waiting for the parse. But that only works if we can parse in the background and hold the result in a cache temporarily, and be it for that client only.

Its benefit from PoolCounter-based rate limits that deny clients above a certain level of concurrentcy does not depend on it getting a cache hit.

Not directly, but if the result is not cached and so blocked clients won't benefit from it, we could easily run into a pathological situation when several clients are requesting the resource at once: on a page that takes long to parse, it is more likely that additional clients request it while parsing is in progress. They get unblocked when the parsing is done, but fail to find anything in the cache, so they start to parse for themselves. The initial wait was useless for them (but spread the load for the server). However, since the client already spent time waiting, we have an increased risk of the request timing out, which would cause the client to re-try.

I suppose that doesn't happen often enough to actually be a problem: what I described is the current behavior, and to my knowledge nobody has complained so far. It's conceivable that a malicious client may try to trigger this case intentionally, but since the load gets spread out, that's probably not a big deal either.

I am broadly supportive of the idea to simplify ParserCache logic, and explore a ParserCache that has entries for all pages.

Yea, but I think that'll have to wait until we rewrite the storage backend for the parser cache. Also, it wouldn't solve the issue for intrinsically uncacheable pages.

[…] when several clients are requesting the resource at once: […] we have an increased risk of the request timing out, which would cause the client to re-try.

Is this a scenario in which request coalescing no longer works at the edge? Or an attack scenario where the CDN is bypassed?

Remember that for article page views (i.e. Michael Jackson effect) we have to deal with the fact that the URL is regularly purged, which may discrupt coalescing, and there is significant logged-in traffic which bypasses the edge. Neither applies to the REST API endpoint.

Serving an HTTP 500 with Retry-Later is imho fine in extreme cases for APIs, since automated scripts can honour these with minimal impact. They have to anyway, since there are plenty of other reasons for which we may return HTTP 500 at a higher level (e.g. appservers unavailable, k8s pod shutting down, or any number of other small and short-lived infrastructure issues).

For page views, we want to avoid these at all costs, and thus default to stale content.

Yea, but I think that'll have to wait until we rewrite the storage backend for the parser cache. Also, it wouldn't solve the issue for intrinsically uncacheable pages.

What is an intrinsically uncachable page? There should be no calls to updateCacheExpiry( 0 ); other than e.g. during previews or personal/non-canonical parser options. If there are a wikitext features that can trigger this form the main namespace, when wgMiserMode is on, that's probably a bug. I think outside that, there are a handful of edge cases such as dynamic <categorytree> on certain project pages and certain user pages. These will be slower to render, and that's something we've basically accepted so long as its rare. Even there I'd expect caching to not be 0, though but e.g. 1 hour. For example, parsers/CoreMagicVariables has a minimum ParserCache of 1 hour, even for content with dynamic {{currenttime}} or {{numberofedits}} which change every second.

And even if ParserCache is disabled, these pages aren't uncached. They still have Varnish cache enabled all the same. Afaik there is no way for a Parser extension to disable HTTP caching on an article, which will be 24 hours either way. OutputPage::lowerCdnMaxage/adaptCdnTTL are called for things like replication lag and some non-canonical/security scenarios, but not informed by ParserCache expiry. Idem for the REST API where the 60s cache is unconditional.

Change #1136151 abandoned by Daniel Kinzler:

[mediawiki/core@master] [PoC] ParserOutputAccess: move filter logic from ParserCache

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