In T240474#5745959, @Gilles wrote:

Body of the template coming later in the quarter, I assume?

One hopes. I created this as a structural blocker for deployment from Release Engineering's perspective for the Editing team, but actually asking for this is happnening at management level, AIUI.

Hey @Esanders or @marcella can you complete this task for the team? I forwarded you an email to pull info from.

Thanks for updating this @Esanders . @Gilles , we have completed the review ticket.

I've just a quick first look (by no means the full review, I will let you know when I'm done finding things of interest).

The first thing that strikes me is how expensive the auto-expanding textarea is. Presumably this is a pre-existing OOUI feature that is merely being reused here, but basically every keystroke induces a layout+paint - which is an expensive operation - due to the JS size checks done on content changes:

On low-powered devices this will cause framerate drops and feeling of unresponsiveness. At its most extreme it can give a feeling of "sticky typing".

A simple mitigation would be to debounce this event. If the person is typing very fast, there's no point running the size check for every single keystroke. Every 100-200ms at most would be sufficient and reduce the performance penalty a little. And the text being typed won't create a scrollbar or cutoff for more than that debounce duration.

A better solution, however, would be to use a contenteditable element rather than a textarea, which has the native ability of auto-expanding without the need for any javascript. And only falling back to a JS-autoexpanded textarea for browsers that don't support contenteditable.

In terms of parsing the comment via the API, it seems like the typing is correctly debounced and the in-flight XHT requests correctly cancelled. Nicely done on that front!

In terms of JS loading strategy, you seem to have picked the best tradeoff. The JS is low-priority and loaded on talk pages only.

While loading any JS isn't free, even low-priority lazy-loaded (it can cause jank during pageload on low-powered devices), in this case the likelihood of feature usage is very high on a talk page and it makes more sense to do this rather than, for example, only load the JS when users click on the reply link.

Kudos for using the recommended best practices there.

A better solution, however, would be to use a contenteditable element rather than a textarea, which has the native ability of auto-expanding without the need for any javascript. And only falling back to a JS-autoexpanded textarea for browsers that don't support contenteditable.

Thanks, we will probably be switching to CE based inputs in the near future anyway.

Good to hear! FYI, I'm heading to Wikilead module 1 right now, and the week after I'm on vacation. As a result I expect to pick up work on this again on Feb 24.

@Esanders I understand the feature hasn't launched "for real" yet (it's behind an opt-in gesture), but as part this week's deployment to several Wikipedias the side-wide costs are now in effect, ahead of the perf review. E.g. server-side setup and client-side startup costs on page views, and any backend pressures from it or core that it exposes can become risk factors.

I noticed a startup regression on nlwiki today. The extension currently registers 8 additional ResourceLoader modules which is more than the recommended maximum for new developments. It looks like it might only need two. Could you look into that meanwhile?

We might need a few more modules than two. We have a bit of debug code that should not be loaded on normal page views (ext.discussionTools.debug), and an optional implementation of the reply box that uses VE, which is mutually exclusive with the normal one (ext.discussionTools.ReplyWidgetPlain/ext.discussionTools.ReplyWidgetVisual). But there's probably 3 or 4 other modules that can be merged into one.

@matmarex Great :) - Regarding the debug files, would debugScripts work for (some) of that? It seems ext.discussionTools.debug is immediately loaded by ext.discussionTools.init which suggest it might work as a debug script delivered within the same module bundle.

debugScripts is IMO awful for debugging in production because it requires RL debug mode, which disables all the RL features that make it fast, so the page takes a minute or longer to actually load. It's only bearable for local development when you have 0ms ping to the server.

I misread new mw.Uri().query.dtdebug as new mw.Uri().query.debug. I didn't realise a new debug parameter was introduced as part of DiscussionTools.

[…] the RL features that make it fast,

I assume you're referring to RL's current debug mode which, for legacy modules, means they fetch and execute serially. That's indeed slow. For package files, this is not the case anymore though. Are you sure this applies to DT?

