Page MenuHomePhabricator

Find out the reason and potentially eliminate ParserCache split on action:render
Open, MediumPublic

Description

Currently ParserCache splits on whether the parse is an immediate result of Action=render or not. The reason for this split is unclear, but removing it most likely will allow us to cut the size of the ParserCache by a factor of 2.

We should investigate why the split exist and strive for removing the split.

Event Timeline

18:46 Pchelolo: TimStarling: I wonder, maybe you know - why is entire ParserCache split on action=render vs everything else? https://github.com/wikimedia/mediawiki/blob/master/includes/parser/ParserCache.php#L114 We've been looking at it with Daniel today and could find the reason, thought maybe you could know right away. if not I'll keep figuring it out
18:47 TimStarling: because action=render is (was?) used for commons file description transclusion and so contained fully qualified URLs instead of domain-relative URLs
18:48 TimStarling: Ls
18:48 TimStarling: xchat sucks so much now, I cannot see the "Ls" at the end of that line unless I select it
18:49 TimStarling: should switch to irccloud or something like everyone else
18:50 TimStarling: yeah it is still the same, see FileRepo::getDescriptionRenderUrl() which is fetched in ForeignDBFile::getDescriptionText()
18:54 Pchelolo: hm.. but how does it affect the result of the parse? like, when Action=render is called, it's not setting any special parser options or calling any special methods on a parser?
18:55 Pchelolo: it only sets viewIsRenderAction which doesn't seem to propagate into the parser
18:56 TimStarling: you're not going to like this Pchelolo, are you ready?
18:56 Pchelolo: I'm sitting
18:56 TimStarling: it is the exact same hack that we implemented when commons first started, on line 2300 of Title.php:
18:56 TimStarling: @todo FIXME: This causes breakage in various places when we
18:56 TimStarling:
actually expected a local URL and end up with dupe prefixes.
18:56 TimStarling: if ( $wgRequest->getVal( 'action' ) == 'render' ) {
18:56 TimStarling: $url = $wgServer . $url;
18:56 TimStarling: }
18:56 TimStarling: Title::getLocalUrl() is hacked up to look at the query string
18:59 TimStarling: it's actually had a fixme comment since it was introduced by Brion in 2006 https://www.mediawiki.org/wiki/Special:Code/MediaWiki/12621
19:01 Pchelolo: it even indicates that the correct fix would be to move prepending elsewhere :)
19:01 TimStarling: sorry actually the hack was on the LHS of this change too, but that is where the fixme was introduced
19:03 TimStarling: without the fixme comment it goes back to July 2005, this change has both the parser cache hack and the URL hack: https://www.mediawiki.org/wiki/Special:Code/MediaWiki/9864

Yuck. Can we at least fold this into ParserOptions somehow? Or we convert local to absolute URLs in ParserOutput::getText(), or in the render action...

I was thinking introduce 'useAbsoluteUrls' ParserOption should be the easiest. Will let us drop this hack from both Title and ParserCache, but will proliferate the cache split.

Converting local to absolute URLs in a post-processing step... There is $options array there, and we already pass some special options for render action, but I don't know how easy would it be to replace it on the fly with a regex.. I mean, it would be easy, but I'm not sure how fragile it would be. Don't know if there's a lot of corner cases. Will investigate.

Change 629454 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Remove old hack for splitting ParserCache on action=render

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

I'm going to unroot it and put into backlog for now. This doesn't at all block the project, but it's a great opportunity to eliminate a horrific piece of technical debt.

Change 654487 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/extensions/ImageMap@master] Prepare for Title::getLocalURL to stop producing full URLs

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

Change 654503 had a related patch set uploaded (by Ppchelko; owner: Ppchelko):
[mediawiki/core@master] Temporary debug log if Title::getLocalURL is used from render action

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

Change 654503 merged by jenkins-bot:
[mediawiki/core@master] Temporary debug log if Title::getLocalURL is used from render action

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