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