In any case, the new debug mode we're working on this year will effectively behave more like production (in that it adopts packageFiles' debug mode of shipping raw code, but using one request per module instead of one request per source file), and this will apply to legacy modules. (The slow old debug mode will be retained via something like debug=old, until we have source maps). For 99% of debugging I believe concatenation is tolerable while debugging. And at least for myself, I haven't used debug mode in years. Production mode with prettification suffices in most cases, and is what the new debug mode will effectively be (but with original whitespace and comment, and with module name easily discoverable in the url, given 1 req per module instead of batched).

I understand the need for this debugging feature. However, I can't in good concious approve of the current workaround. The module count introduced by DT cuts 10% (0.5K of 4.5K) into our safety margin for the startup module, which is the budget we everyone has to share for all products and other feature developers for the next year(s). We have to stop module bundles as a way to solve problems :)

If this it is critical that we ship it in DT in this manner then it's probably important enough to either warrant helping me free up budget to make this possible, and/or for me to drop what I'm doing and build the new debug mode we've been waiting for.

Alternatively, perhaps we start out using the regular debug mode for 3-6 months and move so something differenet later. Possibly by using debugScripts in the interim, or a different temporary compromise.

Change 574630 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Merge RL modules which are only loaded by 'ext.discussionTools.init'

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

Change 574631 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/DiscussionTools@master] Merge RL module 'ext.discussionTools.ReplyWidget' into modules that use it

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

@Krinkle Your thoughts on these patches would be welcome. Especially on the second one https://gerrit.wikimedia.org/r/574631, since it's a bit funky. They reduce the number of modules from 9 (we added another one while you weren't looking ;) ) to 4.

In T240474#5914376, @Krinkle wrote:

Alternatively, perhaps we start out using the regular debug mode for 3-6 months and move so something differenet later. Possibly by using debugScripts in the interim, or a different temporary compromise.

I'd rather delete the module and copy-paste it into the browser console whenever I need it like a caveman, than make it use debugScripts. I'll go ahead and do it if you insist. RL debug mode is the worst. Just like you, I haven't used debug mode for a long time, except when I have to debug weird issues in VE, where we have a debugging feature that loads via debugScripts, and that's how I know how terrible it is.

In T240474#5905816, @Krinkle wrote:

I noticed a startup regression on nlwiki today. The extension currently registers 8 additional ResourceLoader modules which is more than the recommended maximum for new developments. It looks like it might only need two. Could you look into that meanwhile?

For all the reasons we discussed on T225842 [1] requiring us to use one or two monolithic modules is not a good idea. If the startup module size is critical then patches like that should be prioritised, rather than putting a tech debt burden on our team all product developers.

1. Especially "This means that when a feature is later refactored or removed, one has to manually check where dependencies and messages were used and which ones can be safely removed." as this will likely lead to non-startup performance regressions in the medium term

Change 574631 abandoned by Bartosz Dziewoński:
Merge RL module 'ext.discussionTools.ReplyWidget' into modules that use it

Reason:
These modules will probably work differently anyway when we implement switching between plain and visual, and it's easier for development to leave it like this for now.

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

@Esanders if T225842 was implemented, how many different modules would DiscussionTools use instead of the current status quo of 9?

I think we could squeeze it down to two (debug and everything else) if we don't mind loading both of the reply widget implementations and their dependencies at once despite only ever using one of them on a given wiki by config. That's definitely a performance trade-off, since one of them is the one which pulls in a large chunk of VisualEditor.

I'd say three, if we kept ReplyWidgetPlain in the core package and just split out ReplyWidgetVisual because of the heavier dependency load.

In T240474#5945568, @DLynch wrote:

[…] loading both of the reply widget implementations and their dependencies at once despite only ever using one of them on a given wiki by config.

If it can be determined server-side by config, then this sacrifice won't be needed and we can get the best of both. Module definitions are allow to vary by wiki, skin, user language and thus (implicitly) the wiki's site-wide config. This can be done on a per-file basis with a packageFile's callback (docs), or for the module as a whole by defining it from the ResourceLoaderRegisterModules hook. There's a number of examples of this in production already.

Looking at PHP code, the only new data stored by the extension are recent change tags, is that correct?

In T240474#5953948, @Gilles wrote:

Looking at PHP code, the only new data stored by the extension are recent change tags, is that correct?

Yes, everything else is done by just editing pages.

Taking another tour, I don't think there's anything else to report.

