From 0c24cefa12221fe13be75198df7a5e2300627994 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Wed, 24 May 2017 17:59:50 -0700
Subject: [PATCH] objectcache: fix cache warmup bug in
 getMultiWithSetCallback()

The warmup cache was not properly prefixed and was also using the entity
IDs instead of the cache keys. Thus, it effectively just wasted a
getMulti() query and resulted in the usual separate GETs anyway.

Added some unit tests for this.

Change-Id: I75b7a31214b515511856f9d95db32e8881d80ccc
---
 includes/libs/objectcache/WANObjectCache.php  | 33 ++++++++++++++++---
 .../libs/objectcache/WANObjectCacheTest.php   |  2 ++
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index bc6047f93d..cb1be95586 100644
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -97,6 +97,8 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 	private $callbackDepth = 0;
 	/** @var mixed[] Temporary warm-up cache */
 	private $warmupCache = [];
+	/** @var integer Key fetched */
+	private $warmupKeyMisses = 0;
 
 	/** Max time expected to pass between delete() and DB commit finishing */
 	const MAX_COMMIT_DELAY = 3;
@@ -298,10 +300,13 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 		if ( $this->warmupCache ) {
 			$wrappedValues = array_intersect_key( $this->warmupCache, array_flip( $keysGet ) );
 			$keysGet = array_diff( $keysGet, array_keys( $wrappedValues ) ); // keys left to fetch
+			$this->warmupKeyMisses += count( $keysGet );
 		} else {
 			$wrappedValues = [];
 		}
-		$wrappedValues += $this->cache->getMulti( $keysGet );
+		if ( $keysGet ) {
+			$wrappedValues += $this->cache->getMulti( $keysGet );
+		}
 		// Time used to compare/init "check" keys (derived after getMulti() to be pessimistic)
 		$now = microtime( true );
 
@@ -1103,18 +1108,30 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 	final public function getMultiWithSetCallback(
 		ArrayIterator $keyedIds, $ttl, callable $callback, array $opts = []
 	) {
-		$keysWarmUp = iterator_to_array( $keyedIds, true );
 		$checkKeys = isset( $opts['checkKeys'] ) ? $opts['checkKeys'] : [];
+
+		$keysWarmUp = [];
+		// Get all the value keys to fetch...
+		foreach ( $keyedIds as $key => $id ) {
+			$keysWarmUp[] = self::VALUE_KEY_PREFIX . $key;
+		}
+		// Get all the check keys to fetch...
 		foreach ( $checkKeys as $i => $checkKeyOrKeys ) {
 			if ( is_int( $i ) ) {
-				$keysWarmUp[] = $checkKeyOrKeys;
+				// Single check key that applies to all value keys
+				$keysWarmUp[] = self::TIME_KEY_PREFIX . $checkKeyOrKeys;
 			} else {
-				$keysWarmUp = array_merge( $keysWarmUp, $checkKeyOrKeys );
+				// List of check keys that apply to value key $i
+				$keysWarmUp = array_merge(
+					$keysWarmUp,
+					self::prefixCacheKeys( $checkKeyOrKeys, self::TIME_KEY_PREFIX )
+				);
 			}
 		}
 
 		$this->warmupCache = $this->cache->getMulti( $keysWarmUp );
 		$this->warmupCache += array_fill_keys( $keysWarmUp, false );
+		$this->warmupKeyMisses = 0;
 
 		// Wrap $callback to match the getWithSetCallback() format while passing $id to $callback
 		$id = null;
@@ -1316,6 +1333,14 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
 		return (int)min( $maxTTL, max( $minTTL, $factor * $age ) );
 	}
 
+	/**
+	 * @return integer Number of warmup key cache misses last round
+	 * @since 1.30
+	 */
+	public function getWarmupKeyMisses() {
+		return $this->warmupKeyMisses;
+	}
+
 	/**
 	 * Do the actual async bus purge of a key
 	 *
diff --git a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
index 72effd79ea..dcb09867e3 100644
--- a/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
+++ b/tests/phpunit/includes/libs/objectcache/WANObjectCacheTest.php
@@ -310,10 +310,12 @@ class WANObjectCacheTest extends PHPUnit_Framework_TestCase  {
 			$keyedIds, 30, $genFunc, [ 'lowTTL' => 0, 'lockTSE' => 5, ] + $extOpts );
 		$this->assertEquals( $value, $v[$keyB], "Value returned" );
 		$this->assertEquals( 1, $wasSet, "Value regenerated" );
+		$this->assertEquals( 0, $cache->getWarmupKeyMisses(), "Keys warmed yet in process cache" );
 		$v = $cache->getMultiWithSetCallback(
 			$keyedIds, 30, $genFunc, [ 'lowTTL' => 0, 'lockTSE' => 5, ] + $extOpts );
 		$this->assertEquals( $value, $v[$keyB], "Value returned" );
 		$this->assertEquals( 1, $wasSet, "Value not regenerated" );
+		$this->assertEquals( 0, $cache->getWarmupKeyMisses(), "Keys warmed in process cache" );
 
 		$priorTime = microtime( true );
 		usleep( 1 );
-- 
2.20.1