objectcache: cleanup tombstone/mutex logic in doGetWithSetCallback()

This largely follows-up 6b2f13b055c84d.

* Make the comments and use of temporary variables easier to follow.
* Simplified some conditionals by remove redundant checks.
* Bypass the final set() call if $isTombstone is true. It will almost
  always be rejected due to the tombstone still being there anyway, so
  there is no point in the roundtrips. Also, the most likely case where
  it would succeed is due to the callback taking a long time to run, in
  which case data from the replication lag uncertainty period that the
  tombstone represents would be getting saved to cache with the full
  nominal TTL, which is wrong.

Change-Id: Ic28e15b24f39e128bd72ad4d905edb852bc907aa

diff --git a/includes/libs/objectcache/WANObjectCache.php b/includes/libs/objectcache/WANObjectCache.php
index 88f87f8..b0f311f 100644 (file)
--- a/includes/libs/objectcache/WANObjectCache.php
+++ b/includes/libs/objectcache/WANObjectCache.php
@@ -1264,21 +1264,26 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
                        }
                }
 
-               // A deleted key with a negative TTL left must be tombstoned
+               // Only a tombstoned key yields no value yet has a (negative) "current time left"
                $isTombstone = ( $curTTL !== null && $value === false );
-               if ( $isTombstone && $lockTSE <= 0 ) {
-                       // Use the INTERIM value for tombstoned keys to reduce regeneration load
-                       $lockTSE = self::INTERIM_KEY_TTL;
+               // Decide if only one thread should handle regeneration at a time
+               if ( $isTombstone ) {
+                       // Note that since tombstones no-op set(), $lockTSE and $curTTL cannot be used to
+                       // deduce the key hotness because $curTTL will always keep increasing until the
+                       // tombstone expires or is overwritten by a new tombstone. Also, even if $lockTSE
+                       // is not set, constant regeneration of a key for the tombstone lifetime might be
+                       // very expensive. In either case, reduce regeneration load during this time by
+                       // using the INTERIM value key with a small TTL.
+                       $useMutex = true;
+               } else {
+                       $useMutex =
+                               // Assume a key is hot if requested soon ($lockTSE seconds) after invalidation.
+                               // This avoids stampedes when timestamps from $checkKeys/$touchedCallback bump.
+                               ( $curTTL !== null && $curTTL <= 0 && abs( $curTTL ) <= $lockTSE ) ||
+                               // Assume a key is hot if there is no value and a busy fallback is given.
+                               // This avoids stampedes on eviction or preemptive renegeration taking too long.
+                               ( $busyValue !== null && $value === false );
                }
-               // Assume a key is hot if requested soon after invalidation
-               $isHot = ( $curTTL !== null && $curTTL <= 0 && abs( $curTTL ) <= $lockTSE );
-               // Use the mutex if there is no value and a busy fallback is given
-               $checkBusy = ( $busyValue !== null && $value === false );
-               // Decide whether a single thread should handle regenerations.
-               // This avoids stampedes when $checkKeys are bumped and when preemptive
-               // renegerations take too long. It also reduces regenerations while $key
-               // is tombstoned. This balances cache freshness with avoiding DB load.
-               $useMutex = ( $isHot || ( $isTombstone && $lockTSE > 0 ) || $checkBusy );
 
                $lockAcquired = false;
                if ( $useMutex ) {
@@ -1325,17 +1330,17 @@ class WANObjectCache implements IExpiringStore, LoggerAwareInterface {
                $valueIsCacheable = ( $value !== false && $ttl >= 0 );
 
                // When delete() is called, writes are write-holed by the tombstone,
-               // so use a special INTERIM key to pass the new value around threads.
-               if ( ( $isTombstone && $lockTSE > 0 ) && $valueIsCacheable ) {
-                       $tempTTL = max( 1, (int)$lockTSE ); // set() expects seconds
+               // so use a special INTERIM key to pass the new value among threads.
+               if ( $isTombstone && $valueIsCacheable ) {
+                       $tempTTL = max( self::INTERIM_KEY_TTL, (int)$lockTSE ); // set() expects seconds
                        $newAsOf = $this->getCurrentTime();
                        $wrapped = $this->wrap( $value, $tempTTL, $newAsOf );
                        // Avoid using set() to avoid pointless mcrouter broadcasting
                        $this->setInterimValue( $key, $wrapped, $tempTTL );
                }
 
-               // Save the value unless a mutex-winning thread is already expected to do that
-               if ( $valueIsCacheable && ( !$useMutex || $lockAcquired ) ) {
+               // Save the value unless a lock-winning thread is already expected to do that
+               if ( $valueIsCacheable && !$isTombstone && ( !$useMutex || $lockAcquired ) ) {
                        $setOpts['lockTSE'] = $lockTSE;
                        $setOpts['staleTTL'] = $staleTTL;
                        // Use best known "since" timestamp if not provided