From 35af086e57d5faa0ed282d0c67121ee684a0d2f8 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 27 Mar 2019 20:09:06 -0700
Subject: [PATCH] objectcache: make WANObjectCache prefer ADD over GET/CAS for
 misses

This avoids an extra cache query and also avoids I/O if another thread
already saved the value in the meantime, bloating the GET response.

Bug: T203786
Change-Id: I05539873f55d3254e2b9ecad0df158db1e6a1a1a
---
 includes/libs/objectcache/WANObjectCache.php | 36 ++++++++++++++------
 1 file changed, 25 insertions(+), 11 deletions(-)

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index 87bccc5208..c31b74ab15 100644
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -521,31 +521,34 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 	 * @param int $ttl Seconds to live. Special values are:
 	 *   - WANObjectCache::TTL_INDEFINITE: Cache forever (default)
 	 * @param array $opts Options map:
-	 *   - lag : Seconds of replica DB lag. Typically, this is either the replica DB lag
+	 *   - lag: seconds of replica DB lag. Typically, this is either the replica DB lag
 	 *      before the data was read or, if applicable, the replica DB lag before
 	 *      the snapshot-isolated transaction the data was read from started.
 	 *      Use false to indicate that replication is not running.
 	 *      Default: 0 seconds
-	 *   - since : UNIX timestamp of the data in $value. Typically, this is either
+	 *   - since: UNIX timestamp of the data in $value. Typically, this is either
 	 *      the current time the data was read or (if applicable) the time when
 	 *      the snapshot-isolated transaction the data was read from started.
 	 *      Default: 0 seconds
-	 *   - pending : Whether this data is possibly from an uncommitted write transaction.
+	 *   - pending: whether this data is possibly from an uncommitted write transaction.
 	 *      Generally, other threads should not see values from the future and
 	 *      they certainly should not see ones that ended up getting rolled back.
 	 *      Default: false
-	 *   - lockTSE : if excessive replication/snapshot lag is detected, then store the value
+	 *   - lockTSE: if excessive replication/snapshot lag is detected, then store the value
 	 *      with this TTL and flag it as stale. This is only useful if the reads for this key
 	 *      use getWithSetCallback() with "lockTSE" set. Note that if "staleTTL" is set
 	 *      then it will still add on to this TTL in the excessive lag scenario.
 	 *      Default: WANObjectCache::TSE_NONE
-	 *   - staleTTL : Seconds to keep the key around if it is stale. The get()/getMulti()
+	 *   - staleTTL: seconds to keep the key around if it is stale. The get()/getMulti()
 	 *      methods return such stale values with a $curTTL of 0, and getWithSetCallback()
 	 *      will call the regeneration callback in such cases, passing in the old value
 	 *      and its as-of time to the callback. This is useful if adaptiveTTL() is used
 	 *      on the old value's as-of time when it is verified as still being correct.
 	 *      Default: WANObjectCache::STALE_TTL_NONE.
+	 *   - creating: optimize for the case where the key does not already exist.
+	 *      Default: false
 	 * @note Options added in 1.28: staleTTL
+	 * @note Options added in 1.33: creating
 	 * @return bool Success
 	 */
 	final public function set( $key, $value, $ttl = self::TTL_INDEFINITE, array $opts = [] ) {
@@ -553,6 +556,7 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 		$lockTSE = $opts['lockTSE'] ?? self::TSE_NONE;
 		$staleTTL = $opts['staleTTL'] ?? self::STALE_TTL_NONE;
 		$age = isset( $opts['since'] ) ? max( 0, $now - $opts['since'] ) : 0;
+		$creating = $opts['creating'] ?? false;
 		$lag = $opts['lag'] ?? 0;
 
 		// Do not cache potentially uncommitted data as it might get rolled back
@@ -618,14 +622,23 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 
 		// Wrap that value with time/TTL/version metadata
 		$wrapped = $this->wrap( $value, $logicalTTL ?: $ttl, $now );
+		$storeTTL = $ttl + $staleTTL;
 
-		$func = function ( $cache, $key, $cWrapped ) use ( $wrapped ) {
-			return ( is_string( $cWrapped ) )
-				? false // key is tombstoned; do nothing
-				: $wrapped;
-		};
+		if ( $creating ) {
+			$ok = $this->cache->add( self::VALUE_KEY_PREFIX . $key, $wrapped, $storeTTL );
+		} else {
+			$ok = $this->cache->merge(
+				self::VALUE_KEY_PREFIX . $key,
+				function ( $cache, $key, $cWrapped ) use ( $wrapped ) {
+					// A string value means that it is a tombstone; do nothing in that case
+					return ( is_string( $cWrapped ) ) ? false : $wrapped;
+				},
+				$storeTTL,
+				1 // 1 attempt
+			);
+		}
 
-		return $this->cache->merge( self::VALUE_KEY_PREFIX . $key, $func, $ttl + $staleTTL, 1 );
+		return $ok;
 	}
 
 	/**
@@ -1418,6 +1431,7 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 				}
 			} elseif ( !$useMutex || $hasLock ) {
 				if ( $this->checkAndSetCooloff( $key, $kClass, $ago, $lockTSE, $hasLock ) ) {
+					$setOpts['creating'] = ( $curValue === false );
 					// Save the value unless a lock-winning thread is already expected to do that
 					$setOpts['lockTSE'] = $lockTSE;
 					$setOpts['staleTTL'] = $staleTTL;
-- 
2.20.1