Page MenuHomePhabricator

Consider using APC for the individually cached keys (e.g. 'TOO BIG') in MessageCache
Closed, ResolvedPublic

Description

I'm not entirely sure, but it seems that keys in this category currently only get this information from wanCache/MainCache/Memcached.

Putting this information in APC, as well as the actual revision text (for "TOO BIG").

We can keep using wgMaxMsgCacheEntrySize to determine whether to include these in the main 'messages' key (so that they are only loaded in the process when needed). But I don't think there is a compelling reason not to store them in LocalStore/APC at all (albeit under their own key) instead of wanCache.

Details

Related Gerrit Patches:

Event Timeline

Krinkle created this task.Nov 17 2015, 7:32 PM
Krinkle raised the priority of this task from to Needs Triage.
Krinkle updated the task description. (Show Details)
Krinkle added subscribers: Krinkle, aaron, ori.
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald TranscriptNov 17 2015, 7:32 PM
ori renamed this task from Consider useing APC for 'NONEXISTENT' and 'TOO BIG' as well in MessageCache to Consider using APC for 'NONEXISTENT' and 'TOO BIG' as well in MessageCache.Nov 17 2015, 7:55 PM
ori set Security to None.
Krinkle updated the task description. (Show Details)Oct 10 2016, 11:46 PM
Krinkle updated the task description. (Show Details)Oct 12 2016, 5:53 PM
Gilles triaged this task as Medium priority.Dec 7 2016, 5:40 PM
Krinkle lowered the priority of this task from Medium to Low.Mar 2 2017, 5:12 AM

@aaron To take a look at whether this is an easy/quick to do.

(Or add stats)

aaron renamed this task from Consider using APC for 'NONEXISTENT' and 'TOO BIG' as well in MessageCache to Consider using APC for 'TOO BIG' keys and for non-existing messages that have no i18n default in MessageCache.Jun 23 2018, 3:12 PM
aaron renamed this task from Consider using APC for 'TOO BIG' keys and for non-existing messages that have no i18n default in MessageCache to Consider using APC for 'TOO BIG'/'NONEXISTENT' keys and for non-existing messages that have no i18n default in MessageCache.Jun 23 2018, 3:39 PM
aaron renamed this task from Consider using APC for 'TOO BIG'/'NONEXISTENT' keys and for non-existing messages that have no i18n default in MessageCache to Consider using APC for the individually cached keys (e.g. 'TOO BIG') in MessageCache.Jun 23 2018, 3:53 PM
aaron added a comment.EditedJun 23 2018, 4:07 PM

For the case of message with no page definitions but rather hook-based definitions:
Looking at MWOAuthUIHooks::onMessagesPreLoad, I see that the use of MWOAuthDAOAccessControl would be problematic for APC use, though a process cache could still be used.

For clarity, !NONEXISTENT was only mentioned in the title since it is sometimes the value of !TOO BIG keys when things are out of sync for a bit. The normal case of individual key use is only for large keys (and always for keys *thought* to large and existing).

Looking at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/441601/

I assume the reason they're not fine for APC is not because of access control (e.g. vary by user or session), but rather that the content pre-filled there might change without a purge strategy?

I suppose that means we should mark them as un-cacheable somehow. Which makes me wonder, is there a code path that can save to cache without re-creating MessageCache::mCache? Right now this change sets the values there from the hook, which is fine as long as anything re-populating APC will re-create that from scratch without triggering the hooks. Can you confirm that is the case?

I suppose it's fine to leave hook-defined messages outside of the main cache key in APC, and also out of APC in generally. Messages that aren't in Memcached either, can stay as-is for the purpose of this task.

What I mainly wondered about, for this task, is messages that are "too big" to put in the combined APC key, and are currently fetched from Memcached each time. Is it possible to have those cached in APC as well? That would help on re-repeat fetches. They can presumably follow the same purge/validate strategy as for the main combined blob.

aaron added a comment.Jun 25 2018, 6:01 AM

Looking at https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/441601/
I assume the reason they're not fine for APC is not because of access control (e.g. vary by user or session), but rather that the content pre-filled there might change without a purge strategy?

Purging as well as varying on RequestContext. The access control wrapper in OAuth is mostly about checking permission to see things.

I suppose that means we should mark them as un-cacheable somehow. Which makes me wonder, is there a code path that can save to cache without re-creating MessageCache::mCache? Right now this change sets the values there from the hook, which is fine as long as anything re-populating APC will re-create that from scratch without triggering the hooks. Can you confirm that is the case?

There are two paths that use loadFromDB() as the value and save it and a third one that saves such a value from memcached back to APC. The ad-hoc mCache key/value pair added throughout the request are not (and can't) be saved to persistent cache. Even before that patch, the ad-hoc !NOTEXISTANT values were also not saved (though they could probably be since a new MW: page would trigger a rebuild of cache).

I suppose it's fine to leave hook-defined messages outside of the main cache key in APC, and also out of APC in generally. Messages that aren't in Memcached either, can stay as-is for the purpose of this task.
What I mainly wondered about, for this task, is messages that are "too big" to put in the combined APC key, and are currently fetched from Memcached each time. Is it possible to have those cached in APC as well? That would help on re-repeat fetches. They can presumably follow the same purge/validate strategy as for the main combined blob.

They could be keyed using the same HASH field the memcached key does, except an APC key.

Change 441844 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] [WIP] Make MessageCache use APC for big messages

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

Change 441844 merged by jenkins-bot:
[mediawiki/core@master] Make MessageCache use APC for big messages

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

aaron closed this task as Resolved.Aug 14 2018, 6:16 PM