From 8c92400909c74dff595d24a092f5965155530a48 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Tue, 19 Mar 2019 22:50:41 -0700
Subject: [PATCH] objectcache: optimize merge()/incr() for WinCacheBagOStuff

Do not hold WinCache locks during merge() callbacks, as that
could be seconds of being blocked (and some callers do not
expect indefinite blocking). Make merge() use the CAS method
which only uses wincache_lock() after callback runs in very
tight getWithToken()/set() cycle.

Make incr() use wincache_lock() since it also is very tight
doGet()/set() cycle with negligable work in between.

Change-Id: I0dde6f62c7e3d4a802470c181570ad4353d0e6ea
---
 .../libs/objectcache/WinCacheBagOStuff.php    | 69 ++++++++++++++-----
 1 file changed, 51 insertions(+), 18 deletions(-)

diff --git a/includes/libs/objectcache/WinCacheBagOStuff.php b/includes/libs/objectcache/WinCacheBagOStuff.php
index cae0280128..0ca3e4a5f9 100644
--- a/includes/libs/objectcache/WinCacheBagOStuff.php
+++ b/includes/libs/objectcache/WinCacheBagOStuff.php
@@ -29,12 +29,50 @@
  */
 class WinCacheBagOStuff extends BagOStuff {
 	protected function doGet( $key, $flags = 0 ) {
-		$val = wincache_ucache_get( $key );
-		if ( is_string( $val ) ) {
-			$val = unserialize( $val );
+		$blob = wincache_ucache_get( $key );
+
+		return is_string( $blob ) ? unserialize( $blob ) : false;
+	}
+
+	protected function getWithToken( $key, &$casToken, $flags = 0 ) {
+		$casToken = null;
+
+		$blob = wincache_ucache_get( $key );
+		if ( !is_string( $blob ) ) {
+			return false;
+		}
+
+		$value = unserialize( $blob );
+		if ( $value === false ) {
+			return false;
+		}
+
+		$casToken = $blob; // don't bother hashing this
+
+		return $value;
+	}
+
+	protected function cas( $casToken, $key, $value, $exptime = 0, $flags = 0 ) {
+		if ( !wincache_lock( $key ) ) { // optimize with FIFO lock
+			return false;
+		}
+
+		$curCasToken = null; // passed by reference
+		$this->getWithToken( $key, $curCasToken, self::READ_LATEST );
+		if ( $casToken === $curCasToken ) {
+			$success = $this->set( $key, $value, $exptime, $flags );
+		} else {
+			$this->logger->info(
+				__METHOD__ . ' failed due to race condition for {key}.',
+				[ 'key' => $key ]
+			);
+
+			$success = false; // mismatched or failed
 		}
 
-		return $val;
+		wincache_unlock( $key );
+
+		return $success;
 	}
 
 	public function set( $key, $value, $expire = 0, $flags = 0 ) {
@@ -56,14 +94,7 @@ class WinCacheBagOStuff extends BagOStuff {
 	}
 
 	public function merge( $key, callable $callback, $exptime = 0, $attempts = 10, $flags = 0 ) {
-		if ( wincache_lock( $key ) ) { // optimize with FIFO lock
-			$ok = $this->mergeViaLock( $key, $callback, $exptime, $attempts, $flags );
-			wincache_unlock( $key );
-		} else {
-			$ok = false;
-		}
-
-		return $ok;
+		return $this->mergeViaCas( $key, $callback, $exptime, $attempts, $flags );
 	}
 
 	/**
@@ -109,18 +140,20 @@ class WinCacheBagOStuff extends BagOStuff {
 	 * @return int|bool New value or false on failure
 	 */
 	public function incr( $key, $value = 1 ) {
-		if ( !$this->lock( $key ) ) {
+		if ( !wincache_lock( $key ) ) { // optimize with FIFO lock
 			return false;
 		}
-		$n = $this->get( $key );
-		if ( $this->isInteger( $n ) ) { // key exists?
-			$n += intval( $value );
+
+		$n = $this->doGet( $key );
+		if ( $this->isInteger( $n ) ) {
+			$n = max( $n + (int)$value, 0 );
 			$oldTTL = wincache_ucache_info( false, $key )["ucache_entries"][1]["ttl_seconds"];
-			$this->set( $key, max( 0, $n ), $oldTTL );
+			$this->set( $key, $n, $oldTTL );
 		} else {
 			$n = false;
 		}
-		$this->unlock( $key );
+
+		wincache_unlock( $key );
 
 		return $n;
 	}
-- 
2.20.1