[08:49:35] https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Scribunto/+/1232313 2x speed-up for many/most calls to mw.ustring.gsub(), which is 0.5% of index.php wall time and a lot more than that on edits and parser cache misses. [09:03:36] I did a fair bit of this kind of work when I joined Google in 2016. It was a lot of fun, but within a few years all the low-hanging opportunities in hot code paths were picked clean, and I had to wean myself off. But looking at Wikimedia flame graphs made me fall into bad habits again. [17:13:17] ori: oh dear, what a shame (for us) ;) [21:14:26] Reedy: don't worry if not, but i was wondering if you might have an immediate idea about what might be going wrong re https://gerrit.wikimedia.org/r/c/mediawiki/extensions/ConfirmEdit/+/1231582/comment/32a6a278_5e61bac3/ (if not, don't worry, I am happy to file a task about it if I can't fix it at the same time as the others :) ) [21:23:38] Sorry, nope... [21:39:41] no worries :) i'll change that patch to just fix the issues I was able to fix & I'll file a task about that other one. thanks for taking a look! [22:28:35] A_smart_kitten: I think the wfMessage calls need to be $out->msg() to use the correct language context (qqx for the test) [22:31:07] ori: hmmm... the differences between the two is a bit out of my zone of knowledge at the moment, so I probably won't submit a patch to change that myself (at least, not yet), but i'll note it on the task i file -- ty! [22:34:31] I also noticed that myself when looking at it yesterday and I was wondering if that had something to do with it... but then, when visiting `Special:Captcha?uselang=qqx` locally, the qqx message-key *is* displayed (rather than the English message). so then I'm also wondering whether it's something specific to the way the PHPUnit tests executes the special pages... [22:41:18] A_smart_kitten: the test harness is setting a derivative context that is passed to the special page, but not changing the global context/language [22:41:47] wfMessage() uses the global request singleton so it doesn't get the override [22:44:29] ahhh, thank you :] [22:44:47] does use of wfMessage just make things harder to test then (in general)? (or am I getting the wrong takeaway from this?) [22:44:57] https://github.com/wikimedia/mediawiki/blob/76bb755dd9422e869c0fad0671de974258c4f823/tests/phpunit/includes/Specials/SpecialPageExecutor.php#L51-L58 [22:45:04] is where the magic happens, I think [22:49:29] https://www.mediawiki.org/wiki/Manual:Messages_API has a note at the top discouraging wfMessage() use [22:52:02] I'm not sure why the unit tests do it this way -- it might be to enable parallelism? (if you rely on globals you can't run tests in the same process, because they'll fight over global state). Or maybe because it's hard to reset global state perfectly after a test. It's easier to do when you inject a context that disappears automatically when it falls out of scope. [22:52:29] (thank you so much for finding those ori, they both look very relevant & helpful here :) and to think that all I had to do was RTFM & examine the function one level below the actual test itself...lol) [22:53:52] I don't think it's obvious at all and I don't think it's documented very well [23:08:54] I'd probably agree it could be documented better FWIW [23:14:31] ori: also FWIW, I just searched https://github.com/search?q=org%3Awikimedia+wfMessage+test&type=commits & I came across https://github.com/wikimedia/mediawiki-extensions-FileImporter/commit/046dfc6e6ee0dbe2, the commit message for which seems to support the idea that `wfMessage` might kinda make things harder to test [23:15:38] ...and I *also* just came across https://github.com/wikimedia/mediawiki-extensions-Cite/commit/f5b9360467a38bc5 from that search, which suggests that another way of solving this might be to call `setMwGlobals` to set `wgLanguageCode` to 'qqx' [23:18:35] it might get the test to pass, but that's not a solution -- the test harness clearly intends for the output to be mediated by the derivative request object. By making the global req match the state of the derivative context, the test might pass, but the real fix is to use $out->msg [23:27:46] I see where you're coming from. I guess it would hopefully be relatively simple to just change these two calls from the ext:ConfirmEdit file in question to use $out->msg, but then (while wfMessage is still a supported - if slightly discouraged - way of getting a message's text) I guess I wonder if anything else can/should be done to (e.g.) make special-pages that still use it easier to test [23:31:20] I'm currently busy feeling sorry for myself because the whiz-bang optimization I thought I made last night turns out to be slightly worse than useless on real-world inputs [23:37:14] the inputs to mw.ustring.gsub() are so small that the costs are dominated by overhead of the function calls rather than the work inside ustringGsub itself. [23:38:29] The good news is that a lot of these calls are redundant and can be eliminated with some straightforward changes to module Lua code [23:40:19] the bad news is that it seems every wiki has their own heavily customized fork and (if itwiki is representative) they haven't been reconciled in a decade, so it's hard to do centrally [23:47:54] TimStarling: re: real-world usage of mw.ustring.gsub() (many calls, tiny inputs). I wonder if that means the decision to delegate it to the PHP implementation was likewise wrong. The comment in UStringLibrary.php says "// Pattern matching is still much faster in PHP, even with the overhead of serialization", but the efficiency of pattern-matching might not matter.