Caching rev id of properties for an hour could save a lot of db lookups and make us more resilient towards scrapers.

And show stale labels for up to an hour, I assume?

To me this looks like something we should address with prefetching/batching rather than caching. (In fact I would be surprised if we don’t already have some form of prefetching here… my guess is that we it might have broken accidentally at some point and we didn’t notice?)

Edit: we have PrefetchingWikiPageEntityMetaDataAccessor (most relevant-looking class I’ve found so far), but AFAICT we currently mainly use it to prefetch multiple top-level entities that will be needed by wbgetentities or the dump generator, not to prefetch all the entities linked from a single entity we’re about to render.

In T428512#11998180, @Lucas_Werkmeister_WMDE wrote:

Caching rev id of properties for an hour could save a lot of db lookups and make us more resilient towards scrapers.

And show stale labels for up to an hour, I assume?

Yeah, I don't see this being a problem. The CDN cache is for a day, so we serve stale data for a day to logged out users regardless. Plus once the label of property changes, it takes a while to be reflected on all items since HTML of the page is stored in ParserCache for a month, we will be serving stale data up to a month anyway so one hour more is not making a difference.

In T428512#11999104, @Ladsgroup wrote:

Plus once the label of property changes, it takes a while to be reflected on all items since HTML of the page is stored in ParserCache for a month

I thought we purged the cache when the property changes, but (based on some local testing) apparently not… good to know.

Still, people expect that purging the page will get them the latest state, and this would break that.

Also, surely a batched database query would be better for performance anyway, compared to (judging by this XHGui for a purge on the same small item) 13 separate queries?

In T428512#12013751, @Lucas_Werkmeister_WMDE wrote:

In T428512#11999104, @Ladsgroup wrote:

Plus once the label of property changes, it takes a while to be reflected on all items since HTML of the page is stored in ParserCache for a month

I thought we purged the cache when the property changes, but (based on some local testing) apparently not… good to know.

They are registered as links so only a reparse gets triggered if the page gets deleted/created.

Still, people expect that purging the page will get them the latest state, and this would break that.

It's not a hard problem to solve, we can just invalidate the cache for the property on page edit of that property. That way, we can even increase the TTL to a day even.

To be clear, I'm suggesting this for properties only.

Also, surely a batched database query would be better for performance anyway, compared to (judging by this XHGui for a purge on the same small item) 13 separate queries?

Definitely and that should be fixed regardless but the data will need to be looked up by the db too, so not looking it up from a database is much preferred than looking it up in a batch. It moves the load from db to memecached which is much cheaper and much easier to horizontally scale. Performance impact of batching (on many things, not just db) is not as big as many people think it is.

In that profile, it takes only 2% of the load time but I can imagine in larger items, it'll start to add up.

Let’s move beyond imagining ;) I took an Excimer profile of a purge on Q64 (Berlin), a reasonably large item. In this profile we can see, if I’m not mistaken, that all calls to TypeDispatchingEntityRevisionLookup::getLatestRevisionId() (including methods called by it) took up 613.00 ms, or 18% of the full request execution time. So yes, this is adding up.

What Excimer doesn’t tell us (AFAICT) is how many of those calls are for properties, rather than items. We’re assuming here that caching for properties will be worth it (because they’re more likely to be reused in statements across many entities?), but I don’t know if we can check this assumption with the profiling tools we have.

The other thought I had today is: what’s the point of caching the revision ID? We’re only using the revision ID as part of the cache key (in CachingFallbackLabelDescriptionLookup / TermFallbackCacheFacade) to make sure that the cached label is up to date. So instead of caching the revision ID, it seems like it would make more sense to remove the revision ID from the cache key for property IDs, and rely on active purging of the cache when property labels are edited in this case. (So I guess TermFallbackCacheFacade would have two slightly different interfaces: one without the revision ID, for properties, and one with the revision ID, for all other entities.)

In T428512#12016848, @Ladsgroup wrote:

It's not a hard problem to solve, we can just invalidate the cache for the property on page edit of that property. That way, we can even increase the TTL to a day even.

Good point.

Although this gets trickier if we want to remove the revision ID from the label cache key (as described above), rather than have a separate cache for the revision ID; because that means we need to invalidate (or update) lots of cache entries on an edit (one per language and term type). Or we change the cache key even more and store all the terms for a property in a single cache entry; but that means we’re loading way more data than needed (all languages rather than just one) when reading from the cache.

