Page MenuHomePhabricator

Deal with poor edit stash hit rate due to Lua modules using {{REVISIONID}}
Closed, ResolvedPublic

Description

Migration guide


Preview detection

The most common use case for {{REVISIONID}} is to detect preview mode. This will not be broken.

The magic word will still be valid in wikitext, and will still return "" (empty string) when previewing edits, and something else (a dash) when reading pages after saving the edit.

Edit summary permalinks for advanced user actions

On Meta-Wiki, it is common to use {{REVISIONID}} in combination with {{fullurl:}} to give privileged users a way to quickly perform an action with the edit summary already pre-configured to reference the current version of the page. For example, when fulfilling a request from another user.

This feature has been accommodated with a small JavaScript snippet, that can be used in templates via class="tpl-permalink-reason". An example can be found at Meta-Wiki JS and Template:Checkuser.


Original task description

The line:

local ispreview = frame:preprocess( "{{REVISIONID}}" ) == "" and 1 or 0

is extremely bad for performance since:
a) it disables edit stashing on any page that uses this template (e.g. via infoboxes).
b) it causes a double parse on edit due to vary-revision in doEditUpdates()

Maybe a dedicated {{PREVIEWMODE}} word can be added instead?

Edit stash graphs: https://grafana.wikimedia.org/dashboard/db/edit-stash

I suspect most of the "uncacheable" sector of the pie is from this.

List of main enwiki templates doing this:

  • Module:Check for unknown parameters
  • Module:Convert
  • Module:Citation/CS1
  • Module:TemplatePar

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron added a subscriber: Legoktm.Jun 15 2016, 8:36 PM
aaron updated the task description. (Show Details)Jun 15 2016, 8:58 PM

Change 294642 had a related patch set uploaded (by Aaron Schulz):
[WIP] Add {{PREVIEWMODE}} magic word

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

aaron updated the task description. (Show Details)Jun 15 2016, 10:22 PM
aaron updated the task description. (Show Details)

How would such a magic word avoid the need to disable caching / fragmenting renders between previews & actual view?

Could we instead discourage this kind of behavior change on preview? What are the use cases that drive this, and can they be supported more sanely?

We don't share the renders, so that's not an issue. Stashing/doEditContent/doEditUpdates all happen in non-preview mode. The preview output is not cached anywhere.

aaron added a subscriber: Catrope.Jun 15 2016, 10:54 PM

Stashing/doEditContent/doEditUpdates all happen in non-preview mode.

I see, that makes this less of a concern for that particular case.

I am still doubtful about introducing a new parser function. VisualEditor and Parsoid probably already get a mix of preview & normal render content, depending on whether users switched from wikitext or not. A new parser function could be clamped to always return false in that context, but then people will probably just continue to look at {{REVISIONID}}, thereby not really solving the problem. And, we'd have another not-quite-working parser function to maintain.

ori added a subscriber: ori.Jun 15 2016, 11:16 PM

Could we instead discourage this kind of behavior change on preview? What are the use cases that drive this, and can they be supported more sanely?

Lua modules use this to insert debug messages into the preview HTML. Take a look at Module:Check for unknown parameters to see what I mean.

It is indeed an abuse of preview, because the intended purpose of preview is to, well, pre-view how the page would look like if the staged editor were saved. Preview-specific HTML defeats that.

Possible solutions:

  • Don't vary the output at all, and instead rely on CSS to show / hide these messages. Possibly a non-starter if it causes debug messages to show up in search results.
  • Don't vary the output at all, and instead use JavaScript to scrape and display specially-marked <!-- comments --> when previewing.
  • Don't vary the output at all, and provide an API for Lua to log messages that are saved in ParserOutput but not as part of the HTML.
  • Just explain that preview warnings are an abuse of previews and try to persuade editors to stop using them. This is my favorite option, since I question the value and sanity of these debug messages. But I don't think it would go over well with editors. I could be wrong, though.

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

aaron renamed this task from Deal with poor edit stash hit rate due to "Module:Check for unknown parameters" to Deal with poor edit stash hit rate due to Lua modules using {{REVISIONID}}.Jun 15 2016, 11:31 PM
aaron updated the task description. (Show Details)
aaron updated the task description. (Show Details)Jun 15 2016, 11:33 PM

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

Yes, this seems like a better solution.

Change 294774 had a related patch set uploaded (by Aaron Schulz):
[WIP] Disable expensive REVISION* magic words in miser mode

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

aaron added a comment.Jun 16 2016, 8:19 PM

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

These don't have locational context, but I'd agree that they'd be useful for some things. Errors like "invalid param 'foo'" would just mean the user has to cntrl-f for 'foo' to find that parameter.

Change 294776 had a related patch set uploaded (by Legoktm):
Expose ParserOutput::addWarning() to modules

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

We already have a show-only-on-preview warning system through ParserOutput::addWarning() and ParserOutput::getWarnings(), I would be in favor of exposing that via Scribunto/Lua and allowing module authors to take advantage of showing real errors upon preview instead of the current hacks.

These don't have locational context, but I'd agree that they'd be useful for some things. Errors like "invalid param 'foo'" would just mean the user has to cntrl-f for 'foo' to find that parameter.

I'm pretty sure that the lack of locational context would keep enwiki from switching to this.

aaron updated the task description. (Show Details)Jun 17 2016, 1:10 AM
ori added a comment.Jun 17 2016, 2:25 AM

I'm pretty sure that the lack of locational context would keep enwiki from switching to this.

I don't see why. I see how locational context is useful, but performance is important too, and so is having preview functionality that is faithful to the final HTML.

Change 294776 merged by jenkins-bot:
Expose ParserOutput::addWarning() to modules

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