To summarise:

• Typing is potentially slow on low-powered devices due to the excessive sizing-related callbacks induced by the auto-expand feature. Suggested remedy is to use contenteditable, which allows auto-expand as a native feature.
• The amount ofnewly introduced ResourceLoader modules is excessive, 9 modules being an unreasonable budget for a new feature only dedicated to talk pages. The debate on how to go about this seems unresolved and/or dependent on T225842, but I think everyone understands the tradeoffs involved and how development convenience is currently at the expense of visitors with slow internet access, who are seeing the amount of blocking <head> data inflated by those module declarations, delaying the rendering of page content on all wiki pages, not just talk pages.

I highly suggest filing tasks for both of these. You can mark the second one as blocked on T225842 if you believe it to be the case.

Please let us know what your plan is to follow up on these issues and if you do file such tasks or not, after which which I will close this performance review.

In T240474#5945635, @Krinkle wrote:

In T240474#5945568, @DLynch wrote:

[…] loading both of the reply widget implementations and their dependencies at once despite only ever using one of them on a given wiki by config.

If it can be determined server-side by config, then this sacrifice won't be needed and we can get the best of both. Module definitions are allow to vary by wiki, skin, user language and thus (implicitly) the wiki's site-side config. This can be done on a per-file basis with a packageFile's callback (docs), or for the module as a whole by defining it from the ResourceLoaderRegisterModules hook. There's a number of examples of this in production already.

packageFiles callbacks don't seem to allow changing the dependencies of the module, so we'd have to move the definition to PHP code, which is super unfun (and avoiding that was my main motivation for using packageFiles for everything in this extension). I supposed we could just not use dependencies, and just wrap the entire module's code in mw.loader.using( dependencies ) instead (it's already lazy-loaded).

In T240474#5954001, @Gilles wrote:

• Typing is potentially slow on low-powered devices due to the excessive sizing-related callbacks induced by the auto-expand feature. Suggested remedy is to use contenteditable, which allows auto-expand as a native feature.

I would not expect contenteditable to fare better. If anything, I'd expect it to be slower, since potentially the browser would have to re-layout the entire page on every keystroke rather than just the hidden textbox, and since it can have all kinds of text formatting that is not possible in a textarea. My experience with VE's source mode tells me that contenteditable definitely can be slow.

I don't think any action is really needed though. I just tested the current implementation on a Huawei Y5 2019 phone (which, I was told, was the worst Android device they had at the store when I was buying it a few months ago, and which is definitely sluggish at everything) and typing in our reply widget had no delay.

In T240474#5954001, @Gilles wrote:

• The amount ofnewly introduced ResourceLoader modules is excessive, 9 modules being an unreasonable budget for a new feature only dedicated to talk pages. The debate on how to go about this seems unresolved and/or dependent on T225842, but I think everyone understands the tradeoffs involved and how development convenience is currently at the expense of visitors with slow internet access, who are seeing the amount of blocking <head> data inflated by those module declarations, delaying the rendering of page content on all wiki pages, not just talk pages.

We talked about this today in the engineering meeting. We decided that we should go ahead with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/574630, which reduces the 9 modules to 5, and which seemed like a clear improvement to the code (I'll update the patch soon, it needs rebasing). Out of the remaining modules, 3 are used for ReplyWidget stuff, and the structure of that code might need to change soon as we work on visual editor for the replies and editor switching; it would be quite inconvenient and a bit premature to optimize it right now and we'd like to leave it alone. Hopefully that's an acceptable compromise.

With respect to fixing the auto-expand widget upstream, obviously VE's source mode is by far the most lightweight implementation of a CE editor for plain text, but there is a lot of work that goes into making CE behave like a plain textarea: we have to disable native formatting (Ctrl+B works in a CE), hijack rich paste, copy and a bunch of other issues.

That patch sounds like an acceptable compromise, thank you for doing that and being responsive on the review. I will keep the task open until the patch is merged. For all intents and purpose, the performance review is done.

(I updated the patch: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/DiscussionTools/+/574630)

Change 574630 merged by jenkins-bot:
[mediawiki/extensions/DiscussionTools@master] Merge RL modules which are only loaded by 'ext.discussionTools.init'

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