jenkins-bot has submitted this change and it was merged.
Change subject: Try to avoid huge inserts in DatabaseMessageIndex
......................................................................
Try to avoid huge inserts in DatabaseMessageIndex
Replaced delete_all + insert with delete + replace.
Added locking so that only one rebuild is run at a time,
but only implemented for DatabaseMessageIndex. Also added
a flag to load old state from master when rebuilding.
Bug: T86385
Change-Id: I742884cf48eec807c623d3c7e249f2b254406e8f
---
M tests/phpunit/MessageIndexTest.php
M utils/MessageIndex.php
2 files changed, 243 insertions(+), 58 deletions(-)
Approvals:
Amire80: Looks good to me, approved
jenkins-bot: Verified
diff --git a/tests/phpunit/MessageIndexTest.php
b/tests/phpunit/MessageIndexTest.php
index 07c7359..5d9136f 100644
--- a/tests/phpunit/MessageIndexTest.php
+++ b/tests/phpunit/MessageIndexTest.php
@@ -20,6 +20,99 @@
) );
}
+ /**
+ * @dataProvider provideTestGetArrayDiff
+ */
+ public function testGetArrayDiff( $expected, $old, $new ) {
+ $actual = MessageIndex::getArrayDiff( $old, $new );
+ $this->assertEquals( $expected['keys'], $actual['keys'], 'key
diff' );
+ $this->assertEquals( $expected['values'], $actual['values'],
'value diff' );
+ }
+
+ public function provideTestGetArrayDiff() {
+ $tests = array();
+
+ // Addition
+ $old = array();
+ $new = array(
+ 'label' => 'carpet',
+ );
+ $expected = array(
+ 'keys' => array(
+ 'add' => array(
+ 'label' => array(
+ array(),
+ array( 'carpet' ),
+ ),
+ ),
+ 'del' => array(),
+ 'mod' => array(),
+ ),
+ 'values' => array( 'carpet' ),
+ );
+ $tests[] = array( $expected, $old, $new );
+
+ // Deletion
+ $old = array(
+ 'bath' => array( 'goal', 'morals', 'coronation' ),
+ );
+ $new = array();
+ $expected = array(
+ 'keys' => array(
+ 'add' => array(),
+ 'del' => array(
+ 'bath' => array(
+ array( 'goal', 'morals',
'coronation' ),
+ array(),
+ ),
+ ),
+ 'mod' => array(),
+ ),
+ 'values' => array( 'goal', 'morals', 'coronation' ),
+ );
+ $tests[] = array( $expected, $old, $new );
+
+ // No change
+ $old = $new = array(
+ 'label' => 'carpet',
+ 'salt' => array( 'morals' ),
+ 'bath' => array( 'goal', 'morals', 'coronation' ),
+ );
+ $expected = array(
+ 'keys' => array(
+ 'add' => array(),
+ 'del' => array(),
+ 'mod' => array(),
+ ),
+ 'values' => array(),
+ );
+ $tests[] = array( $expected, $old, $new );
+
+ // Modification
+ $old = array(
+ 'bath' => array( 'goal', 'morals', 'coronation' ),
+ );
+ $new = array(
+ 'bath' => array( 'goal', 'beliefs', 'coronation',
'showcase' ),
+ );
+ $expected = array(
+ 'keys' => array(
+ 'add' => array(),
+ 'del' => array(),
+ 'mod' => array(
+ 'bath' => array(
+ array( 'goal', 'morals',
'coronation' ),
+ array( 'goal', 'beliefs',
'coronation', 'showcase' ),
+ ),
+ ),
+ ),
+ 'values' => array( 'morals', 'beliefs', 'showcase' ),
+ );
+ $tests[] = array( $expected, $old, $new );
+
+ return $tests;
+ }
+
protected static function getTestData() {
static $data = null;
if ( $data === null ) {
@@ -35,7 +128,8 @@
public function testMessageIndexImplementation( $mi ) {
$data = self::getTestData();
/** @var
TestableDatabaseMessageIndex|TestableCDBMessageIndex|TestableSerializedMessageIndex
*/
- $mi->store( $data );
+ $diff = MessageIndex::getArrayDiff( array(), $data );
+ $mi->store( $data, $diff['keys'] );
$tests = array_rand( $data, 10 );
foreach ( $tests as $key ) {
@@ -79,8 +173,8 @@
class TestableDatabaseMessageIndex extends DatabaseMessageIndex {
// @codingStandardsIgnoreStart PHP CodeSniffer warns "Useless method
overriding
// detected", but store() and get() are protected in parent.
- public function store( array $a ) {
- parent::store( $a );
+ public function store( array $a, array $diff ) {
+ parent::store( $a, $diff );
}
public function get( $a ) {
@@ -91,8 +185,8 @@
class TestableCDBMessageIndex extends CDBMessageIndex {
// @codingStandardsIgnoreStart PHP CodeSniffer warns "Useless method
overriding
// detected", but store() and get() are protected in parent.
- public function store( array $a ) {
- parent::store( $a );
+ public function store( array $a, array $diff ) {
+ parent::store( $a, $diff );
}
public function get( $a ) {
@@ -103,8 +197,8 @@
class TestableSerializedMessageIndex extends SerializedMessageIndex {
// @codingStandardsIgnoreStart PHP CodeSniffer warns "Useless method
overriding
// detected", but store() and get() are protected in parent.
- public function store( array $a ) {
- parent::store( $a );
+ public function store( array $a, array $diff ) {
+ parent::store( $a, $diff );
}
public function get( $a ) {
@@ -115,8 +209,8 @@
class TestableHashMessageIndex extends HashMessageIndex {
// @codingStandardsIgnoreStart PHP CodeSniffer warns "Useless method
overriding
// detected", but store() and get() are protected in parent.
- public function store( array $a ) {
- parent::store( $a );
+ public function store( array $a, array $diff ) {
+ parent::store( $a, $diff );
}
public function get( $a ) {
diff --git a/utils/MessageIndex.php b/utils/MessageIndex.php
index b5505fe..09aa05e 100644
--- a/utils/MessageIndex.php
+++ b/utils/MessageIndex.php
@@ -91,9 +91,17 @@
}
/// @return array
- abstract public function retrieve();
+ abstract public function retrieve( $forRebuild = false );
- abstract protected function store( array $array );
+ abstract protected function store( array $array, array $diff );
+
+ protected function lock() {
+ return true;
+ }
+
+ protected function unlock() {
+ return true;
+ }
public function rebuild() {
static $recursion = 0;
@@ -109,8 +117,12 @@
$groups = MessageGroups::singleton()->getGroups();
+ if ( !$this->lock() ) {
+ throw new Exception( __CLASS__ . ': unable to acquire
lock' );
+ }
+
$new = $old = array();
- $old = $this->retrieve();
+ $old = $this->retrieve( 'rebuild' );
$postponed = array();
/**
@@ -134,56 +146,101 @@
$this->checkAndAdd( $new, $g, true );
}
- $this->store( $new );
- $this->clearMessageGroupStats( $old, $new );
+ $diff = self::getArrayDiff( $old, $new );
+ $this->store( $new, $diff['keys'] );
+ $this->unlock();
+ $this->clearMessageGroupStats( $diff );
+
$recursion--;
return $new;
}
/**
- * Purge message group stats when set of keys have changed.
+ * Compares two associative arrays.
+ *
+ * Values must be a string or list of strings. Returns an array of
added,
+ * deleted and modified keys as well as value changes (you can think
values
+ * as categories and keys as pages). Each of the keys ('add', 'del',
'mod'
+ * respectively) maps to an array whose keys are the changed keys of the
+ * original arrays and values are lists where first element contains the
+ * old value and the second element the new value.
+ *
+ * @code
+ * $a = [ 'a' => '1', 'b' => '2', 'c' => '3' ];
+ * $b = [ 'b' => '2', 'c' => [ '3', '2' ], 'd' => '4' ];
+ *
+ * self::getArrayDiff( $a, $b ) ) === [
+ * 'keys' => [
+ * 'add' => [ 'd' => [ [], [ '4' ] ] ],
+ * 'del' => [ 'a' => [ [ '1' ], [] ] ],
+ * 'mod' => [ 'c' => [ [ '3' ], [ '3', '2' ] ] ],
+ * ],
+ * 'values' => [ 2, 4, 1 ]
+ * ];
+ * @endcode
+ *
* @param array $old
* @param array $new
+ * @return array
*/
- protected function clearMessageGroupStats( array $old, array $new ) {
- $changes = array();
+ public static function getArrayDiff( array $old, array $new ) {
+ $values = array();
+ $record = function ( $groups ) use ( &$values ) {
+ foreach ( $groups as $group ) {
+ $values[$group] = true;
+ }
+ };
+
+ $keys = array(
+ 'add' => array(),
+ 'del' => array(),
+ 'mod' => array(),
+ );
foreach ( $new as $key => $groups ) {
- // Using != here on purpose to ignore order of items
if ( !isset( $old[$key] ) ) {
- $changes[$key] = array( array(), (array)$groups
);
+ $keys['add'][$key] = array( array(),
(array)$groups );
+ $record( (array)$groups );
+ // Using != here on purpose to ignore the order of items
} elseif ( $groups != $old[$key] ) {
- $changes[$key] = array( (array)$old[$key],
(array)$groups );
+ $keys['mod'][$key] = array( (array)$old[$key],
(array)$groups );
+ $record( array_diff( (array)$old[$key],
(array)$groups ) );
+ $record( array_diff( (array)$groups,
(array)$old[$key] ) );
}
}
foreach ( $old as $key => $groups ) {
if ( !isset( $new[$key] ) ) {
- $changes[$key] = array( (array)$groups, array()
);
+ $keys['del'][$key] = array( (array)$groups,
array() );
+ $record( (array)$groups, array() );
}
// We already checked for diffs above
}
- $changedGroups = array();
- foreach ( $changes as $data ) {
- foreach ( $data[0] as $group ) {
- $changedGroups[$group] = true;
- }
- foreach ( $data[1] as $group ) {
- $changedGroups[$group] = true;
- }
- }
+ return array(
+ 'keys' => $keys,
+ 'values' => array_keys( $values ),
+ );
+ }
- MessageGroupStats::clearGroup( array_keys( $changedGroups ) );
+ /**
+ * Purge stuff when set of keys have changed.
+ *
+ * @param array $diff
+ */
+ protected function clearMessageGroupStats( array $diff ) {
+ MessageGroupStats::clearGroup( $diff['values'] );
- foreach ( $changes as $key => $data ) {
- list( $ns, $pagename ) = explode( ':', $key, 2 );
- $title = Title::makeTitle( $ns, $pagename );
- $handle = new MessageHandle( $title );
- list ( $oldGroups, $newGroups ) = $data;
- Hooks::run( 'TranslateEventMessageMembershipChange',
- array( $handle, $oldGroups, $newGroups ) );
+ foreach ( $diff['keys'] as $keys ) {
+ foreach ( $keys as $key => $data ) {
+ list( $ns, $pagename ) = explode( ':', $key, 2
);
+ $title = Title::makeTitle( $ns, $pagename );
+ $handle = new MessageHandle( $title );
+ list ( $oldGroups, $newGroups ) = $data;
+ Hooks::run(
'TranslateEventMessageMembershipChange',
+ array( $handle, $oldGroups, $newGroups
) );
+ }
}
}
@@ -281,7 +338,7 @@
protected $filename = 'translate_messageindex.ser';
/** @return array */
- public function retrieve() {
+ public function retrieve( $forRebuild = false ) {
if ( $this->index !== null ) {
return $this->index;
}
@@ -296,7 +353,7 @@
return $this->index;
}
- protected function store( array $array ) {
+ protected function store( array $array, array $diff ) {
$file = TranslateUtils::cacheFile( $this->filename );
file_put_contents( $file, serialize( $array ) );
$this->index = $array;
@@ -324,13 +381,32 @@
*/
protected $index;
+ protected function lock() {
+ $db = wfGetDB( DB_MASTER );
+
+ // Any transaction should be flushed after getting the lock to
avoid
+ // stale pre-lock REPEATABLE-READ snapshot data.
+ $ok = $db->lock( 'translate-messageindex', __METHOD__, 30 );
+ if ( $ok ) {
+ $db->commit( __METHOD__, 'flush' );
+ }
+
+ return $ok;
+ }
+
+ protected function unlock() {
+ $db = wfGetDB( DB_MASTER );
+
+ return $db->unlock( 'translate-messageindex', __METHOD__ );
+ }
+
/** @return array */
- public function retrieve() {
- if ( $this->index !== null ) {
+ public function retrieve( $forRebuild = false ) {
+ if ( $this->index !== null && !$forRebuild ) {
return $this->index;
}
- $dbr = wfGetDB( DB_SLAVE );
+ $dbr = wfGetDB( $forRebuild ? DB_MASTER : DB_SLAVE );
$res = $dbr->select( 'translate_messageindex', '*', array(),
__METHOD__ );
$this->index = array();
foreach ( $res as $row ) {
@@ -358,18 +434,33 @@
return $value;
}
- protected function store( array $array ) {
- $dbw = wfGetDB( DB_MASTER );
- $rows = array();
+ protected function store( array $array, array $diff ) {
+ $updates = array();
- foreach ( $array as $key => $value ) {
- $value = $this->serialize( $value );
- $rows[] = array( 'tmi_key' => $key, 'tmi_value' =>
$value );
+ foreach ( array( $diff['add'], $diff['mod'] ) as $changes ) {
+ foreach ( $changes as $key => $data ) {
+ list( $old, $new ) = $data;
+ $updates[] = array(
+ 'tmi_key' => $key,
+ 'tmi_value' => $this->serialize( $new ),
+ );
+ }
}
+ $index = array( 'tmi_key' );
+ $deletions = array_keys( $diff['del'] );
+
+ $dbw = wfGetDB( DB_MASTER );
$dbw->startAtomic( __METHOD__ );
- $dbw->delete( 'translate_messageindex', '*', __METHOD__ );
- $dbw->insert( 'translate_messageindex', $rows, __METHOD__ );
+
+ if ( $updates !== array() ) {
+ $dbw->replace( 'translate_messageindex', array( $index
), $updates, __METHOD__ );
+ }
+
+ if ( $deletions !== array() ) {
+ $dbw->delete( 'translate_messageindex', array(
'tmi_key' => $deletions ), __METHOD__ );
+ }
+
$dbw->endAtomic( __METHOD__ );
$this->index = $array;
@@ -398,7 +489,7 @@
}
/** @return array */
- public function retrieve() {
+ public function retrieve( $forRebuild = false ) {
if ( $this->index !== null ) {
return $this->index;
}
@@ -414,7 +505,7 @@
return $this->index;
}
- protected function store( array $array ) {
+ protected function store( array $array, array $diff ) {
$key = wfMemckey( $this->key );
$this->cache->set( $key, $array );
@@ -452,7 +543,7 @@
protected $filename = 'translate_messageindex.cdb';
/** @return array */
- public function retrieve() {
+ public function retrieve( $forRebuild = false ) {
$reader = $this->getReader();
// This must be below the line above, which may fill the index
if ( $this->index !== null ) {
@@ -489,7 +580,7 @@
return $value;
}
- protected function store( array $array ) {
+ protected function store( array $array, array $diff ) {
$this->reader = null;
$file = TranslateUtils::cacheFile( $this->filename );
@@ -538,7 +629,7 @@
protected $index = array();
/** @return array */
- public function retrieve() {
+ public function retrieve( $forRebuild = false ) {
return $this->index;
}
@@ -555,10 +646,10 @@
}
}
- protected function store( array $array ) {
+ protected function store( array $array, array $diff ) {
$this->index = $array;
}
- protected function clearMessageGroupStats( array $old, array $new ) {
+ protected function clearMessageGroupStats( array $diff ) {
}
}
--
To view, visit https://gerrit.wikimedia.org/r/249436
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I742884cf48eec807c623d3c7e249f2b254406e8f
Gerrit-PatchSet: 9
Gerrit-Project: mediawiki/extensions/Translate
Gerrit-Branch: master
Gerrit-Owner: Nikerabbit <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Amire80 <[email protected]>
Gerrit-Reviewer: Nikerabbit <[email protected]>
Gerrit-Reviewer: Ori.livneh <[email protected]>
Gerrit-Reviewer: Santhosh <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
