Nikerabbit has uploaded a new change for review.
https://gerrit.wikimedia.org/r/249436
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, 202 insertions(+), 51 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Translate
refs/changes/36/249436/1
diff --git a/tests/phpunit/MessageIndexTest.php
b/tests/phpunit/MessageIndexTest.php
index 07c7359..fa5b6b5 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 ) {
diff --git a/utils/MessageIndex.php b/utils/MessageIndex.php
index b5505fe..03f89cc 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,79 @@
$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.
- * @param array $old
- * @param array $new
+ * Compares a diff of two associative arrays. Returns a list of added,
deleted and modified
+ * keys as well as value changes (think values as a groups having keys
as a member).
+ *
+ * @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 +316,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 +331,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 +359,21 @@
*/
protected $index;
+ protected function lock() {
+ return wfGetDB( DB_MASTER )->lock( 'translate-messageindex',
__METHOD__, 30 );
+ }
+
+ protected function unlock() {
+ return wfGetDB( DB_MASTER )->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 +401,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' => $delConds ), __METHOD__ );
+ }
+
$dbw->endAtomic( __METHOD__ );
$this->index = $array;
@@ -398,7 +456,7 @@
}
/** @return array */
- public function retrieve() {
+ public function retrieve( $forRebuild = false ) {
if ( $this->index !== null ) {
return $this->index;
}
@@ -414,7 +472,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 +510,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 +547,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 +596,7 @@
protected $index = array();
/** @return array */
- public function retrieve() {
+ public function retrieve( $forRebuild = false ) {
return $this->index;
}
@@ -555,10 +613,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: newchange
Gerrit-Change-Id: I742884cf48eec807c623d3c7e249f2b254406e8f
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Translate
Gerrit-Branch: master
Gerrit-Owner: Nikerabbit <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
