Merge pull request #765 from wmde/release720

Release 7.2.0
wmde · Oct 23, 2017 · b54117c · b54117c
2 parents d65b89c + a3f3d14
commit b54117c
Show file tree

Hide file tree

Showing 6 changed files with 65 additions and 63 deletions.
diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md
@@ -1,5 +1,11 @@
 # Wikibase DataModel release notes
 
+## Version 7.2.0 (2017-10-23)
+
+* Performance optimizations on methods critical for dump generation:
+	* `DispatchingEntityIdParser::parse`
+	* `SnakList::orderByProperty`
+
 ## Version 7.1.0 (2017-09-01)
 
 * Changed `EntityIdValue::getArrayValue` to allow it handle foreign entity IDs and entity IDs that

diff --git a/WikibaseDataModel.php b/WikibaseDataModel.php
@@ -11,4 +11,4 @@
 	return;
 }
 
-define( 'WIKIBASE_DATAMODEL_VERSION', '7.1.0' );
+define( 'WIKIBASE_DATAMODEL_VERSION', '7.2.0' );
diff --git a/composer.json b/composer.json
@@ -52,7 +52,7 @@
 	},
 	"extra": {
 		"branch-alias": {
-			"dev-master": "7.1.x-dev"
+			"dev-master": "7.2.x-dev"
 		}
 	},
 	"scripts": {

diff --git a/src/Entity/DispatchingEntityIdParser.php b/src/Entity/DispatchingEntityIdParser.php
@@ -37,20 +37,19 @@ public function __construct( array $idBuilders ) {
 	 * @return EntityId
 	 */
 	public function parse( $idSerialization ) {
-		$this->assertIdIsString( $idSerialization );
-
-		if ( empty( $this->idBuilders ) ) {
+		if ( $this->idBuilders === [] ) {
 			throw new EntityIdParsingException( 'No id builders are configured' );
 		}
 
+		try {
+			list( , , $localId ) = EntityId::splitSerialization( $idSerialization );
+		} catch ( InvalidArgumentException $ex ) {
+			// EntityId::splitSerialization performs some sanity checks which
+			// might result in an exception. Should this happen, re-throw the exception message
+			throw new EntityIdParsingException( $ex->getMessage(), 0, $ex );
+		}
+
 		foreach ( $this->idBuilders as $idPattern => $idBuilder ) {
-			try {
-				list( , , $localId ) = EntityId::splitSerialization( $idSerialization );
-			} catch ( InvalidArgumentException $ex ) {
-				// EntityId::splitSerialization performs some sanity checks which
-				// might result in an exception. Should this happen, re-throw the exception message
-				throw new EntityIdParsingException( $ex->getMessage(), 0, $ex );
-			}
 			if ( preg_match( $idPattern, $localId ) ) {
 				return $this->buildId( $idBuilder, $idSerialization );
 			}
@@ -61,21 +60,6 @@ public function parse( $idSerialization ) {
 		);
 	}
 
-	/**
-	 * @param string $idSerialization
-	 *
-	 * @throws EntityIdParsingException
-	 */
-	private function assertIdIsString( $idSerialization ) {
-		if ( !is_string( $idSerialization ) ) {
-			throw new EntityIdParsingException(
-				'$idSerialization must be a string, got ' . ( is_object( $idSerialization )
-					? get_class( $idSerialization )
-					: getType( $idSerialization ) )
-			);
-		}
-	}
-
 	/**
 	 * @param callable $idBuilder
 	 * @param string $idSerialization

diff --git a/src/Snak/SnakList.php b/src/Snak/SnakList.php
@@ -144,53 +144,33 @@ public function getHash() {
 	}
 
 	/**
-	 * Orders the snaks in the list grouping them by property.
+	 * Groups snaks by property, and optionally orders them.
 	 *
-	 * @param string[] $order List of serliazed property ids to order by.
+	 * @param string[] $order List of property ID strings to order by. Snaks with other properties
+	 *  will also be grouped, but put at the end, in the order each property appeared first in the
+	 *  original list.
 	 *
 	 * @since 0.5
 	 */
 	public function orderByProperty( array $order = [] ) {
-		$snaksByProperty = $this->getSnaksByProperty();
-		$orderedProperties = array_unique( array_merge( $order, array_keys( $snaksByProperty ) ) );
-
-		foreach ( $orderedProperties as $property ) {
-			if ( array_key_exists( $property, $snaksByProperty ) ) {
-				$snaks = $snaksByProperty[$property];
-				$this->moveSnaksToBottom( $snaks );
-			}
+		$byProperty = array_fill_keys( $order, [] );
+
+		/** @var Snak $snak */
+		foreach ( $this as $snak ) {
+			$byProperty[$snak->getPropertyId()->getSerialization()][] = $snak;
 		}
-	}
 
-	/**
-	 * @param Snak[] $snaks to remove and re add
-	 */
-	private function moveSnaksToBottom( array $snaks ) {
-		foreach ( $snaks as $snak ) {
-			$this->removeSnak( $snak );
-			$this->addSnak( $snak );
+		$ordered = [];
+		foreach ( $byProperty as $snaks ) {
+			$ordered = array_merge( $ordered, $snaks );
 		}
-	}
 
-	/**
-	 * Gets the snaks in the current object in an array
-	 * grouped by property id
-	 *
-	 * @return array[]
-	 */
-	private function getSnaksByProperty() {
-		$snaksByProperty = [];
+		$this->exchangeArray( $ordered );
 
-		foreach ( $this as $snak ) {
-			/** @var Snak $snak */
-			$propertyId = $snak->getPropertyId()->getSerialization();
-			if ( !isset( $snaksByProperty[$propertyId] ) ) {
-				$snaksByProperty[$propertyId] = [];
-			}
-			$snaksByProperty[$propertyId][] = $snak;
+		$index = 0;
+		foreach ( $ordered as $snak ) {
+			$this->offsetHashes[$snak->getHash()] = $index++;
 		}
-
-		return $snaksByProperty;
 	}
 
 }
diff --git a/tests/unit/Snak/SnakListTest.php b/tests/unit/Snak/SnakListTest.php
@@ -175,6 +175,7 @@ public function orderByPropertyProvider() {
 		$id1 = new PropertyId( 'P1' );
 		$id2 = new PropertyId( 'P2' );
 		$id3 = new PropertyId( 'P3' );
+		$id4 = new PropertyId( 'P4' );
 
 		/**
 		 * List of test data containing snaks to initialize SnakList objects. The first list of
@@ -248,6 +249,27 @@ public function orderByPropertyProvider() {
 				],
 				[ 'P1' ]
 			],
+			'Multiple IDs in order' => [
+				[
+					new PropertyValueSnak( $id1, new StringValue( 'a' ) ),
+					new PropertyValueSnak( $id2, new StringValue( 'b' ) ),
+					new PropertyValueSnak( $id1, new StringValue( 'c' ) ),
+					new PropertyValueSnak( $id3, new StringValue( 'd' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'e' ) ),
+					new PropertyValueSnak( $id2, new StringValue( 'f' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'g' ) ),
+				],
+				[
+					new PropertyValueSnak( $id2, new StringValue( 'b' ) ),
+					new PropertyValueSnak( $id2, new StringValue( 'f' ) ),
+					new PropertyValueSnak( $id3, new StringValue( 'd' ) ),
+					new PropertyValueSnak( $id1, new StringValue( 'a' ) ),
+					new PropertyValueSnak( $id1, new StringValue( 'c' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'e' ) ),
+					new PropertyValueSnak( $id4, new StringValue( 'g' ) ),
+				],
+				[ 'P2', 'P3', 'P1' ]
+			],
 		];
 
 		$arguments = [];
@@ -282,6 +304,16 @@ public function testOrderByProperty( SnakList $snakList, SnakList $expected, arr
 		} else {
 			$this->assertNotSame( $initialSnakList->getHash(), $snakList->getHash() );
 		}
+
+		/** @var Snak $snak */
+		foreach ( $snakList as $snak ) {
+			$hash = $snak->getHash();
+			$this->assertSame(
+				$hash,
+				$snakList->getSnak( $hash )->getHash(),
+				'Reordering must not mess up the lists internal state'
+			);
+		}
 	}
 
 	public function testComparableInterface() {