[02:59:20] Lucas_WMDE: it made no difference (same benchmark, getAll() for enwiki on a deployment host with the change live-patched) [03:11:59] It does "work" though. I ran my benchmark script with `php -d zend_extension=opcache.so -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.opt_debug_level=0x20000` to dump the optimized generated opcodes. Without the leading slash, PHP emits: [03:12:28] 0008 INIT_NS_FCALL_BY_NAME 2 string("MediaWiki\\Config\\array_key_exists") [03:12:30] 0009 SEND_VAL_EX string("default") 1 [03:12:34] With the leading slash, PHP emits: [03:12:42] 0008 T5 = ARRAY_KEY_EXISTS string("default") CV0($thisSetting) [03:16:12] it's just that in this particular case (I906d5b3f2) the value of the optimization is in all the hairy tag-iteration logic that gets short-circuited. Making the array_key_exists() a shade faster doesn't meaningfully move the needle. [03:24:00] standalone benchmark: https://phabricator.wikimedia.org/P86767 (LLM-generated). [03:25:19] On a production host, PHP 8.3.26, 10m iterations: array_key_exists(): 0.2389 seconds, \array_key_exists(): 0.1681 seconds (~30% faster) [03:28:59] well, but.. [03:31:30] remembering now that the short-circuit only applies to a third of cases and the fallback code is also array_key_exists()-heavy, I fully-qualified the array_key_exists() calls in the fallback code as well [03:32:28] With that, getAll() for enwiki goes from 0.952 ms to 0.688 ms, ~40% faster! [03:55:09] Lucas_WMDE: please create a change, it's a nice win and should be credited to you. You can cite these numbers in your commit message if you like. [11:35:42] ori: nice, thanks! I’ll try to put together the commit [11:36:16] I was wondering if this would be something to enforce via phpcs, but if we do it across the whole code base I think it might end up being a wash performance-wise, since we’d be adding more code bytes to parse 🤔 [11:38:53] bah, `use array_key_exists;` doesn’t work? I thought we could declare the non-namespaced function that way :/ [11:45:05] Lucas_WMDE: `use function array_key_exists;` iirc? [11:47:52] ori, Krinkle: https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1223644 [11:47:58] taavi: ah thanks, https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1223645 [16:55:48] hey all, genuine question - do folks think that there's value in having T410514 open as a general task for that specific PHP deprecation? asking because i'm planning to file tasks relating to specific pieces of code that need addressing (for that PHP8.5 deprecation); but i'm not currently 100% sure of its utility as a (parent) task in general, [16:55:49] T410514: Using null as array offset or as the key parameter for array_key_exists() is deprecated in PHP 8.5 - https://phabricator.wikimedia.org/T410514 [16:55:55] given that (to me) this seems like something that might more need to be addressed on the basis of individual deprecation reports, as opposed to something that can (e.g.) easily be found & fixed by Codesearch. [17:14:25] I think I would be tempted to just attach a bunch of changes directly to that general task, and not bother creating subtasks for each occurrence [17:14:44] maybe with subtasks for broad groups like “core”, “WMF-deployed extensions”, “WMF-deployed skins” etc. [17:15:03] but I don’t know remember how tasks have been handled for other deprecations. I think various patterns have been used [17:17:50] There's no one size fits all answer ;) [17:29:00] fair fair [17:31:34] Lucas_WMDE: i guess the thing in my mind is that i could (e.g.) create a 'core' subtask, but then right now i only know about the failures in core that i'm seeing when running the tests locally [17:31:48] whereas there might be more that the tests aren't covering [17:32:34] so i guess it wouldn't be clear to me what the acceptance criteria for that task would be :p [17:32:44] at some people we just close it [17:32:53] because it's stale/superceded/whatever [17:33:52] Reedy: yeah i mean, that makes sense [17:35:22] anyways, there are 14 files in core that cause that specific error in tests (when I run them locally, I don't think they're visible on `check experimental` jobs yet bc of the order in which WMF CI runs the tests) [17:36:32] would it be better to file them as one task (for CI failures caused by this error in core) or separately? happy to do either but it does strike me that it might be a bit of an unwieldy task description if i try to put them all in the same one.. [17:39:52] I don't think there's any real right answer... [17:40:12] They're all a similar class of problem, and if they're in the same codebase... [17:43:02] 'I don't think there's any real right answer...' yeah, i see :/ [17:43:08] i might file them separately, if only because i have no idea which ones might be more complicated to resolve 'properly' / which might involve more discussion than others. [17:43:16] ty for putting up with my questions :) [17:43:42] Yeah, I don't think anything is wrong with that [17:43:59] Maybe group together if it's in the same code file where it's occuring etc, but beyond that... [18:14:20] funnily enough that's actually what i've already done - i've (locally) sorted the deprecation errors by type & by file that they occur in. all that is left is to file the actual tasks about them :p [20:06:01] A_smart_kitten: do you want to update your parsoid patch? Sounds like we can get it merged (and backported!) pretty quickly with one of your propsed solutions [20:35:09] Reedy: yep, will hopefully do within the next few hours (and barring that, early-ish tomorrow) :) it's been on my mental list of things to do but i kept either getting sidetracked or having other stuff to do, apologies! [21:18:21] heh [21:18:23] rabbit holes [21:20:40] indeed indeed [22:34:24] Krinkle: would be nice to have a 24.05.2 version of fresh that includes f9a996f . [23:20:06] Lucas_WMDE: enforcing this via phpcs would be very nice. It's fair to assume there are (and will be) other hot code paths using array_key_exists(). It's basically free. [23:23:53] Anyone want to +2 https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EventStreamConfig/+/1220048 ? It got a +1 from Ottomata [23:44:21] lol [23:44:39] wikibase uses array_key_exists 160 times vs core's 187