[00:02:19] AaronSchulz: cool, taking a look now [00:03:05] AaronSchulz: do you see this as superseding WAN's own version of this? Or would that be hard to do without loss of detail? [00:39:22] Krinkle: it seems easier to keep both [00:39:56] passing size info up to wancache looked awkward when I though about the code [01:03:07] hey, I have a few messagecache questions regarding the effects of the https://phabricator.wikimedia.org/T193271 refactor [01:04:04] basically, before this change, a lookup via getMsgFromNamespace() for a message that didn't exist on the wiki would always return no content, as every message wold be stored in the main msg cache [01:04:43] however, now it is looked up in apcu+memcache each time, and this seems to create noticeable memcached i/o if the messages cannot all fit into apcu [01:05:33] in our case, it seems 10+% of all memcache traffic is lookups from loadCachedMessagePageEntry() [01:06:19] so I was wondering - what's our best option? would it make sense to adjust the MC behavior, perhaps via a flag? or should we work around it by introducing a service override or similar? [01:08:48] even a trivial request will always look up several messages that don't exist in the DB and aren't defined by any extension or core, in large part due to e.g. SkinTemplate::buildContentActionUrls() looking up a bunch of message variants like "$skname-action-$action" before falling back to the core message [01:17:36] AaronSchulz: I was thinking down rather than up. the bagostuff keeps track of it so wan doesn't need to do anything, other then making sure key class can be derived. [01:20:12] mszabo: hm.. that seems like a bug? afaik it should only go down there if the page is known to exist (or recently existed) and be too big for the main blob. [01:20:22] s/if/unless/ [01:21:00] mszabo: could you file and bug and maybe confrim stack trace to the problematic loadCachedMessagePageEntry() call? (there is more than one) [01:21:46] so, I'm not very familiar with this area of the code, but the call seems to be from this else branch - https://github.com/wikimedia/mediawiki/blob/master/includes/cache/MessageCache.php#L1118 [01:21:59] I don't think it's a bug though, I think it's an intentional change from T193271 [01:22:00] T193271: Refactor MessageCache to deal with NS_MEDIAWIKI pages that aren't standard interface messages - https://phabricator.wikimedia.org/T193271 [01:22:29] mszabo: hm.. well, yes and no. [01:22:54] a message that doesn't exist in the wiki DB and is not defined by any extension nor core will not be stored in the main blob, so if there's a lookup for it, it needs to be checked for existence every time, at least that's how I understand the current logic [01:23:26] mszabo: as far as I know, there is no reason to go down that page for a title/messagekey that is a valid localisation message key. For those we do the batch query ahead of time, and store anything small enough, with nown non-existance or '-big' branches laid out as needed. [01:24:06] but for titles that are not localisation related, e.g. random MediaWiki:Foobar.js and such, yeah, it would likely have to check always, since we decided not to invldue the entire NS_MEDIAWIKI anymore [01:24:11] yeah, the problem comes for those that are not "valid" ie neither defined nor customized, and there seems actually plenty of those on every request [01:24:27] which turned out rather problematic for e.g. Meta-Wiki where there's a large archive of unused banners and old message overrides ec. [01:24:40] OK, that makes a bit more sense. [01:24:56] Are these from core things or something in a Fandom extension? [01:25:20] https://github.com/wikimedia/mediawiki/blob/master/includes/skins/SkinTemplate.php#L991 one big source is aliases SkinTemplate looks for [01:25:49] interesting. [01:26:05] another one is from Linker::accesskey()/titleAttrib() [01:26:45] I think it probably doesn't cause an issue so long as APCu is large enough to accommodate, but since this cache needs to be sharded per wiki, our 800M SHM is filled in ~10mins [01:27:14] ack, yeah, this seems like an anti-pattern. [01:27:44] also this isn't what that change was meant to affect, it was meant to e.g. exclude wikitext/js/css revisions that aren't related to localisation. [01:28:03] but I guess that means absence of messages that aren't knkown also don't get cachd in the main blob. [01:28:14] The anti-pattern here being that the code is supposed to know what messages to exist [01:28:19] conditional existence isn't really supported imho [01:28:47] there's also no good reason for that to be conditinoal. skins know what they have and do't have. it's just legacy from pre-vector vs post-vector [01:29:16] where pre-vector skins rely on a bunch of stuff from core, and post-vector things are generally more local to the skin repo. [01:29:16] yeah, it seems like some of these might be B/C or might be customization points (?) [01:29:43] I think we can perhaps feature flag this statically, e.g. rather than checking both, the skin says which one it wants, explicitly [01:29:55] $skinname-view-history is for Vector and other new ones [01:30:03] history_short is for Monobook and such [01:30:19] it is not meant to allow for per-skin overrides within legacy skins from lcoal MW pages [01:30:23] althoguh it technically does allow that [01:30:30] ahh, didn't know the history behind that [01:30:31] e.g. even though Monobook doesn't use this right now [01:30:36] yeah, that makes sense [01:30:54] one could create Mediawiki:monobook-view-history and overide that message only for monobook users [01:31:07] but that's not a supported/dcumetned feature, just accident and afaik not relied upon [01:31:09] happy to break that [01:31:14] :) [01:31:34] mszabo: if you're looking for a quick fix, something that might work is to set useDB(false) with a core patch on some of these. [01:31:45] depending on whether you think your users are overiding those particular messages [01:31:58] or indeed remove the first or second mesage pattern if you know all your skins do it the same way [01:32:21] for accesskey, same thing, they all have defaults of "" in i18n/ dir [01:32:24] or should have, anyway. [01:32:42] yup, I don't think anything is leveraging this on our side [01:33:30] if you go the useDB(false) route, would be great to have a task for this that you can then reference in your core local patch for future ref :) [01:34:25] yeah, I'll check with the team about the approach & plan to file a task and perhaps an associated patch after we've seen the impact [01:34:41] for accesskey/tooltip, the core ones seem already covered, so that seems fine [01:34:58] https://grafana.wikimedia.org/d/lqE4lcGWz/wanobjectcache-key-group?orgId=1&var-kClass=messages_big [01:35:16] I'm having troubble assessing whether 40K/min is more than it should be or not. [01:35:33] vector hits the first patch of these afaik, so probably ok [01:35:38] but still seems fairly high [01:42:32] yeah, could be interesting to see which keys are responsiblle [01:45:20] thanks for helping to investigate! [03:16:20] Krinkle: if bagostuff could understand the makeSisterKey() key schemes it could do some magic maybe [15:59:53] Krinkle: hey :) following up from yesterday, the team decided to handle these messages via an extension - https://github.com/Wikia/mediawiki-extensions-MessageCachePerformance/pull/1 [16:18:11] phedenskog: https://hacks.mozilla.org/2021/01/improving-cross-browser-testing-part-2-new-automation-features-in-firefox-nightly/