r69191 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r69190‎ | r69191 | r69192 >
Date:17:01, 8 July 2010
Author:demon
Status:ok (Comments)
Tags:
Comment:
(bug 24124) Diffs are taking 10 to 20 seconds to load. Use parser cache for page preview when diffing to current version (which is the most often). Cuts execution time down on my sample page from 1141.44ms down to 13.78ms on a pcache hit
Modified paths:
  • /trunk/phase3/includes/diff/DifferenceInterface.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/diff/DifferenceInterface.php
——@@ -421,8 +421,10 @@
422422 $wgOut->wrapWikiMsg( "<div class='mw-warning plainlinks'>\n$1\n</div>\n", 'rev-deleted-text-view' );
423423 }
424424
 425+ $pCache = true;
425426 if( !$this->mNewRev->isCurrent() ) {
426427 $oldEditSectionSetting = $wgOut->parserOptions()->setEditSection( false );
 428+ $pCache = false;
427429 }
428430
429431 $this->loadNewText();
——@@ -441,6 +443,13 @@
442444 $wgOut->addHTML( htmlspecialchars( $this->mNewtext ) );
443445 $wgOut->addHTML( "\n</pre>\n" );
444446 }
 447+ } elseif( $pCache ) {
 448+ $pOutput = ParserCache::singleton()->get( new Article( $this->mTitle ), $wgUser );
 449+ if( $pOutput ) {
 450+ $wgOut->addHtml( $pOutput->getText() );
 451+ } else {
 452+ $wgOut->addWikiTextTidy( $this->mNewtext );
 453+ }
445454 } else {
446455 $wgOut->addWikiTextTidy( $this->mNewtext );
447456 }

Follow-up revisions

RevisionCommit summaryAuthorDate
r69204Cleanup to r69191: use addParserOutput() instead of addHtml(), specify curren...demon12:43, 9 July 2010
r69414MFT r69191, r69204: use parser cache to display article HTML when diffing to ...tstarling01:44, 16 July 2010

Comments

#Comment by Ævar Arnfjörð Bjarmason (talk | contribs)   21:38, 8 July 2010

Awesome.

#Comment by RobLa (talk | contribs)   01:07, 9 July 2010

Awesome indeed. Adding 1.16wmf4 since this will hopefully make Pending Changes work a lot more smoothly.

#Comment by Tim Starling (talk | contribs)   05:30, 9 July 2010

$wgOut->addHTML() is incorrect here, it will break extensions such as OggHandler and SyntaxHighlight_GeSHi, which depend on <head> element modification. You should be trying to simulate addWikiTextTidy() if you want this change to have no user-visible impact. In this case that means calling $wgOut->addParserOutput(). Note that Article::view() also calls $wgOut->addParserOutput() on a parser cache hit.

There is also the question of why you choose to throw away the parser output in the case of a parser cache miss. You do have to worry about parser cache pollution when you do that, but I don't think it ends up being significantly more code. Instead of this: 

	$pOutput = ParserCache::singleton()->get( new Article( $this->mTitle ), $wgUser );
	if( $pOutput ) {
		$wgOut->addHtml( $pOutput->getText() );
	} else {
		$wgOut->addWikiTextTidy( $this->mNewtext );
	}

You would have this: 

	$article = new Article( $this->mTitle, 0 );
	$pOutput = ParserCache::singleton()->get( $article, $wgUser );
	if( $pOutput ) {
		$wgOut->addParserOutput( $pOutput );
	} else {
		$article->doViewParse();
	}

Note the second parameter to the Article constructor, which is necessary to prevent possibly incorrect interpretation of the oldid request parameter.

It would be even more elegant with a bit of refactoring in Article.php, but that's probably outside our current scope.

#Comment by 😂 (talk | contribs)   12:47, 9 July 2010

Should be better in r69204, implemented your suggestions.

#Comment by RobLa (talk | contribs)   18:36, 9 July 2010

Since ^demon fixed this in r69204, marking as "new"

Status & tagging log