Module talk:debug/track

expandTemplate is bad for tracking

I am personally a fan of transclusion-based tracking (even though a sort key cannot be provided like in category-based tracking), however, expandTemplate, unnecessarily wastes resources expanding the wikitext of the target page.

frame:expandTemplate{ title = page } is essentially the same as frame:callParserFunction{ 'msg', { page } } which in turn are basically the same as wikitext like {{#if:{{page}}}} and {{#if:{{msg:page}}}} where the #if ensures the expansion of its conditional predicate while not using it in the actual result—i.e, discarding the actual result (notice I don't bother to define results for the true or false paths as they are unneeded and thus should both be empty).

So while expandTemplate is wrapped in pcall, I can still cause the pages using this to fail by doing something nasty to the target page transclused in this way. For example, if I saved the page Wiktionary:Tracking/links/alt-ignored with the contents <includeonly>{{Wiktionary:Tracking/links/alt-ignored}}</includeonly> this will cause a template loop error on all the pages expanding it via transclusion (which currently is about 1500 pages).

To avoid the unnecessary expansion one can instead do {{#if:{{msgnw:page}}}} or in Scribunto Lua frame:callParserFunction{ 'msgnw', { page } } since the msgnw magic word transcludes the target page without expanding its wikitext. However, in Scribunto Lua there is an even better way: mw.title:getContent(). For example this module could do something like:

-- transclusion-based tracking as subpages of [[Wiktionary:Tracking]] or [[Special:Redirect/page/10142436]]
-- it is possible allow this to be renamed by only referring to this by pageID , etc. but so far that is "parser function expensive"
-- another option is mw.title.new(mw.wikbase.getSitelink(QID)) but currently it has no Wikidata item association
local basetitle = mw.title.makeTitle(4, 'Tracking') -- mw.title.new(10142436) is "expensive"
basetitle:subPageTitle(key):getContent() -- returns essentially the same as "msgnw" parser function

Of course to be robust and avoid illegal page title names this still need some error checking, however, I think this is just checking the return value of mw.title:subPageTitle() as I believe mw.title:getContent() never throws an error and we do not care if it returns nil when the target page does not exist (it also is not "parser function expensive" so this is a good way to test if a page exists). I shall try to work up something in the sandbox. —Uzume (talk) 06:20, 22 April 2024 (UTC)

@Surjection, Benwing2, Theknightwho: So, I haven't really tested this much but I put something in the sandbox that should be more performant as it does not attempt to expand the target(s) via the parser like the current expandTemplate implementation does and it does not need to trap errors via pcall() either. —Uzume (talk) 10:10, 24 April 2024 (UTC)

@Uzume @Benwing2 @Surjection I find that :getContent() becomes slow with scale, as this issue affects Chinese transliteration on pages that link to thousands of other pages.

There is a much, much faster way if we track using the module namespace, though: package.loaders[2](modname) is extremely fast (as in, millions of times per second), and since we don’t care about the result would avoid the mess with grabbing page content. It might even work with non-module pages, since it just returns nil if nothing is found, so isn’t limited to existing pages in the same way require is.

Theknightwho (talk) 13:21, 24 April 2024 (UTC)

After testing this, it only works with the module namespace, but in all honesty that might be desirable anyway, since this kind of tracking is generally only useful for module development. Theknightwho (talk) 15:26, 24 April 2024 (UTC)

Profiling both methods tracking 10,000 pages gives results that are basically the same (with :getContent() edging out slightly), so in light of that @Uzume's solution is probably best. For comparison, the current method takes over 7 seconds to do the same thing. Theknightwho (talk) 15:39, 24 April 2024 (UTC)

Try instead comparing the situation where a single page repeatedly attempts to create the same tracking over and over 10,000 times. That is probably the type of situation you are seeing an improvement in, in the Chinese transliteration cases that you found faster. My transclude() function in the sandbox currently makes no attempt to optimize this situation. —Uzume (talk) 16:08, 24 April 2024 (UTC)

@Uzume package.loaders[2]() is much faster when caching a page which has already been loaded, but slightly slower with new pages. However, repeat loading is >1000x faster than the first load with both methods, so I don't think the speed advantage of package.loaders[2]() is relevant there. Theknightwho (talk) 16:13, 24 April 2024 (UTC)

@Theknightwho: It faster because of the Scribunto Lua-based memoization of module loading. It is slower because not only does it have to do the very small amount of work required for memoization and loads the module via php.loadPackage() but prior to that it runs php.loadPHPLibrary() which walks all the Scribunto library hook functions allowing other extensions to add their own local libraries (e.g., Extension:Wikibase client loads mw.wikibase code this way), etc. —Uzume (talk) 17:53, 24 April 2024 (UTC)

@Theknightwho: Let me see if I understand your suggestion. You are suggesting the possibly of moving tracking from the Wiktionary namespace to the Module namespace to facilitate the usage of the Lua package loaders environment used by require() since it has to create a transclusion link as well and the tracking can still be queried by the likes of Special:WhatLinksHere (or other tools that can query the link tables via SQL like toolforge tools, etc.). Now let me infer as to why you think this is considerably faster. You are running into (the possibly quite common) situation where the parsing of a single page attempts to create the same tracking repeatedly over and over due to how the code is organized and executes. And since the require() package loaders implement caching, this eliminates actually fetching the contents of the target page over and over again and thus yields marked improvement in your wiki environment. Does that capture your thoughts adequately? —Uzume (talk) 16:02, 24 April 2024 (UTC)

@Uzume Not quite - I was running it with a single page, but package.loaders[2]() doesn't cache in the same way require() does; in fact, package.loaders[2]() is what require() runs in the first place before it's able to cache at all. However, I think there must be some kind of caching of the raw page content PHP-side as well, which would explain the very fast results. Theknightwho (talk) 16:08, 24 April 2024 (UTC)

@Theknightwho: I am almost certain the memoization of require() is on the Lua side of things in the package loaders (which you were directly using). —Uzume (talk) 16:12, 24 April 2024 (UTC)

@Uzume I'm not quite sure what you mean: package.lua (which contains the Scribunto implementation of require()) runs package.loaders[2]() at line 75. The function returned by the loader is run at line 103, and the result (if any) is memoised at line 105. You don't get Lua memoization if you run the loader directly, but I'm certain there's memoization happening in the PHP as well. Theknightwho (talk) 16:16, 24 April 2024 (UTC)

@Theknightwho: look at packageCache in mw.lua (where loadPackage() is stuffed into package.loaders). —Uzume (talk) 16:23, 24 April 2024 (UTC)

@Uzume Sure, makes sense. I don't have time right now, but I'll also test the difference between the two methods tracking the same page under many separate invocations, because that might reveal a difference in how much they're able to take advantage of caching. Theknightwho (talk) 16:31, 24 April 2024 (UTC)

@Theknightwho: Scribunto does not reinitialize Lua for every #invoke (which is instead initialized for each parser page render; these are in turn cached in the page cache and updated via the dependency chains of the page links via work issued to the job queue). What it does do is create a new isolated Lua environment for every #invoke. mw.lua is loaded into the global environment at Scribunto initialization time. Most of the rest of the .lua files like package.lua are loaded into each individual #invoke environment (where the specified module is loaded as the anonymous "main" module rather than how require() loads modules as "libraries"). This means that require() can only memoize across calls in the same #invoke (there is no need to load a library that is already loaded). However, there is a higher-level package memoization across each #invoke. This is what mw.loadData(), mw.loadJsonData() and the package loader used by require() you found directly use. mw.loadData(), mw.loadJsonData() also validate their data and memoize that. Interestingly mw.loadJsonData() seems to have its own PHP entry so perhaps it allows one to load things in other namespaces. —Uzume (talk) 17:08, 24 April 2024 (UTC)

@Uzume Yes, that's correct, but it looks as though package.loaders[2]() caches for the whole page, since packageCache is initialised in mw.lua, which is the same place loadedData is. I don't know whether that also applies to page content loaded via getContent(), though, since in Lua the cache is only applied to the specific title object (lines 173-176 of mw.title). Theknightwho (talk) 17:49, 24 April 2024 (UTC)

@Theknightwho: You are right. As far as I know, the libraries currently do not perform any memoization across getContent() calls (either within a #invoke or across them). The libraries probably could do that as a nice optimization (what would be the point of considering dynamic changes to a page's content during the parsing of another page?). —Uzume (talk) 18:00, 24 April 2024 (UTC)

@Theknightwho: I was wrong, apparently getContent() does memoize across calls, at least for the same mw.title object (which of course cannot be easily shared across #invoke calls). You can see that at includes/Engines/LuaCommon/lualib/mw.title.lua § line 73 where for a particular mw.title object it replaces the getContent() function with one that returns a static value after fetching the contents the first time. The problem is that one can create multiple mw.title objects for the same page title so it doesn't really help us unless we cache the mw.title objects. But that is a tad silly for our use here. This module should probably memoize calls to it with the same subpage "keys" (ignoring such if we did that earlier). Then we do not even need to create the subpage mw.title object to begin with. —Uzume (talk) 18:40, 24 April 2024 (UTC)

We might be able to optimize in a somewhat similar way as require() by memoizing calls to getContents() (e.g., in my transclude() function). We also might be able to take even better advantage of this same Lua package loaders infrastructure via the likes of mw.loadData() or mw.loadJsonData(), the latter of which is documented as taking a "page" vs. a "module". I believe they both optimize caching across multiple uses of #invoke (which might be tricky with our own memoization). —Uzume (talk) 16:02, 24 April 2024 (UTC)

@Theknightwho: I added some caching to my sandbox implementation. At first I considered it might be possible to have multiple mw.title objects referring to the same page that were created from different subpage "keys" (e.g., 'xyzzy/var' vs. 'abc/../xyzzy/var') but apparently that type of syntax is not accepted and just treated as an error (returning nil). So maybe I ought to cache the subpage keys instead of creating the mw.title objects and caching the full page names but for now it has some caching. Hopefully that will have some impact for your Chinese transliteration use cases. —Uzume (talk) 19:18, 24 April 2024 (UTC)

@Theknightwho: I swapped the caching to the tracking keys (which should be subpage names) instead of the full page names. —Uzume (talk) 19:45, 24 April 2024 (UTC)

@Uzume @Theknightwho Thanks Uzume for looking into this. Where do things stand currently? Is Uzume's version ready to be pushed to production or does it need more testing or reworking? Benwing2 (talk) 00:07, 25 April 2024 (UTC)

@Benwing2: It might be possible to eke out a few more performance gains by optimizing a few more things but it is already a huge improvement over the current one and as @Theknightwho seems to attest to, such small gains would be unlikely to be be worth the trouble of making things more complex to attain such. As far as I am concerned it can be deployed but perhaps you want someone's feedback that did not actually develop such (that said I have done a fair amount of such development and I have had TE@ENWP for a while now). —Uzume (talk) 02:19, 25 April 2024 (UTC)

@Uzume I just want to get @Theknightwho's OK and then I will deploy it. You seem to have a lot of experience with the ins and outs of MediaWiki (what does TE@ENWP mean?). Benwing2 (talk) 02:24, 25 April 2024 (UTC)

@Benwing2 @Uzume I've just gone over it, and I think it was a bit over-engineered, so I've made the following changes:

• Replaced the callable table with a function, as it wasn't serving any purpose. I guess we could change it so it's used as the memo table, but I don't see the point when we can just use a local for that, which avoids metamethod overhead.
• Removed the check for non-sequential keys - there's no need for it, because the current implementation only accepts one key anyway. Plus, any kind of recursion with a vararg (even if it's only once) adds overhead.
• Iterate over arg instead of using select() with ..., because it's faster. arg is a hidden argument that's available with any vararg function, which is a table of all arguments (including any nils), with the table length at key n.
• Merged the transclude() local function into the loop directly, to reduce overhead.

I'm not fully convinced it needs to be a vararg function at all, as whichever method we use to handle that adds overhead to tracking. In the cases where it needs to be done multiple times, it's probably easier to just leave that up to the calling function. Theknightwho (talk) 04:34, 25 April 2024 (UTC)

@Theknightwho The only issue with your changes is that they are incompatible with the current implementation, and there may be code that depends on being able to pass in a table or even a table with non-numeric or non-sequential keys. That's why the code was there to handle that, and throw an error on non-sequential or non-numeric keys. But I agree we don't need the varargs. Benwing2 (talk) 04:43, 25 April 2024 (UTC)

@Benwing2 I completely forgot that the current one takes a table - I've changed it to fix that. My bad. Theknightwho (talk) 04:48, 25 April 2024 (UTC)

@Theknightwho One thing we haven't considered is memory usage. Does getContent() increase memory usage? I would expect so; definitely, I didn't enable tracking of all {{lb}} labels previously (which I'd like to do) because of memory issues. So maybe we should use the other method instead? Benwing2 (talk) 04:51, 25 April 2024 (UTC)

@Benwing2 Memory impact is pretty minimal with non-existent pages: testing it with 10k pages used 1MB, and a lot of that will be inherent overhead. Theknightwho (talk) 04:54, 25 April 2024 (UTC)

@Theknightwho OK, sounds good. I wonder then why I saw significant memory impact using the expandTemplate method back in the 50MB days (on large pages, it was adding several MB of memory to enable tracking of all labels). Can you do a test with the new code and enabling tracking of all labels (temporarily change line 392 of Module:labels to true) to see how much memory is increased? Benwing2 (talk) 04:59, 25 April 2024 (UTC)

@Benwing2 I'm not sure. Since you agreed we don't need varargs, I've reworked it to more resemble the current version (i.e. it just does a type check), but the pairs loop now contains an in-built check for non-sequential keys as it goes, which avoids doing the loop twice. We can just replace it with a standard for i = 1, #t do loop once we know there aren't any.

I'll push it to the main module now to see how it performs. I'll see how water/translations and i do, since they're two big pages, but the former is mostly contained in one invoke, while the latter has hundreds. Theknightwho (talk) 05:20, 25 April 2024 (UTC)

OK, sounds good. Benwing2 (talk) 05:22, 25 April 2024 (UTC)

@Benwing2 Appreciable drop in memory use of about 2MB on very large pages with many invokes, but no noticeable difference on water/translations. They're both about 0.5 seconds faster on average - though it's hard to measure with the variation.

Turning on label tracking has no impact on memory use: it went down by 2KB on i when I turned that setting on, even though it's tracking 103 more pages. Theknightwho (talk) 05:39, 25 April 2024 (UTC)

@Theknightwho Great! Thank you. Benwing2 (talk) 06:08, 25 April 2024 (UTC)

@Benwing2: I would not expect increases in memory usage due to getContent(), in fact it should decrease compared to expandTemplate(). —Uzume (talk) 08:32, 25 April 2024 (UTC)

@Benwing2 @Uzume In light of that, I've redone the check for non-sequential keys by using table.maxn() (which gives the highest value key), then iterating from 1 to that value. It's a lot faster than using pairs. Theknightwho (talk) 04:51, 25 April 2024 (UTC)

@Theknightwho: Probably the fastest way is to just sequentially plough ahead and stop when the first value is nil. arg is not a hidden argument but a deprecated global. I believe maxn is also deprecated in Lua. —Uzume (talk) 08:14, 25 April 2024 (UTC)

@Uzume How can arg be a global when it's only accessible from inside the function? It doesn't appear in _G, either, and I don't see anything about it being deprecated in Lua 5.1: for example, it's mentioned here in the documentation, where's it's called a "hidden parameter". (Edit: I just speed profiled it, and it definitely performs like a local.) Theknightwho (talk) 10:25, 25 April 2024 (UTC)

Ah okay - I see where the confusion has come from: arg the global exists (though it's not avaialble most of the time - it seems to be something used by require), whereas arg (the implicit local vararg table) does seem to be a deprecated feature in 5.1. Theknightwho (talk) 10:44, 25 April 2024 (UTC)

@Theknightwho: The deprecated arg is never a local (implicit or otherwise). The deprecated feature is controlled by the definition of LUA_COMPAT_VARARG. The 5.1 manual also mentions "in a global table called arg". —Uzume (talk) 11:03, 25 April 2024 (UTC)

@Uzume I don't think that part of the 5.1 documentation is referring to the implicit arg in varargs, since it's only available if LUA_COMPAT_VARARG is set at compile time, whereas that section seems to be referring to calling functions in general. It also has the same performance as a local (i.e. there's no performance hit as you'd expect with global accesses) and doesn't appear in _G, so it can't be a global in any conventional sense. Theknightwho (talk) 11:15, 25 April 2024 (UTC)

@Theknightwho: Okay, I checked the source code and the code that is controlled by LUA_COMPAT_VARARG (in ldo.c and lparser.c) does seem to be referring to an implicitly created local variable. Regardless the feature is deprecated. —Uzume (talk) 11:48, 25 April 2024 (UTC)

@Uzume Hmm - are there any immediate downsides to using deprecated Lua 5.1 features in Scribunto? arg proved to be the fastest way to memoise arbitrary return values (see Module:fun#L-323), since you can catch them with a vararg and memoise the arg table straight away. Generating the equivalent table in Lua (including the n key) was quite a bit slower. Theknightwho (talk) 13:25, 25 April 2024 (UTC)

I doubt there are any immediate ones as Lua has moved on and Scribunto and luajit, and many others have yet to move. That said, it might not be a good idea to depend on such for new code. —Uzume (talk) 15:52, 25 April 2024 (UTC)

@Benwing2: By TE@ENWP I just meant I have template editor at English Wikipedia. —Uzume (talk) 08:34, 25 April 2024 (UTC)

multiple args and error checking

@Surjection, Benwing2, Theknightwho: The current code does some funny things. It allows one to pass a table and then attempts to track all the table values, however it uses pairs() and not ipairs(). Do we have or even want to support tables that aren't sequential arrays/lists? The current code also throws an error if no key value is provided (or technically if it is nil or false) however, it does not complain if an empty table is provided. Should we add an error there too?

I have been trying to improve this in the sandbox and I made the interface use Lua varargs and loop through the provided args. And then if any of the args is a table I also loop through each. This seems overkill. It seems to me we really only need to loop through the first table or all the varargs.

I propose changing the code to check if the first arg is a table and then recursively call the same function, passing the table through unpack() turning it into a vararg. And then of course continue with parsing the varargs as usual. This would reduce the looping and always allow it to check for at least one value and throw an error if not provided, however, it also removes the ability to use tables with non-sequential and/or non-numeric keys.

I am planning to implement this in the sandbox. Let me know your thoughts. —Uzume (talk) 20:19, 24 April 2024 (UTC)

Okay, I implemented this in the sandbox. Let me know if you think this breaks anything or for some reason you think it is wrong. Thank you, —Uzume (talk) 20:50, 24 April 2024 (UTC)

@Uzume I don't know why the existing code uses pairs instead of ipairs. I doubt there is code that supplies non-sequential or non-numeric keys but one thing to do is to add some code to your new sandbox version that checks to see if it encounters such a thing and throws an error if so. That way if there is any code relying on this, we can fix it, and if not, eventually we can remove the extra checking code. Benwing2 (talk) 00:06, 25 April 2024 (UTC)

@Benwing2: I can look into that, but it will likely make the code really messy. —Uzume (talk) 00:16, 25 April 2024 (UTC)

@Uzume It shouldn't be so messy and it will be just temporary, to make sure no code breaks. If you don't feel comfortable writing the code, I can do it. Benwing2 (talk) 00:24, 25 April 2024 (UTC)

@Benwing2: After looking at it, it doesn't seem too bad actually (but it will definitely slow down the path where clients supply a single table of keys). —Uzume (talk) 00:35, 25 April 2024 (UTC)

@Benwing2:  Done —Uzume (talk) 00:44, 25 April 2024 (UTC)

@Theknightwho, Benwing2: There are a few issue with the new deployment I would like to point out. First, it again does not check for an empty set of table keys so no argument is an error but an empty table is apparently kosher. I recommend just testing input[1] and throwing the error and then do a repeat ... until loop. The missing argument error could use to be pushed up with a , 2 at the end of the error() call (like the nonseq error does). I put updated code in the sandbox. The only "downside" to that code is if one provides a table with no sequential keys at all it provides the empty table error but I think that actually correct (or will be once we remove the nonseq checking). —Uzume (talk) 09:22, 25 April 2024 (UTC)

@Uzume So I did consider using next, but we'll need to make sure nothing is relying on the pairs metamethod first. I appreciate your point about the empty table - I'm not sure if it's a problem as such, but it is wasteful if anything's calling it with an empty table, so it would be good to stop that. Theknightwho (talk) 10:35, 25 April 2024 (UTC)

@Theknightwho: You can always call pairs() and manually use the returned generator for iteration. I changed my coded in the sandbox to support such. Do we care about tracking such usage? The idea is to eventually remove support for non-sequential access and then we can stop using pairs() so it won't matter will it? —Uzume (talk) 11:30, 25 April 2024 (UTC)

@Uzume You're right - it won't matter once we've got rid of pairs() and next(). Given we're seeing 0 errors thrown after several hours, it's probably safe to assume nothing is feeding it non-sequential tables anyway. Theknightwho (talk) 13:12, 25 April 2024 (UTC)

@Theknightwho Yeah I think it's fine to remove it. Benwing2 (talk) 20:25, 25 April 2024 (UTC)

@Benwing2 @Uzume Okay, I've made the following changes:

1. Removed checks for non-sequential keys: it now simply iterates from 1 until it finds a nil value.
2. Added tracking for empty table inputs; the error message is commented-out for now, but can be added once we know it's safe (since there's a real possibility of some module having a table of accumulated tracking keys that it naively pushes through before returning, even if it's empty).
3. Added tracking for invalid keys (to be replaced with an error when safe). Unlike mw.title.new, mw.title.makeTitle makes this straightforward as it doesn't normalise the input (e.g. HTML entities don't get resolved), so these are very easy to filter out. The one exception is if the key contains #, as it's treated as a fragment separator, but we can filter these out with string.find (which is extremely fast when the plain flag is set, so there's no noticeable performance hit).
4. Instead of calling basetitle:subPageTitle(key), it now just calls mw.title.makeTitle(4, "Tracking/" .. key) directly, since it's a bit faster.

Theknightwho (talk) 12:09, 26 April 2024 (UTC)

Just on this: we're already seeing some results for invalid keys, but nothing for empty table inputs yet. Theknightwho (talk) 12:16, 26 April 2024 (UTC)

@Theknightwho: "some results"!? I just checked and it was counting 344 (I suppose that isn't too bad considering the eight million transclusions of this tracking module). As for the new code, that looks very clean. I really like it. I added some comments to the sandbox version to allow easy linking for our tracking usage (since we are now consumers as well as purveyors of such tracking). —Uzume (talk) 18:47, 26 April 2024 (UTC)

It should be noted that this is not directly accessible via #invoke and all clients should be via require(). Towards that end, we currently do no validation of the provided tracking keys save looking for # and ensuring a title can be created. This has a few implications. First the find() and the .. string concatenation will throw an error if nil, a boolean, a function or a table are used. This leaves numbers which get implicitly converted to strings in these cases. Interestingly makeTitle() throws errors if numbers are passed an argument to the second parameter "title" (which we avoid with string concatenation). This means numeric strings and numbers get treated the same except for memoization where separate entries will be made for each, e.g., tracking "1" and 1, "inf" and 1/0, etc. will result in the same tracking but make extra work. It should be noted that `nil` and "nan" numbers (e.g., -(0/0)) can be used to index tables but using them as actual key values is an error (one can support assignment syntax with __newindex meta-method but rawset() will error; attempting to read from such keys always yield nil values). —Uzume (talk) 18:47, 26 April 2024 (UTC)

@Uzume Yes, I discovered these awkward edge-cases when I was writing the memoizer in Module:fun, since it iterates over the arguments and uses them as keys to store the results table in a distinct location. I had to get around it by using placeholder objects for nil, NaN, -NaN and -0 (the latter two are distinct under certain circumstances, such as conversion to string, and 1/0 ~= 1/-0). I normally wouldn't care about these kinds of cases, but it seemed important for memoisation (since it needs to support arbitrary functions). Theknightwho (talk) 19:00, 26 April 2024 (UTC)

@Theknightwho: I am not sure about -0, but I don't think it makes sense to consider NaNs that way. Floating point allows for many NaNs not just positive and negative (similar to Inf). Lua considers all NaNs to be incomparable—even to themselves (they are probably the only values that once assigned to a variable x will return true from x ~= x). I usually handle nil and NaNs during memoization with mapping through sentinel values (e.g., constant upvalues like tables or functions). —Uzume (talk) 19:18, 26 April 2024 (UTC)

@Uzume You're right from a technical perspective, but from a Lua perspective NaN can only take two distinct forms, and that's all that matters for memoisation. And yes, you're right about the identity check - the only way to check for NaN is if v ~= v then. Theknightwho (talk) 19:20, 26 April 2024 (UTC)

@Theknightwho: My point was NaNs probably do not matter for memoization and for that case, they can probably all be converted to a single sentinel value. Anyone that is using NaNs will have bigger problems than memoization of positive vs. negative NaNs. —Uzume (talk) 19:26, 26 April 2024 (UTC)

@Uzume Yes, that's basically what I did, except I used two placeholders instead of one, as you're suggesting.

To be honest, the only times I've ever run into NaN in the wild have been when something is trying to convert language codes to numbers, because "nan" is relatively common. Theknightwho (talk) 19:29, 26 April 2024 (UTC)

@Theknightwho: I recommend we easily remedy this by making everything but strings illegal by changing line 17:

• local title = not find(key, "#", 1, true) and makeTitle(4, "Tracking/" .. key)
• local title = type(key) == "string" and not find(key, "#", 1, true) and makeTitle(4, "Tracking/" .. key)

—Uzume (talk) 19:08, 26 April 2024 (UTC)

@Uzume I took a slightly different approach, because if we simply resolve title to false then the invalid key error message will still try to concatenate it. Better to do a type check earlier and throw an error for that instead. Theknightwho (talk) 19:17, 26 April 2024 (UTC)

@Theknightwho: You are right, but I would move it up before you check for memoization. Why are you creating key_type? Are you trying to optimize performance for error conditions? —Uzume (talk) 19:22, 26 April 2024 (UTC)

@Uzume Yes - I don't think it really matters, to be honest. Theknightwho (talk) 19:24, 26 April 2024 (UTC)

As a minor side note, I notice that empty subpage names are valid, i.e., mw.title.makeTitle(4, "Tracking/" .. ""). One can actually create pages such as Wiktionary:Tracking/ in MW too. I am not sure this is really an issue for tracking but it is an interesting corner case. —Uzume (talk) 18:47, 26 April 2024 (UTC)

memoization and aliasing

@Theknightwho: We should probably curate the provided keys a bit more before memoization. For example, spaces and underscores are considered equal by title mechanics but not as Lua keys. Multiple consecutive spaces or underscores are considered as only one. Trailing space or underscores are trimmed or removed. Again this is not how Lua handles string keys in tables at all. This aliasing will lead to failures to memoize and result in more work. I am sure there are other cases too (title naming semantics are nontrivial). —Uzume (talk) 12:58, 27 April 2024 (UTC)

@Uzume I'm not sure if the extra work this involves will be more expensive than the time saved, since I expect many of these issues won't be very common. Even in cases where keys are (semi-)arbitrary (e.g. many of the invalid keys come from Module:place, as it's using argument values as keys), there's likely to have been some degree of normalisation already, as arguments will have had whitespace stripped etc. Theknightwho (talk) 13:13, 27 April 2024 (UTC)

@Theknightwho: You are right. I put some sweeping broad strokes towards this in the sandbox but I am thinking there might be a better way. mw.title.makeTitle() takes some time but compared to mw.title:getContent(), this is minimal. Do we care if errors are fast? Why not not cache them and only cache successes? Then we can use the mw.title objects text key as the memoization key? That solves the aliasing issues—at least for the success cases. —Uzume (talk) 13:47, 27 April 2024 (UTC)

@Theknightwho: I put that in the sandbox now. Maybe you can see how that performs? It should be faster for successes (because it solves the aliasing issues) but slower for errors (because it does no memoization there). —Uzume (talk) 13:54, 27 April 2024 (UTC)

@Theknightwho: That was funny, I just noticed the missing return when you stuffed it in just ahead of me via edit collision. —Uzume (talk) 14:00, 27 April 2024 (UTC)

@Uzume One thing I did notice while testing this was that mw.title.new() is about 50% faster than mw.title.makeTitle(), but the latter provides better protection against invalid keys since it won't do various normalisations that we don't want (e.g. decoding HTML entities). I don't know why there's a performance difference, since they just call php.newTitle() or php.makeTitle() respectively, so it must be some difference in PHP.

On your point about caching, I've made a few changes: we still want to cache failures (since it avoids calling the function again) - this can be removed once we make invalid keys throw an error. We also need to make sure we cache the input and the normalised key, and the normalised key needs to be title.subpageText, because title.text is always prefixed with "Tracking/". Theknightwho (talk) 14:06, 27 April 2024 (UTC)

@Theknightwho: Does it matter what key we use in the cache so long as we are consistent? I figured it would be faster to use the full page name string since MW already calculates that. Why bother to spend time figuring out the subpageText? —Uzume (talk) 14:13, 27 April 2024 (UTC)

@Uzume If you track the key "foo", it tracks the page Wiktionary:Tracking/foo. In the cache, we need it to store "foo" so that any duplicate inputs immediately return. However, the title.text value is "Tracking/foo". It would be wrong to cache that, because if we then receive the input "Tracking/foo" it will immediately return, but in actual fact that should be tracking the page Wiktionary:Tracking/Tracking/foo (which is what you'll get if you input that to mw.title.makeTitle()).

Having said that, I've just realised that the subpage text might not be the value we want: it's title.text with the initial "Tracking/" stripped off, because the subpage text of Wiktionary:Tracking/Tracking/foo is still "foo".

The reason for caching both is because the input "foo " should cache "foo " and "foo". Theknightwho (talk) 14:20, 27 April 2024 (UTC)

@Theknightwho: There is no need to strip it. It is about consistent key usage. Notice how in the sandbox I stuff the full key in the cache and check the full key so there is no issue. —Uzume (talk) 14:23, 27 April 2024 (UTC)

@Uzume But if you stuff the full key in from "foo" then you get incorrect results with the key "Tracking/foo". We can't do that. Theknightwho (talk) 14:29, 27 April 2024 (UTC)

Hang on - I see you moved the initial check for the key further down. We shouldn't do that - that just wastes time processing duplicate keys. Theknightwho (talk) 14:33, 27 April 2024 (UTC)

@Theknightwho: How do you get wrong results? full pagename is always a full pagename? There are no duplicate keys, since makeTitle takes care of aliasing. —Uzume (talk) 14:34, 27 April 2024 (UTC)

@Uzume We want if memo[key] then return end to be right at the top of track(), because otherwise we waste time processing duplicate keys. Otherwise, you're running mw.title.makeTitle() (and a few other functions) for every duplicate, which is wasteful, because mw.title.makeTitle() is quite slow. This means that we need to normalise to the input form only, so "Tracking/" has to be stripped from title.text after normalisation.

I hadn't realised that you'd removed that initial memo check, which is why I didn't realise you were normalising everything to the full page name. Theknightwho (talk) 14:42, 27 April 2024 (UTC)

@Theknightwho: To remove aliasing it is a matter of calculating our own slug value for the key or letting MW do it for us with mw.title.makeTitle(). Maybe that is too slow and calculating our own slug first is better (or maybe we do not care about aliasing). My point is I would rather have many calls with valid aliased names (which will result in multiple getContent() calls for the same page if we do not handle this) be fast than be concerned about repeated erroneous calls of the exact same value (which will never call getContent() anyway). —Uzume (talk) 14:50, 27 April 2024 (UTC)

@Uzume The difference is this:

• If we check the cache right at the top, we need to do an extra sub(title.text, 10) to normalise the key.
• If we let MediaWiki do it, every duplicate needs to run type(), find() and (most importantly) mw.title.makeTitle(), which is much, much slower.

Now, we could profile this across pages to see if - statistically-speaking - duplicates are rare enough that avoiding the initial sub() is worth it, but I'm sceptical.

Theknightwho (talk) 14:59, 27 April 2024 (UTC)

┌────────────────────────────────────────────────────────────────────────────────────────────────────┘@Theknightwho: I can understand wanting to escape as much work possible by checking the memo cache as soon as possible and what you have done with normalized looks good but it is a little odd that track('a_b') will result in two new entries in the memo cache: 'a_b' (line 48) and 'a b' (line 40). On the other hand it does seem to nicely handle alias situations like: require[[Module:debug/track]]{'a b _', 'a _ b', 'a_b'}. Notice the first entry will make two entries but subsequent ones will leave no new entries despite the code never having seen them before. —Uzume (talk) 18:28, 27 April 2024 (UTC)

Also, to be clear, it's not a trivial difference: it's about 35 times slower to run mw.title.makeTitle() every time. Theknightwho (talk) 15:02, 27 April 2024 (UTC)

@Theknightwho: Calling track("debug/track/invalid key") again should be cheap though since it will succeed and will be cached. So I see no reason to consider caching errors. —Uzume (talk) 14:18, 27 April 2024 (UTC)

@Uzume Additional function calls are always more expensive than table accesses. Theknightwho (talk) 14:22, 27 April 2024 (UTC)

@Theknightwho: Exactly why I said errors will be slower. Do we care if errors are performant? —Uzume (talk) 14:25, 27 April 2024 (UTC)

@Uzume Once invalid keys are throwing errors, there's no point in caching them because there won't be any future calls (ignoring pcall, which won't be used for this). However, until then, it's totally possible for something to feed through the same invalid key multiple times, so it's faster to cache it than re-call track each time. Theknightwho (talk) 14:29, 27 April 2024 (UTC)

@Theknightwho: True but our usage will be faster because it is cached. It is about removing aliasing. Either we spend time calculating a cache slug or we just let MW do it. —Uzume (talk) 14:37, 27 April 2024 (UTC)

@Uzume It isn't: if I move caching after the if not title then block, it's about 15% slower if I try to track the same invalid title repeatedly in a loop. Theknightwho (talk) 14:47, 27 April 2024 (UTC)

@Theknightwho: Yes, errors will be slower. Do we care? —Uzume (talk) 14:53, 27 April 2024 (UTC)

@Uzume It's not an error yet, though. As I said, once it throws an error there's no need to cache it. Theknightwho (talk) 14:54, 27 April 2024 (UTC)

"debug/track/invalid key"

@Theknightwho: By calling track("debug/track/invalid key") when the provided subpage key is invalid is essentially the same as converting the provided (erroneous) key to debug/track/invalid key. So it would probably be quicker and more straight forward to handle the failure first by treating it as if that was the provided key which should even be faster than tail recursion like return track("debug/track/invalid key"). —Uzume (talk) 17:48, 27 April 2024 (UTC)

@Theknightwho: I put a simplified version of the current code in the sandbox which treats provided invalid keys as an alias for debug/track/invalid key instead of recursing. I also moved the memoization of the provided key up so it will always be memoized instead of skipping this when the normalized key is already memoized as the provided key could be a new alias of the normalized form. —Uzume (talk) 19:32, 27 April 2024 (UTC)