Move user_editcount updates to a mergeable deferred update

This should reduce excess contention and lock timeouts.
Previously, it used a pre-commit hook which ran just before the
end of the DB transaction round.

Also removed unused User::incEditCountImmediate() method.

Bug: T202715
Depends-on: I6d239a5ea286afb10d9e317b2ee1436de60f7e4f
Depends-on: I0ad3d17107efc7b0e59f1dd54d5733cd1572a2b7
Change-Id: I0d6d7ddd91bbb21995142808248d162e05696d47

autoload.php

@@ -1542,6 +1542,7 @@ $wgAutoloadLocalClasses = [
 	'UserBlockedError' => __DIR__ . '/includes/exception/UserBlockedError.php',
 	'UserCache' => __DIR__ . '/includes/cache/UserCache.php',
 	'UserDupes' => __DIR__ . '/maintenance/userDupes.inc',
+	'UserEditCountUpdate' => __DIR__ . '/includes/deferred/UserEditCountUpdate.php',
 	'UserGroupExpiryJob' => __DIR__ . '/includes/jobqueue/jobs/UserGroupExpiryJob.php',
 	'UserGroupMembership' => __DIR__ . '/includes/user/UserGroupMembership.php',
 	'UserMailer' => __DIR__ . '/includes/mail/UserMailer.php',

includes/deferred/MergeableUpdate.php

@@ -1,8 +1,17 @@
 <?php
 
 /**
- * Interface that deferrable updates can implement. DeferredUpdates uses this to merge
- * all pending updates of PHP class into a single update by calling merge().
+ * Interface that deferrable updates can implement to signal that updates can be combined.
+ *
+ * DeferredUpdates uses this to merge all pending updates of PHP class into a single update
+ * by calling merge(). Note that upon merge(), the combined update goes to the back of the FIFO
+ * queue so that such updates occur after related non-mergeable deferred updates. For example,
+ * suppose updates that purge URLs can be merged, and the calling pattern is:
+ *   - a) DeferredUpdates::addUpdate( $purgeCdnUrlsA );
+ *   - b) DeferredUpdates::addUpdate( $deleteContentUrlsB );
+ *   - c) DeferredUpdates::addUpdate( $purgeCdnUrlsB )
+ *
+ * The purges for urls A and B will all happen after the $deleteContentUrlsB update.
  *
  * @since 1.27
  */

includes/deferred/UserEditCountUpdate.php

@@ -0,0 +1,113 @@
+<?php
+/**
+ * User edit count incrementing.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+ * http://www.gnu.org/copyleft/gpl.html
+ *
+ * @file
+ */
+
+use Wikimedia\Assert\Assert;
+use MediaWiki\MediaWikiServices;
+
+/**
+ * Handles increment the edit count for a given set of users
+ */
+class UserEditCountUpdate implements DeferrableUpdate, MergeableUpdate {
+	/** @var array[] Map of (user ID => ('increment': int, 'instances': User[])) */
+	private $infoByUser;
+
+	/**
+	 * @param User $user
+	 * @param int $increment
+	 */
+	public function __construct( User $user, $increment ) {
+		if ( !$user->getId() ) {
+			throw new RuntimeException( "Got user ID of zero" );
+		}
+		$this->infoByUser = [
+			$user->getId() => [ 'increment' => $increment, 'instances' => [ $user ] ]
+		];
+	}
+
+	public function merge( MergeableUpdate $update ) {
+		/** @var UserEditCountUpdate $update */
+		Assert::parameterType( __CLASS__, $update, '$update' );
+
+		foreach ( $update->infoByUser as $userId => $info ) {
+			if ( !isset( $this->infoByUser[$userId] ) ) {
+				$this->infoByUser[$userId] = [ 'increment' => 0, 'instances' => [] ];
+			}
+			// Merge the increment amount
+			$this->infoByUser[$userId]['increment'] += $info['increment'];
+			// Merge the list of User instances to update in doUpdate()
+			foreach ( $info['instances'] as $user ) {
+				if ( !in_array( $user, $this->infoByUser[$userId]['instances'], true ) ) {
+					$this->infoByUser[$userId]['instances'][] = $user;
+				}
+			}
+		}
+	}
+
+	/**
+	 * Purges the list of URLs passed to the constructor.
+	 */
+	public function doUpdate() {
+		foreach ( $this->infoByUser as $userId => $info ) {
+			$lb = MediaWikiServices::getInstance()->getDBLoadBalancer();
+			$dbw = $lb->getConnection( DB_MASTER );
+
+			$dbw->startAtomic( __METHOD__ ); // define minimum row lock duration
+			$dbw->update(
+				'user',
+				[ 'user_editcount=user_editcount+' . (int)$info['increment'] ],
+				[ 'user_id' => $userId, 'user_editcount IS NOT NULL' ],
+				__METHOD__
+			);
+			/** @var User[] $affectedInstances */
+			$affectedInstances = $info['instances'];
+			// Lazy initialization check...
+			if ( $dbw->affectedRows() == 0 ) {
+				// No rows will be "affected" if user_editcount is NULL.
+				// Get the generic "replica" connection to see if it actually uses the master.
+				$dbr = $lb->getConnection( DB_REPLICA );
+				if ( $dbr !== $dbw ) {
+					// This method runs after the new revisions were committed.
+					// Wait for the replica to catch up so they will all be counted.
+					$dbr->flushSnapshot( __METHOD__ );
+					$lb->safeWaitForMasterPos( $dbr );
+				}
+				$affectedInstances[0]->initEditCountInternal();
+			}
+			$newCount = (int)$dbw->selectField(
+				'user',
+				[ 'user_editcount' ],
+				[ 'user_id' => $userId ],
+				__METHOD__
+			);
+			$dbw->endAtomic( __METHOD__ );
+
+			// Update the edit count in the instance caches. This is mostly useful
+			// for maintenance scripts, where deferred updates might run immediately
+			// and user instances might be reused for a long time.
+			foreach ( $affectedInstances as $affectedInstance ) {
+				$affectedInstance->setEditCountInternal( $newCount );
+			}
+			// Clear the edit count in user cache too
+			$affectedInstances[0]->invalidateCache();
+		}
+	}
+}

includes/user/User.php

@@ -3701,7 +3701,7 @@ class User implements IDBAccessObject, UserIdentity {
 
 			if ( $count === null ) {
 				// it has not been initialized. do so.
-				$count = $this->initEditCount();
+				$count = $this->initEditCountInternal();
 			}
 			$this->mEditCount = $count;
 		}
@@ -5316,73 +5316,37 @@ class User implements IDBAccessObject, UserIdentity {
 	}
 
 	/**
-	 * Deferred version of incEditCountImmediate()
-	 *
-	 * This function, rather than incEditCountImmediate(), should be used for
-	 * most cases as it avoids potential deadlocks caused by concurrent editing.
+	 * Schedule a deferred update to update the user's edit count
 	 */
 	public function incEditCount() {
-		wfGetDB( DB_MASTER )->onTransactionPreCommitOrIdle(
-			function () {
-				$this->incEditCountImmediate();
-			},
-			__METHOD__
+		if ( $this->isAnon() ) {
+			return; // sanity
+		}
+
+		DeferredUpdates::addUpdate(
+			new UserEditCountUpdate( $this, 1 ),
+			DeferredUpdates::POSTSEND
 		);
 	}
 
 	/**
-	 * Increment the user's edit-count field.
-	 * Will have no effect for anonymous users.
-	 * @since 1.26
+	 * This method should not be called outside User/UserEditCountUpdate
+	 *
+	 * @param int $count
 	 */
-	public function incEditCountImmediate() {
-		if ( $this->isAnon() ) {
-			return;
-		}
-
-		$dbw = wfGetDB( DB_MASTER );
-		// No rows will be "affected" if user_editcount is NULL
-		$dbw->update(
-			'user',
-			[ 'user_editcount=user_editcount+1' ],
-			[ 'user_id' => $this->getId(), 'user_editcount IS NOT NULL' ],
-			__METHOD__
-		);
-		// Lazy initialization check...
-		if ( $dbw->affectedRows() == 0 ) {
-			// Now here's a goddamn hack...
-			$dbr = wfGetDB( DB_REPLICA );
-			if ( $dbr !== $dbw ) {
-				// If we actually have a replica DB server, the count is
-				// at least one behind because the current transaction
-				// has not been committed and replicated.
-				$this->mEditCount = $this->initEditCount( 1 );
-			} else {
-				// But if DB_REPLICA is selecting the master, then the
-				// count we just read includes the revision that was
-				// just added in the working transaction.
-				$this->mEditCount = $this->initEditCount();
-			}
-		} else {
-			if ( $this->mEditCount === null ) {
-				$this->getEditCount();
-				$dbr = wfGetDB( DB_REPLICA );
-				$this->mEditCount += ( $dbr !== $dbw ) ? 1 : 0;
-			} else {
-				$this->mEditCount++;
-			}
-		}
-		// Edit count in user cache too
-		$this->invalidateCache();
+	public function setEditCountInternal( $count ) {
+		$this->mEditCount = $count;
 	}
 
 	/**
 	 * Initialize user_editcount from data out of the revision table
 	 *
+	 * This method should not be called outside User/UserEditCountUpdate
+	 *
 	 * @param int $add Edits to add to the count from the revision table
 	 * @return int Number of edits
 	 */
-	protected function initEditCount( $add = 0 ) {
+	public function initEditCountInternal( $add = 0 ) {
 		// Pull from a replica DB to be less cruel to servers
 		// Accuracy isn't the point anyway here
 		$dbr = wfGetDB( DB_REPLICA );

tests/phpunit/includes/api/ApiStashEditTest.php

@@ -364,6 +364,7 @@ class ApiStashEditTest extends ApiTestCase {
 		// Now let's also increment our editcount
 		$this->editPage( ucfirst( __FUNCTION__ ), '' );
 
+		$user->clearInstanceCache();
 		$this->assertFalse( $this->doCheckCache( $user ),
 			"Cache should be invalidated when it's old and the user has an intervening edit" );
 	}

tests/phpunit/includes/user/UserTest.php

@@ -263,6 +263,7 @@ class UserTest extends MediaWikiTestCase {
 
 		// increase the edit count
 		$user->incEditCount();
+		$user->clearInstanceCache();
 
 		$this->assertEquals(
 			4,