From d04a0d3afc836209e49989ae6f0cd7b056583440 Mon Sep 17 00:00:00 2001
From: Daimona Eaytoy <daimona.wiki@gmail.com>
Date: Sun, 10 Feb 2019 11:01:39 +0100
Subject: [PATCH] Store per-filter profiling in a single key

Instead of having three keys, one for total actions, one for time and
one for conditions. This has several benefits: first, it avoids race
conditions which could happen having different keys. Second, it's much
more performant. Third, the code is also clearer to understand,
and more uniform with the one for global stats.
Split from child patch by Dragons flight.

Bug: T53294
Depends-On: I1dc3be6da1cc9e03bc47e8f8c867089ad0100f6f
Change-Id: I8f47beb73cfc1b63c4b3c809fc6d65a1e66ee334
---
 includes/AbuseFilter.php       | 39 ++++++++++++++++++++--------------
 includes/AbuseFilterRunner.php | 32 ++++++++++++++++------------
 2 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/includes/AbuseFilter.php b/includes/AbuseFilter.php
index ee0add669..d4c799680 100644
--- a/includes/AbuseFilter.php
+++ b/includes/AbuseFilter.php
@@ -571,39 +571,35 @@ class AbuseFilter {
 	 */
 	private static function resetFilterProfile( $filter ) {
 		$stash = MediaWikiServices::getInstance()->getMainObjectStash();
-		$countKey = wfMemcKey( 'abusefilter', 'profile', $filter, 'count' );
-		$totalKey = wfMemcKey( 'abusefilter', 'profile', $filter, 'total' );
-		$condsKey = wfMemcKey( 'abusefilter', 'profile', $filter, 'conds' );
+		$profileKey = self::filterProfileKey( $filter );
 
-		$stash->delete( $countKey );
-		$stash->delete( $totalKey );
-		$stash->delete( $condsKey );
+		$stash->delete( $profileKey );
 	}
 
 	/**
+	 * Retrieve per-filter statistics.
+	 *
 	 * @param string $filter
 	 * @return array
 	 */
 	public static function getFilterProfile( $filter ) {
 		$stash = MediaWikiServices::getInstance()->getMainObjectStash();
-		$countKey = wfMemcKey( 'abusefilter', 'profile', $filter, 'count' );
-		$totalKey = wfMemcKey( 'abusefilter', 'profile', $filter, 'total' );
-		$condsKey = wfMemcKey( 'abusefilter', 'profile', $filter, 'conds' );
+		$profile = $stash->get( self::filterProfileKey( $filter ) );
 
-		$curCount = $stash->get( $countKey );
-		$curTotal = $stash->get( $totalKey );
-		$curConds = $stash->get( $condsKey );
-
-		if ( !$curCount ) {
+		if ( $profile !== false ) {
+			$curCount = $profile['count'];
+			$curTotalTime = $profile['total-time'];
+			$curTotalConds = $profile['total-cond'];
+		} else {
 			return [ 0, 0 ];
 		}
 
 		// 1000 ms in a sec
-		$timeProfile = ( $curTotal / $curCount ) * 1000;
+		$timeProfile = ( $curTotalTime / $curCount ) * 1000;
 		// Return in ms, rounded to 2dp
 		$timeProfile = round( $timeProfile, 2 );
 
-		$condProfile = ( $curConds / $curCount );
+		$condProfile = ( $curTotalConds / $curCount );
 		$condProfile = round( $condProfile, 0 );
 
 		return [ $timeProfile, $condProfile ];
@@ -1010,6 +1006,17 @@ class AbuseFilter {
 		return wfMemcKey( 'abusefilter', 'stats', 'overflow' );
 	}
 
+	/**
+	 * Get the memcache access key used to store per-filter profiling data.
+	 *
+	 * @param string|int $filter
+	 * @return string
+	 */
+	public static function filterProfileKey( $filter ) {
+		$cache = MediaWikiServices::getInstance()->getMainWANObjectCache();
+		return $cache->makeKey( 'abusefilter-profile', 'v1', $filter );
+	}
+
 	/**
 	 * @param string $group The filter's group (as defined in $wgAbuseFilterValidGroups)
 	 * @return string
diff --git a/includes/AbuseFilterRunner.php b/includes/AbuseFilterRunner.php
index 7c8166e75..6e9af193a 100644
--- a/includes/AbuseFilterRunner.php
+++ b/includes/AbuseFilterRunner.php
@@ -421,23 +421,27 @@ class AbuseFilterRunner {
 		// Defer updates to avoid massive (~1 second) edit time increases
 		DeferredUpdates::addCallableUpdate( function () use ( $filter, $time, $conds ) {
 			$stash = MediaWikiServices::getInstance()->getMainObjectStash();
-			$countKey = $stash->makeKey( 'abusefilter', 'profile', $filter, 'count' );
-			$totalKey = $stash->makeKey( 'abusefilter', 'profile', $filter, 'total' );
-			$condsKey = $stash->makeKey( 'abusefilter', 'profile', $filter, 'conds' );
+			$profileKey = AbuseFilter::filterProfileKey( $filter );
+			$profile = $stash->get( $profileKey );
 
-			$curCount = $stash->get( $countKey );
-			$curTotal = $stash->get( $totalKey );
-			$curConds = $stash->get( $condsKey );
-
-			if ( $curCount ) {
-				$stash->set( $condsKey, $curConds + $conds, 3600 );
-				$stash->set( $totalKey, $curTotal + $time, 3600 );
-				$stash->incr( $countKey );
+			if ( $profile !== false ) {
+				// Number of observed executions of this filter
+				$profile['count']++;
+				// Total time spent on this filter from all observed executions
+				$profile['total-time'] += $time;
+				// Total number of conditions for this filter from all executions
+				$profile['total-cond'] += $conds;
 			} else {
-				$stash->set( $countKey, 1, 3600 );
-				$stash->set( $totalKey, $time, 3600 );
-				$stash->set( $condsKey, $conds, 3600 );
+				$profile = [
+					'count' => 1,
+					'total-time' => $time,
+					'total-cond' => $conds
+				];
 			}
+			// Note: It is important that all key information be stored together in a single
+			// memcache entry to avoid race conditions where competing Apache instances
+			// partially overwrite the stats.
+			$stash->set( $profileKey, $profile, 3600 );
 		} );
 	}