Izno added a subscriber: Izno.EditedJun 18 2016, 12:20 AM

I would personally think of these templates emitting these errors during preview as a different kind of edit notice, which have been implemented in the software for a while.

Most of these templates do have a tracking category, or even emit visible errors (CS1 does, at least) into the page content. What these sometimes-complex templates are trying to prevent is adding another page edit to fix some error that could have been caught in preview.

Some of this problem goes away when editors aren't touching the source directly as with VisualEditor ("wrong/old/deprecated parameters"), but other errors are emitted due to the value that the parameter takes and not the parameter name itself ("check ISBN for goodness").

Personally, I think a patch that permits a module to access the API to determine whether a page is in preview mode is a good thing, due to those use cases. I'm fairly certain a magic word is unnecessary, but would not be opposed to it.

Change 295109 had a related patch set uploaded (by Aaron Schulz):
[WIP] Try to predict the rev_id when preparing edits

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

Change 294642 abandoned by Aaron Schulz:
[WIP] Add {{PREVIEWMODE}} magic word

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

Change 295109 merged by jenkins-bot:
Try to predict the rev_id when preparing edits

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

Change 296794 had a related patch set uploaded (by Aaron Schulz):
Try to predict the rev_id when preparing edits

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

Change 296794 merged by jenkins-bot:
Try to predict the rev_id when preparing edits

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

GWicke added a comment.EditedJul 26 2016, 10:33 PM

These "preview-only" warnings are causing Parsoid render issues like this:

/cc @ssastry @tstarling

aaron closed this task as Resolved.Aug 3 2016, 9:54 AM

The enwiki CS1 module has been updated and the StashEdit log confirms a huge drop in vary-revision-id.

Krinkle added a subscriber: Krinkle.Apr 1 2019, 8:05 PM

Change 294642 abandoned by Aaron Schulz:
[WIP] Add {{PREVIEWMODE}} magic word

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

@Krinkle wrote at https://gerrit.wikimedia.org/r/470099 (the wrong Gerrit patch, oops):

A visual diffing job has been started at http://mw-expt-tests.wmflabs.org/ - it has two VMs with MediaWiki-Vagrant with a few dozen wikis for different projects/languages, and 64547 articles imported. The "expt" VM has this patch applied.

The label for this job is "FIX_REVID".

64,547 articles were tested. Of these 3 rendered differently between the two versions of MediaWiki. I've confirmed them all to be false negatives;

So.. looks like we're good :)

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptApr 1 2019, 8:05 PM

Change 294774 merged by jenkins-bot:
[mediawiki/core@master] Disable expensive {{REVISIONID}} magic word in miser mode

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

Anomie added a subscriber: Anomie.Apr 2 2019, 9:58 PM

Based on a conversation with @Anomie on IRC, we've identified that {{REVISIONID}} is used across templates in the projects for things such as inserting the permanent link number automatically as log reason (list of templates). Having the templates insert automatically the oldid comes very handy. Will we have a replacement for this function? Thanks.

Johan added a subscriber: Johan.EditedApr 2 2019, 10:27 PM

Edit: Nevermind. My mistake.

Krinkle reopened this task as Open.

@Johan

The magic word {{REVISIONID}} is being deprecated for performance reasons. In the future, it will only return "" (empty string) when previewing edits, or "-" (dash) when reading pages. The release of next week, will only change this behaviour for articles in the content namespaces. It will not change for interface messages and other namespaces (such as talk pages, and user pages). T137900

Krinkle updated the task description. (Show Details)Apr 4 2019, 11:13 PM
Krinkle updated the task description. (Show Details)
Krinkle updated the task description. (Show Details)
Krinkle removed Krinkle as the assignee of this task.Apr 4 2019, 11:16 PM
Krinkle moved this task from Inbox to Doing on the Performance-Team board.Apr 4 2019, 11:31 PM
Krinkle closed this task as Resolved.
geraki added a subscriber: geraki.Apr 5 2019, 6:28 AM

What about {{REVISIONTIMESTAMP}}?

aaron added a comment.Apr 5 2019, 7:13 AM

What about {{REVISIONTIMESTAMP}}?

I wouldn't add any uses of that, and ideally things should move away from it. Nevertheless, nothing about that word has changed, only REVISIONID for the moment.

Johan added a comment.Apr 5 2019, 4:38 PM

@Krinkle Thanks, very helpful.

SerDIDG added a subscriber: SerDIDG.Apr 9 2019, 9:36 PM

Now every time an edit is made to an article where there are unknown parameters, the preview-only warning is shown on view until the page is purged; see report of issues at VPT.

Now every time an edit is made to an article where there are unknown parameters, the preview-only warning is shown on view until the page is purged; see report of issues at VPT.

Correct. You should fix the hacky Lua relying on the former behaviour.

Krinkle reopened this task as Open.Apr 12 2019, 7:23 PM

@Galobtter Thanks, I can reproduce the issue. It appears to not affect null edits and purges. Must be something specific to UI edits, possibly relating to edit sash.

Now every time an edit is made to an article where there are unknown parameters, the preview-only warning is shown on view until the page is purged; see report of issues at VPT.

Correct. You should fix the hacky Lua relying on the former behaviour.

I thought the Lua module was the problem as well, e.g. checking it to be a number or something like that. But it's not. The Lua module is doing the right thing, by checking {{REVISIONID}} against the empty string ''.

The part we have removed as part of this task is that on regular views it is now the dash - instead of a number. Equality to the empty string is still the supported and only way to detect preview mode.

Krinkle closed this task as Resolved.Apr 12 2019, 11:44 PM
Krinkle claimed this task.

Closing in favour of T220854.