Merge pull request #124 from SemanticMediaWiki/audit/perf-hot-path

Simplify Lua bridge hot path

Author: paladox

src/LuaAskResultProcessor.php

@@ -22,35 +22,35 @@
 class LuaAskResultProcessor {
 
 	/**
-	 * Holds all possible representations of "true" in this smw instance
-	 *
-	 * @var array
+	 * Cached representations of "true" — shared across all processor instances
+	 * within a request because the message bundle is request-stable.
 	 */
-	private $msgTrue;
+	private static ?array $msgTrue = null;
 
 	/**
-	 * This counter serves as fallback, if no label for printout is specified
-	 *
-	 * @var int
+	 * Fallback index for printouts without a label.
 	 */
-	private $numericIndex;
+	private int $numericIndex;
 
-	/**
-	 * Holds the query result for this object
-	 *
-	 * @var QueryResult
-	 */
-	private $queryResult;
+	private QueryResult $queryResult;
+
+	private readonly Linker $linker;
 
 	/**
-	 * LuaAskResultProcessor constructor.
+	 * Memoised PrintRequest labels, keyed by spl_object_id.
+	 * Per-processor (i.e. per-ask-call) scope.
 	 *
-	 * @param QueryResult $queryResult
+	 * @var array<int, int|string>
 	 */
+	private array $printRequestLabels = [];
+
 	public function __construct( QueryResult $queryResult ) {
 		$this->queryResult = $queryResult;
-		$this->msgTrue = explode( ',', wfMessage( 'smw_true_words' )->text() . ',true,t,yes,y' );
+		if ( self::$msgTrue === null ) {
+			self::$msgTrue = explode( ',', wfMessage( 'smw_true_words' )->text() . ',true,t,yes,y' );
+		}
 		$this->numericIndex = 1;
+		$this->linker = new Linker();
 	}
 
 	/**
@@ -138,11 +138,15 @@ public function getDataFromResultArray( ResultArray $resultArray ) {
 	 * @return int|string
 	 */
 	public function getKeyFromPrintRequest( PrintRequest $printRequest ) {
-		if ( $printRequest->getLabel() !== '' ) {
-			return $printRequest->getText( SMW_OUTPUT_WIKI );
+		$id = spl_object_id( $printRequest );
+		if ( isset( $this->printRequestLabels[$id] ) ) {
+			return $this->printRequestLabels[$id];
 		}
-
-		return $this->getNumericIndex();
+		$label = $printRequest->getLabel() !== ''
+			? $printRequest->getText( SMW_OUTPUT_WIKI )
+			: $this->getNumericIndex();
+		$this->printRequestLabels[$id] = $label;
+		return $label;
 	}
 
 	/**
@@ -157,7 +161,7 @@ public function getValueFromDataValue( DataValue $dataValue ) {
 		switch ( $dataValue->getTypeID() ) {
 			case '_boo':
 				// boolean value found, convert it
-				$value = in_array( strtolower( $dataValue->getWikiValue() ?? 'null' ), $this->msgTrue );
+				$value = in_array( strtolower( $dataValue->getWikiValue() ?? 'null' ), self::$msgTrue );
 				break;
 			case '_num':
 				// number value found
@@ -167,7 +171,7 @@ public function getValueFromDataValue( DataValue $dataValue ) {
 				break;
 			default:
 				# FIXME ignores parameter link=none|subject
-				$value = $dataValue->getShortText( SMW_OUTPUT_WIKI, new Linker() );
+				$value = $dataValue->getShortText( SMW_OUTPUT_WIKI, $this->linker );
 		}
 
 		return $value;

src/ScribuntoLuaLibrary.php

@@ -130,7 +130,7 @@ public function getQueryResult( $arguments = null ) {
 		if ( !empty( $result["results"] ) ) {
 			// as of now, "results" has page names as keys. lua is not very good, keeping non-number keys in order
 			// so replace string keys with the corresponding number, starting with 0.
-			$result["results"] = array_combine( range( 0, count( $result["results"] ) - 1 ), array_values( $result["results"] ) );
+			$result["results"] = array_values( $result["results"] );
 		}
 
 		return [ $this->convertArrayToLuaTable( $result ) ];
@@ -254,21 +254,26 @@ public function subobject( $arguments, $subobjectId = null ) {
 	}
 
 	/**
-	 * This takes an array and converts it so, that the result is a viable lua table.
-	 * I.e. the resulting table has its numerical indices start with 1
-	 * If `$ar` is not an array, it is simply returned
-	 * @param mixed $ar
-	 * @return mixed array
+	 * Converts an array into a shape suitable for a Lua table: integer keys shift
+	 * by +1 (Lua arrays are 1-indexed), string keys are left untouched. Recurses
+	 * into nested arrays. Non-array inputs are returned unchanged.
+	 *
+	 * Precondition: integer-keyed inputs are expected to use contiguous 0-based
+	 * indices (as produced by `array_values()` or `QueryResultSerializer` output).
+	 * Sparse integer keys are shifted by +1 here rather than re-indexed to a
+	 * contiguous sequence — diverging from the pre-7.0 `array_unshift` + `unset`
+	 * implementation. No caller in this codebase feeds sparse keys.
 	 */
 	private function convertArrayToLuaTable( $ar ) {
-		if ( is_array( $ar ) ) {
-			foreach ( $ar as $key => $value ) {
-				$ar[$key] = $this->convertArrayToLuaTable( $value );
-			}
-			array_unshift( $ar, '' );
-			unset( $ar[0] );
+		if ( !is_array( $ar ) ) {
+			return $ar;
+		}
+		$result = [];
+		foreach ( $ar as $key => $value ) {
+			$newKey = is_int( $key ) ? $key + 1 : $key;
+			$result[$newKey] = $this->convertArrayToLuaTable( $value );
 		}
-		return $ar;
+		return $result;
 	}
 
 	/**
@@ -307,7 +312,7 @@ private function doPostProcessParserFunctionCallResult( $parserFunctionResult )
 	 * @return LibraryFactory
 	 */
 	private function getLibraryFactory(): LibraryFactory {
-		if ( !empty( $this->libraryFactory ) ) {
+		if ( isset( $this->libraryFactory ) ) {
 			return $this->libraryFactory;
 		}
 

tests/phpunit/Unit/ConvertArrayToLuaTableTest.php

@@ -0,0 +1,75 @@
+<?php
+
+namespace SMW\Scribunto\Tests\Unit;
+
+use PHPUnit\Framework\TestCase;
+use ReflectionClass;
+use ReflectionMethod;
+use SMW\Scribunto\ScribuntoLuaLibrary;
+
+/**
+ * Characterisation test pinning down the current behaviour of the private
+ * {@see ScribuntoLuaLibrary::convertArrayToLuaTable} helper before it is
+ * refactored. The test invokes the private method via reflection so the
+ * production class needs no visibility changes.
+ *
+ * @covers \SMW\Scribunto\ScribuntoLuaLibrary::convertArrayToLuaTable
+ * @group semantic-scribunto
+ *
+ * @license GPL-2.0-or-later
+ * @since 7.0.0
+ */
+class ConvertArrayToLuaTableTest extends TestCase {
+
+	private function invoke( mixed $input ): mixed {
+		$instance = ( new ReflectionClass( ScribuntoLuaLibrary::class ) )
+			->newInstanceWithoutConstructor();
+		$method = new ReflectionMethod( ScribuntoLuaLibrary::class, 'convertArrayToLuaTable' );
+		$method->setAccessible( true );
+		return $method->invoke( $instance, $input );
+	}
+
+	public function testNonArrayPassthrough(): void {
+		$this->assertSame( 'hello', $this->invoke( 'hello' ) );
+		$this->assertSame( 42, $this->invoke( 42 ) );
+		$this->assertNull( $this->invoke( null ) );
+	}
+
+	public function testEmptyArray(): void {
+		$this->assertSame( [], $this->invoke( [] ) );
+	}
+
+	public function testZeroIndexedListShiftsToOneIndexed(): void {
+		$this->assertSame(
+			[ 1 => 'a', 2 => 'b', 3 => 'c' ],
+			$this->invoke( [ 'a', 'b', 'c' ] )
+		);
+	}
+
+	public function testAssociativeKeysUnchanged(): void {
+		$this->assertSame(
+			[ 'foo' => 'a', 'bar' => 'b' ],
+			$this->invoke( [ 'foo' => 'a', 'bar' => 'b' ] )
+		);
+	}
+
+	public function testMixedKeys(): void {
+		// Integer keys shift by 1; string keys are left untouched by array_unshift().
+		$this->assertSame(
+			[ 1 => 'a', 'foo' => 'b', 2 => 'c' ],
+			$this->invoke( [ 0 => 'a', 'foo' => 'b', 1 => 'c' ] )
+		);
+	}
+
+	public function testNested(): void {
+		$expected = [ 1 => [ 1 => 'inner-a', 2 => 'inner-b' ] ];
+		$this->assertSame( $expected, $this->invoke( [ [ 'inner-a', 'inner-b' ] ] ) );
+	}
+
+	public function testDeeplyNested(): void {
+		$input = [ [ [ 'leaf' ] ] ];
+		$expected = [ 1 => [ 1 => [ 1 => 'leaf' ] ] ];
+		$this->assertSame( $expected, $this->invoke( $input ) );
+	}
+
+}