r70657 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70656‎ | r70657 | r70658 >
Date:00:28, 8 August 2010
Author:nikerabbit
Status:resolved (Comments)
Tags:
Comment:
Allow negative caching in process cache and use it, return early in replace() if cannot use db
Modified paths:
  • /trunk/phase3/includes/MessageCache.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/MessageCache.php
——@@ -376,38 +376,38 @@
377377 global $wgMaxMsgCacheEntrySize;
378378 wfProfileIn( __METHOD__ );
379379
 380+ if ( $this->mDisable ) {
 381+ return;
 382+ }
380383
381384 list( , $code ) = $this->figureMessage( $title );
382385
383386 $cacheKey = wfMemcKey( 'messages', $code );
384 - $this->load($code);
385 - $this->lock($cacheKey);
 387+ $this->load( $code );
 388+ $this->lock( $cacheKey );
386389
387 - if ( is_array($this->mCache[$code]) ) {
388 - $titleKey = wfMemcKey( 'messages', 'individual', $title );
 390+ $titleKey = wfMemcKey( 'messages', 'individual', $title );
389391
390 - if ( $text === false ) {
391 - # Article was deleted
392 - unset( $this->mCache[$code][$title] );
393 - $this->mMemc->delete( $titleKey );
 392+ if ( $text === false ) {
 393+ # Article was deleted
 394+ $this->mCache[$code][$title] = '!NONEXISTENT';
 395+ $this->mMemc->delete( $titleKey );
394396
395 - } elseif ( strlen( $text ) > $wgMaxMsgCacheEntrySize ) {
396 - # Check for size
397 - $this->mCache[$code][$title] = '!TOO BIG';
398 - $this->mMemc->set( $titleKey, ' ' . $text, $this->mExpiry );
 397+ } elseif ( strlen( $text ) > $wgMaxMsgCacheEntrySize ) {
 398+ # Check for size
 399+ $this->mCache[$code][$title] = '!TOO BIG';
 400+ $this->mMemc->set( $titleKey, ' ' . $text, $this->mExpiry );
399401
400 - } else {
401 - $this->mCache[$code][$title] = ' ' . $text;
402 - $this->mMemc->delete( $titleKey );
403 - }
404 -
405 - # Update caches
406 - $this->saveToCaches( $this->mCache[$code], true, $code );
 402+ } else {
 403+ $this->mCache[$code][$title] = ' ' . $text;
 404+ $this->mMemc->delete( $titleKey );
407405 }
408 - $this->unlock($cacheKey);
409406
 407+ # Update caches
 408+ $this->saveToCaches( $this->mCache[$code], true, $code );
 409+ $this->unlock( $cacheKey );
 410+
410411 // Also delete cached sidebar... just in case it is affected
411 - global $parserMemc;
412412 $codes = array( $code );
413413 if ( $code === 'en' ) {
414414 // Delete all sidebars, like for example on action=purge on the
——@@ -415,6 +415,7 @@
416416 $codes = array_keys( Language::getLanguageNames() );
417417 }
418418
 419+ global $parserMemc;
419420 foreach ( $codes as $code ) {
420421 $sidebarKey = wfMemcKey( 'sidebar', $code );
421422 $parserMemc->delete( $sidebarKey );
——@@ -592,11 +593,12 @@
593594 $message = false;
594595
595596 $this->load( $code );
596 - if (isset( $this->mCache[$code][$title] ) ) {
 597+ if ( isset( $this->mCache[$code][$title] ) ) {
597598 $entry = $this->mCache[$code][$title];
598 - $type = substr( $entry, 0, 1 );
599 - if ( $type == ' ' ) {
 599+ if ( substr( $entry, 0, 1 ) === ' ' ) {
600600 return substr( $entry, 1 );
 601+ } elseif ( $entry === '!NONEXISTENT' ) {
 602+ return false;
601603 }
602604 }
603605
——@@ -606,24 +608,15 @@
607609 return $message;
608610 }
609611
610 - # If there is no cache entry and no placeholder, it doesn't exist
611 - if ( $type !== '!' ) {
612 - return false;
613 - }
614 -
615 - $titleKey = wfMemcKey( 'messages', 'individual', $title );
616 -
617612 # Try the individual message cache
 613+ $titleKey = wfMemcKey( 'messages', 'individual', $title );
618614 $entry = $this->mMemc->get( $titleKey );
619615 if ( $entry ) {
620 - $type = substr( $entry, 0, 1 );
621 -
622 - if ( $type === ' ' ) {
623 - # Ok!
624 - $message = substr( $entry, 1 );
 616+ if ( substr( $entry, 0, 1 ) === ' ' ) {
625617 $this->mCache[$code][$title] = $entry;
626 - return $message;
 618+ return substr( $entry, 1 );
627619 } elseif ( $entry === '!NONEXISTENT' ) {
 620+ $this->mCache[$code][$title] = '!NONEXISTENT';
628621 return false;
629622 } else {
630623 # Corrupt/obsolete entry, delete it
——@@ -631,18 +624,17 @@
632625 }
633626 }
634627
635 - # Try loading it from the DB
 628+ # Try loading it from the database
636629 $revision = Revision::newFromTitle( Title::makeTitle( NS_MEDIAWIKI, $title ) );
637 - if( $revision ) {
 630+ if ( $revision ) {
638631 $message = $revision->getText();
639632 $this->mCache[$code][$title] = ' ' . $message;
640633 $this->mMemc->set( $titleKey, ' ' . $message, $this->mExpiry );
641634 } else {
642 - # Negative caching
643 - # Use some special text instead of false, because false gets converted to '' somewhere
 635+ $this->mCache[$code][$title] = '!NONEXISTENT';
644636 $this->mMemc->set( $titleKey, '!NONEXISTENT', $this->mExpiry );
645 - $this->mCache[$code][$title] = false;
646637 }
 638+
647639 return $message;
648640 }
649641

Follow-up revisions

RevisionCommit summaryAuthorDate
r78179Fix regression in r70657. Misplaced else condition was causing cache misses...nikerabbit13:18, 10 December 2010
r81695Back out trunk r70657. It's broken without the $wgAdaptiveMessageCache-relate...tstarling07:06, 8 February 2011
r81696Back out trunk r70657. It's broken without the $wgAdaptiveMessageCache-relate...tstarling07:08, 8 February 2011

Comments

#Comment by Tim Starling (talk | contribs)   11:12, 17 November 2010

This is a severe performance regression, causing about 100 extra DB queries for a parser cache hit on a default installation. There already was a negative cache: if the $this->mCache entry did not exist, the page does not exist. Maybe a move away from this was preparatory to r71412?

MessageCache::loadFromDB() does not set !NONEXISTENT cache entries as would be expected in this new system, which means that for a message with no corresponding page (i.e. all of them in a default install), MessageCache::getMsgFromNamespace() falls through to the individual message cache. The default message cache is the objectcache table, so we get about 100 queries of the form: 

SELECT /* SqlBagOStuff::get 127.0.1.1 */  value,exptime  FROM `objectcache`  WHERE keyname = 'installtest:messages:individual:Pagetitle'  LIMIT 1

The individual message cache was meant to just be for the "!TOO BIG" case, it's too slow to be using for every call to wfMsg().

Status & tagging log