Outcome from a discussion with @Sadiya.Mohammed_WMDE and @HasanAkgun_WMDE: We think the approach of removing the revision ID from the cache key for property terms (labels/descriptions/aliases) sounds promising, and we want to investigate how to move ahead with this. The especially interesting question will be where the cache invalidation happens, and how we can do it – ideally, we would have a diff of the property edit available at that point, in which case we can write exactly the changed terms to the cache, and don’t have to invalidate all entries for the property.

The especially interesting question will be where the cache invalidation happens, and how we can do it – ideally, we would have a diff of the property edit available at that point

Quick pointers for further investigation:

• PropertyHandler::getSecondaryDataUpdates() returns several secondary data updates, including an update for the wb_property_info table and an update for the term store. At a glance, this sounds like the most logical place for invalidating or updating the cache. (Likewise getDeletionUpdates() in the same class.)
• RepoHooks::onRevisionFromEditComplete() notifies two subsystems about entity edits: the EntityStoreWatcher, and the ChangeNotifier. Either of those might also be a good place to invalidate or update the cache.

In T428512#12109274, @Lucas_Werkmeister_WMDE wrote:

In that profile, it takes only 2% of the load time but I can imagine in larger items, it'll start to add up.

Let’s move beyond imagining ;) I took an Excimer profile of a purge on Q64 (Berlin), a reasonably large item. In this profile we can see, if I’m not mistaken, that all calls to TypeDispatchingEntityRevisionLookup::getLatestRevisionId() (including methods called by it) took up 613.00 ms, or 18% of the full request execution time. So yes, this is adding up.

What Excimer doesn’t tell us (AFAICT) is how many of those calls are for properties, rather than items. We’re assuming here that caching for properties will be worth it (because they’re more likely to be reused in statements across many entities?), but I don’t know if we can check this assumption with the profiling tools we have.

We can check by measuring the ratio in pagelinks table (where pl_from_namespace = 0, split them based on lt_namespace). I can run the query but given that we have triplets, my assumption is that at least 50% of cases is properties. So it gives you the biggest bang for the buck (20K properties vs 150M items). But I'm not super opposed to idea of items either, I was just a bit worried about the invalidation but as you said, we can surgically invalidate terms (and e.g. not do anything if the edit is changing statements and not labels which would cut down on a lot of purges).

---

The other thought I had today is: what’s the point of caching the revision ID? We’re only using the revision ID as part of the cache key (in CachingFallbackLabelDescriptionLookup / TermFallbackCacheFacade) to make sure that the cached label is up to date. So instead of caching the revision ID, it seems like it would make more sense to remove the revision ID from the cache key for property IDs, and rely on active purging of the cache when property labels are edited in this case. (So I guess TermFallbackCacheFacade would have two slightly different interfaces: one without the revision ID, for properties, and one with the revision ID, for all other entities.)

Yeah makes sense actually

---

In T428512#12016848, @Ladsgroup wrote:

It's not a hard problem to solve, we can just invalidate the cache for the property on page edit of that property. That way, we can even increase the TTL to a day even.

Good point.

Although this gets trickier if we want to remove the revision ID from the label cache key (as described above), rather than have a separate cache for the revision ID; because that means we need to invalidate (or update) lots of cache entries on an edit (one per language and term type). Or we change the cache key even more and store all the terms for a property in a single cache entry; but that means we’re loading way more data than needed (all languages rather than just one) when reading from the cache.

Since the edit itself is structured, maybe we can invalidate only the language and the term type that has changed? That should address most issues. If that's the case, we can even switch both properties and items to stop using rev id so we don't need to split the logic between them. Do you think it'd work?

---

Outcome from a discussion with @Sadiya.Mohammed_WMDE and @HasanAkgun_WMDE: We think the approach of removing the revision ID from the cache key for property terms (labels/descriptions/aliases) sounds promising, and we want to investigate how to move ahead with this. The especially interesting question will be where the cache invalidation happens, and how we can do it – ideally, we would have a diff of the property edit available at that point, in which case we can write exactly the changed terms to the cache, and don’t have to invalidate all entries for the property.

56% are properties.

mysql:research@dbstore1009.eqiad.wmnet [wikidatawiki]> select lt_namespace, count(*) from pagelinks join linktarget on pl_target_id = lt_id where pl_from_namespace = 0 group by lt_namespace ;
+--------------+------------+
| lt_namespace | count(*)   |
+--------------+------------+
|            0 | 1241509143 |
|          120 | 1583924611 |
|          146 |      52979 |
|          640 |        229 |
+--------------+------------+
4 rows in set (25 min 28.454 sec)