jenkins-bot has submitted this change and it was merged. Change subject: User: Simplify process cache by using WANObjectCache::getWithSetCallback ......................................................................
User: Simplify process cache by using WANObjectCache::getWithSetCallback Follows-up 7d67b4d919, 9c733318. * Convert loadFromId() to use getWithSetCallback() and centralise cache access logic there instead of spread between loadFromCache() and saveToCache(). * Remove process cache from User class (added in 9c733318). Instead, tell WANObjectCache to process-cache the key for 30 seconds. * No need to deal with process cache in purge() because load uses slaves by default and may be lagged. Reads that require READ_LATEST already bypass the cache. * Remove saveToCache() and move logic to loadFromCache(). It was technically a public method, but marked private and no longer used in any extensions. * Remove redundant isAnon() check in loadFromCache(). This is already done by loadFromId() and loadFromDatabase(). * Remove hasOrMadeRecentMasterChanges() check. It was used to add READ_LATEST to the flags. However, this check only occurred if either READ_LATEST was already set, or after consulting cache. Which means in general, it never does anything. If we want to keep this, we should probably move it higher up. * Let WANObjectCache handle cache version. That way, there is no longer separate logic for "populate cache" and "cache lookup failed". Instead, there is just "get data" that tries cache first. I've considered moving the version into the cache key (like we do elsewhere) but that would be problematic here since User cache must be purgeable cross-wiki and other wikis may run a different version (either in general, or even just during a deployment). As such, the key must remain unchanged when the version changes so that purges from newer wikis affect what older wikis see and vice versa. Change-Id: Icfbc54dfd0ea594dd52fc1cfd403a7f66f1dd0b0 --- M includes/user/User.php M tests/phpunit/MediaWikiTestCase.php 2 files changed, 29 insertions(+), 88 deletions(-) Approvals: Aaron Schulz: Looks good to me, approved jenkins-bot: Verified diff --git a/includes/user/User.php b/includes/user/User.php index ff3171e..9e50f36 100644 --- a/includes/user/User.php +++ b/includes/user/User.php @@ -198,12 +198,6 @@ */ protected static $mAllRights = false; - /** - * An in-process cache for user data lookup - * @var HashBagOStuff - */ - protected static $inProcessCache; - /** Cache variables */ // @{ /** @var int */ @@ -425,6 +419,7 @@ */ public function loadFromId( $flags = self::READ_NORMAL ) { if ( $this->mId == 0 ) { + // Anonymous users are not in the database (don't need cache) $this->loadDefaults(); return false; } @@ -432,17 +427,13 @@ // Try cache (unless this needs data from the master DB). // NOTE: if this thread called saveSettings(), the cache was cleared. $latest = DBAccessObjectUtils::hasFlags( $flags, self::READ_LATEST ); - if ( $latest || !$this->loadFromCache() ) { - wfDebug( "User: cache miss for user {$this->mId}\n" ); - // Load from DB (make sure this thread sees its own changes) - if ( wfGetLB()->hasOrMadeRecentMasterChanges() ) { - $flags |= self::READ_LATEST; - } + if ( $latest ) { if ( !$this->loadFromDatabase( $flags ) ) { - // Can't load from ID, user is anonymous + // Can't load from ID return false; } - $this->saveToCache(); + } else { + $this->loadFromCache(); } $this->mLoadedItems = true; @@ -458,10 +449,8 @@ */ public static function purge( $wikiId, $userId ) { $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); $key = $cache->makeGlobalKey( 'user', 'id', $wikiId, $userId ); $cache->delete( $key ); - $processCache->delete( $key ); } /** @@ -474,44 +463,34 @@ } /** - * @since 1.27 - * @return HashBagOStuff - */ - protected static function getInProcessCache() { - if ( !self::$inProcessCache ) { - self::$inProcessCache = new HashBagOStuff( [ 'maxKeys' => 10 ] ); - } - return self::$inProcessCache; - } - - /** * Load user data from shared cache, given mId has already been set. * - * @return bool false if the ID does not exist or data is invalid, true otherwise + * @return bool True * @since 1.25 */ protected function loadFromCache() { - if ( $this->mId == 0 ) { - $this->loadDefaults(); - return false; - } - $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); - $key = $this->getCacheKey( $cache ); - $data = $processCache->get( $key ); - if ( !is_array( $data ) ) { - $data = $cache->get( $key ); - if ( !is_array( $data ) - || !isset( $data['mVersion'] ) - || $data['mVersion'] < self::VERSION - ) { - // Object is expired - return false; - } - $processCache->set( $key, $data ); - } - wfDebug( "User: got user {$this->mId} from cache\n" ); + $data = $cache->getWithSetCallback( + $this->getCacheKey( $cache ), + $cache::TTL_HOUR, + function ( $oldValue, &$ttl, array &$setOpts ) { + $setOpts += Database::getCacheSetOptions( wfGetDB( DB_SLAVE ) ); + wfDebug( "User: cache miss for user {$this->mId}\n" ); + + $this->loadFromDatabase(); + $this->loadGroups(); + $this->loadOptions(); + + $data = []; + foreach ( self::$mCacheVars as $name ) { + $data[$name] = $this->$name; + } + + return $data; + + }, + [ 'pcTTL' => $cache::TTL_PROC_LONG, 'version' => self::VERSION ] + ); // Restore from cache foreach ( self::$mCacheVars as $name ) { @@ -519,35 +498,6 @@ } return true; - } - - /** - * Save user data to the shared cache - * - * This method should not be called outside the User class - */ - public function saveToCache() { - $this->load(); - $this->loadGroups(); - $this->loadOptions(); - - if ( $this->isAnon() ) { - // Anonymous users are uncached - return; - } - - $data = []; - foreach ( self::$mCacheVars as $name ) { - $data[$name] = $this->$name; - } - $data['mVersion'] = self::VERSION; - $opts = Database::getCacheSetOptions( wfGetDB( DB_SLAVE ) ); - - $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); - $key = $this->getCacheKey( $cache ); - $cache->set( $key, $data, $cache::TTL_HOUR, $opts ); - $processCache->set( $key, $data ); } /** @name newFrom*() static factory methods */ @@ -1269,8 +1219,8 @@ // Paranoia $this->mId = intval( $this->mId ); - // Anonymous user if ( !$this->mId ) { + // Anonymous users are not in the database $this->loadDefaults(); return false; } @@ -2396,16 +2346,13 @@ } $cache = ObjectCache::getMainWANInstance(); - $processCache = self::getInProcessCache(); $key = $this->getCacheKey( $cache ); if ( $mode === 'refresh' ) { $cache->delete( $key, 1 ); - $processCache->delete( $key ); } else { wfGetDB( DB_MASTER )->onTransactionPreCommitOrIdle( - function() use ( $cache, $processCache, $key ) { + function() use ( $cache, $key ) { $cache->delete( $key ); - $processCache->delete( $key ); } ); } diff --git a/tests/phpunit/MediaWikiTestCase.php b/tests/phpunit/MediaWikiTestCase.php index 47d96e1..e0a7ea3 100644 --- a/tests/phpunit/MediaWikiTestCase.php +++ b/tests/phpunit/MediaWikiTestCase.php @@ -1215,12 +1215,6 @@ // If any of the user tables were marked as used, we should clear all of them. if ( array_intersect( $tablesUsed, $userTables ) ) { $tablesUsed = array_unique( array_merge( $tablesUsed, $userTables ) ); - - // Totally clear User class in-process cache to avoid CAS errors - TestingAccessWrapper::newFromClass( 'User' ) - ->getInProcessCache() - ->clear(); - TestUserRegistry::clear(); } -- To view, visit https://gerrit.wikimedia.org/r/274331 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Icfbc54dfd0ea594dd52fc1cfd403a7f66f1dd0b0 Gerrit-PatchSet: 10 Gerrit-Project: mediawiki/core Gerrit-Branch: master Gerrit-Owner: Krinkle <krinklem...@gmail.com> Gerrit-Reviewer: Aaron Schulz <asch...@wikimedia.org> Gerrit-Reviewer: Anomie <bjor...@wikimedia.org> Gerrit-Reviewer: Gergő Tisza <gti...@wikimedia.org> Gerrit-Reviewer: Krinkle <krinklem...@gmail.com> Gerrit-Reviewer: Ori.livneh <o...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits