From 7ff0ae19d65e3b3029246b51de0e361cee6a1a01 Mon Sep 17 00:00:00 2001
From: "Dylan K. Taylor" <dktapps@pmmp.io>
Date: Wed, 13 Mar 2024 18:19:51 +0000
Subject: [PATCH] BlockStateUpgrader: a simple yet hard-to-explain optimization
 Prior to this commit, upgrade schemas would be applied to blockstates with
 the same version, as there wasn't any way to know if they should be applied
 or not given Mojang's tendency to forget to bump the version. However, it
 occurred to me that we only need to do this if there are multiple schemas
 present for the same version ID, which is rarely the case. This allows
 skipping costly logic for blockstates on the newest version (the common
 case), reducing the time taken to process the blockstate for upgrading by
 over 30%. Overall, this translates into less than 10% real performance
 improvement for chunk loading, but it's still a worthwhile improvement.

---
 .../block/upgrade/BlockStateUpgrader.php      | 76 ++++++++++++-------
 1 file changed, 47 insertions(+), 29 deletions(-)

diff --git a/src/data/bedrock/block/upgrade/BlockStateUpgrader.php b/src/data/bedrock/block/upgrade/BlockStateUpgrader.php
index 501ca750a..f4a5b6e93 100644
--- a/src/data/bedrock/block/upgrade/BlockStateUpgrader.php
+++ b/src/data/bedrock/block/upgrade/BlockStateUpgrader.php
@@ -35,7 +35,10 @@ use function sprintf;
 use const SORT_NUMERIC;
 
 final class BlockStateUpgrader{
-	/** @var BlockStateUpgradeSchema[] */
+	/**
+	 * @var BlockStateUpgradeSchema[][] versionId => [schemaId => schema]
+	 * @phpstan-var array<int, array<int, BlockStateUpgradeSchema>>
+	 */
 	private array $upgradeSchemas = [];
 
 	private int $outputVersion = 0;
@@ -52,46 +55,37 @@ final class BlockStateUpgrader{
 
 	public function addSchema(BlockStateUpgradeSchema $schema) : void{
 		$schemaId = $schema->getSchemaId();
-		if(isset($this->upgradeSchemas[$schemaId])){
-			throw new \InvalidArgumentException("Cannot add two schemas with the same schema ID");
+		$versionId = $schema->getVersionId();
+		if(isset($this->upgradeSchemas[$versionId][$schemaId])){
+			throw new \InvalidArgumentException("Cannot add two schemas with the same schema ID and version ID");
 		}
-		$this->upgradeSchemas[$schemaId] = $schema;
+
+		//schema ID tells us the order when multiple schemas use the same version ID
+		$this->upgradeSchemas[$versionId][$schemaId] = $schema;
 
 		ksort($this->upgradeSchemas, SORT_NUMERIC);
+		ksort($this->upgradeSchemas[$versionId], SORT_NUMERIC);
 
 		$this->outputVersion = max($this->outputVersion, $schema->getVersionId());
 	}
 
 	public function upgrade(BlockStateData $blockStateData) : BlockStateData{
 		$version = $blockStateData->getVersion();
-		foreach($this->upgradeSchemas as $schema){
-			$resultVersion = $schema->versionId;
-			if($version > $resultVersion){
-				//even if this is actually the same version, we have to apply it anyway because mojang are dumb and
-				//didn't always bump the blockstate version when changing it :(
+		foreach($this->upgradeSchemas as $resultVersion => $schemaList){
+			/*
+			 * Sometimes Mojang made changes without bumping the version ID.
+			 * A notable example is 0131_1.18.20.27_beta_to_1.18.30.json, which renamed a bunch of blockIDs.
+			 * When this happens, all the schemas must be applied even if the version is the same, because the input
+			 * version doesn't tell us which of the schemas have already been applied.
+			 * If there's only one schema for a version (the norm), we can safely assume it's already been applied if
+			 * the version is the same, and skip over it.
+			 */
+			if($version > $resultVersion || (count($schemaList) === 1 && $version === $resultVersion)){
 				continue;
 			}
 
-			$newStateData = $this->applyStateRemapped($schema, $blockStateData);
-			if($newStateData !== null){
-				$blockStateData = $newStateData;
-				continue;
-			}
-
-			$oldName = $blockStateData->getName();
-			$newName = $schema->renamedIds[$oldName] ?? null;
-
-			$stateChanges = 0;
-			$states = $blockStateData->getStates();
-
-			$states = $this->applyPropertyAdded($schema, $oldName, $states, $stateChanges);
-			$states = $this->applyPropertyRemoved($schema, $oldName, $states, $stateChanges);
-			$states = $this->applyPropertyRenamedOrValueChanged($schema, $oldName, $states, $stateChanges);
-			$states = $this->applyPropertyValueChanged($schema, $oldName, $states, $stateChanges);
-
-			if($newName !== null || $stateChanges > 0){
-				$blockStateData = new BlockStateData($newName ?? $oldName, $states, $resultVersion);
-				//don't break out; we may need to further upgrade the state
+			foreach($schemaList as $schema){
+				$blockStateData = $this->applySchema($schema, $blockStateData);
 			}
 		}
 
@@ -103,6 +97,30 @@ final class BlockStateUpgrader{
 		return $blockStateData;
 	}
 
+	private function applySchema(BlockStateUpgradeSchema $schema, BlockStateData $blockStateData) : BlockStateData{
+		$newStateData = $this->applyStateRemapped($schema, $blockStateData);
+		if($newStateData !== null){
+			return $newStateData;
+		}
+
+		$oldName = $blockStateData->getName();
+		$newName = $schema->renamedIds[$oldName] ?? null;
+
+		$stateChanges = 0;
+		$states = $blockStateData->getStates();
+
+		$states = $this->applyPropertyAdded($schema, $oldName, $states, $stateChanges);
+		$states = $this->applyPropertyRemoved($schema, $oldName, $states, $stateChanges);
+		$states = $this->applyPropertyRenamedOrValueChanged($schema, $oldName, $states, $stateChanges);
+		$states = $this->applyPropertyValueChanged($schema, $oldName, $states, $stateChanges);
+
+		if($newName !== null || $stateChanges > 0){
+			return new BlockStateData($newName ?? $oldName, $states, $schema->getVersionId());
+		}
+
+		return $blockStateData;
+	}
+
 	private function applyStateRemapped(BlockStateUpgradeSchema $schema, BlockStateData $blockStateData) : ?BlockStateData{
 		$oldName = $blockStateData->getName();
 		$oldState = $blockStateData->getStates();