From 23ea72116428c28f748b25720890b622139df5b6 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 22:15:02 +0000 Subject: [PATCH 1/7] Reduce packets-per-batch limit to 100 this should be well in excess of requirements with the ItemStackRequest system in use. --- src/network/mcpe/NetworkSession.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/network/mcpe/NetworkSession.php b/src/network/mcpe/NetworkSession.php index f40f2085f..99ba222b2 100644 --- a/src/network/mcpe/NetworkSession.php +++ b/src/network/mcpe/NetworkSession.php @@ -376,7 +376,7 @@ class NetworkSession{ $stream = new BinaryStream($decompressed); $count = 0; foreach(PacketBatch::decodeRaw($stream) as $buffer){ - if(++$count > 1300){ + if(++$count > 100){ throw new PacketHandlingException("Too many packets in batch"); } $packet = $this->packetPool->getPacket($buffer); From 035a0a4e9dddc58286883abe82c9303151e5618d Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 22:57:58 +0000 Subject: [PATCH 2/7] InventoryManager: specialize trackItemStack() to avoid useless lookups --- src/network/mcpe/InventoryManager.php | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 5b5708b8a..4bf1e5a15 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -412,11 +412,11 @@ class InventoryManager{ $clientSideItem = $inventoryEntry->predictions[$slot] ?? null; if($clientSideItem === null || !$clientSideItem->equals($currentItem)){ //no prediction or incorrect - do not associate this with the currently active itemstack request - $this->trackItemStack($inventory, $slot, $currentItem, null); + $this->trackItemStack($inventoryEntry, $slot, $currentItem, null); $inventoryEntry->pendingSyncs[$slot] = $slot; }else{ //correctly predicted - associate the change with the currently active itemstack request - $this->trackItemStack($inventory, $slot, $currentItem, $this->currentItemStackRequestId); + $this->trackItemStack($inventoryEntry, $slot, $currentItem, $this->currentItemStackRequestId); } unset($inventoryEntry->predictions[$slot]); @@ -481,7 +481,7 @@ class InventoryManager{ $contents = []; foreach($inventory->getContents(true) as $slot => $item){ $itemStack = TypeConverter::getInstance()->coreItemStackToNet($item); - $info = $this->trackItemStack($inventory, $slot, $itemStack, null); + $info = $this->trackItemStack($entry, $slot, $itemStack, null); $contents[] = new ItemStackWrapper($info->getStackId(), $info->getItemStack()); } if($entry->complexSlotMap !== null){ @@ -602,11 +602,7 @@ class InventoryManager{ return $entry?->itemStackInfos[$slot] ?? null; } - private function trackItemStack(Inventory $inventory, int $slotId, ItemStack $itemStack, ?int $itemStackRequestId) : ItemStackInfo{ - $entry = $this->inventories[spl_object_id($inventory)] ?? null; - if($entry === null){ - throw new \LogicException("Cannot track an item stack for an untracked inventory"); - } + private function trackItemStack(InventoryManagerEntry $entry, int $slotId, ItemStack $itemStack, ?int $itemStackRequestId) : ItemStackInfo{ $existing = $entry->itemStackInfos[$slotId] ?? null; if($existing !== null && $existing->getItemStack()->equals($itemStack) && $existing->getRequestId() === $itemStackRequestId){ return $existing; From 1992d3b6db4f3c2e993d7f5f2f0b4d69965508bc Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 23:08:17 +0000 Subject: [PATCH 3/7] InventoryManager: avoid useless work in trackItemStack() this attempts to accommodate slots being set to themselves, which is a rare enough occurrence (only plugins will cause it) that it doesn't make sense to penalize every inventory update this way. attempting to avoid changing the itemstackID in this way is detrimental to performance, and it doesn't actually matter if we set a new itemstackID anyway. --- src/network/mcpe/InventoryManager.php | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 4bf1e5a15..a6fe63635 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -603,11 +603,6 @@ class InventoryManager{ } private function trackItemStack(InventoryManagerEntry $entry, int $slotId, ItemStack $itemStack, ?int $itemStackRequestId) : ItemStackInfo{ - $existing = $entry->itemStackInfos[$slotId] ?? null; - if($existing !== null && $existing->getItemStack()->equals($itemStack) && $existing->getRequestId() === $itemStackRequestId){ - return $existing; - } - //TODO: ItemStack->isNull() would be nice to have here $info = new ItemStackInfo($itemStackRequestId, $itemStack->getId() === 0 ? 0 : $this->newItemStackId(), $itemStack); return $entry->itemStackInfos[$slotId] = $info; From 63310cf764039af3524d9e32b61c6b2d3dd3d9eb Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 23:18:43 +0000 Subject: [PATCH 4/7] Do not cache ItemStacks for every item this is very memory inefficient, and only provides a performance advantage in cold code anyway. --- src/network/mcpe/InventoryManager.php | 19 ++++++++++--------- src/network/mcpe/InventoryManagerEntry.php | 2 +- src/network/mcpe/ItemStackInfo.php | 7 +------ .../mcpe/handler/InGamePacketHandler.php | 7 ++----- 4 files changed, 14 insertions(+), 21 deletions(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index a6fe63635..5aa22b002 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -413,7 +413,7 @@ class InventoryManager{ if($clientSideItem === null || !$clientSideItem->equals($currentItem)){ //no prediction or incorrect - do not associate this with the currently active itemstack request $this->trackItemStack($inventoryEntry, $slot, $currentItem, null); - $inventoryEntry->pendingSyncs[$slot] = $slot; + $inventoryEntry->pendingSyncs[$slot] = $currentItem; }else{ //correctly predicted - associate the change with the currently active itemstack request $this->trackItemStack($inventoryEntry, $slot, $currentItem, $this->currentItemStackRequestId); @@ -422,7 +422,7 @@ class InventoryManager{ unset($inventoryEntry->predictions[$slot]); } - public function syncSlot(Inventory $inventory, int $slot) : void{ + public function syncSlot(Inventory $inventory, int $slot, ItemStack $itemStack) : void{ $entry = $this->inventories[spl_object_id($inventory)] ?? null; if($entry === null){ throw new \LogicException("Cannot sync an untracked inventory"); @@ -439,7 +439,7 @@ class InventoryManager{ $netSlot = $slot; } - $itemStackWrapper = new ItemStackWrapper($itemStackInfo->getStackId(), $itemStackInfo->getItemStack()); + $itemStackWrapper = new ItemStackWrapper($itemStackInfo->getStackId(), $itemStack); if($windowId === ContainerIds::OFFHAND){ //TODO: HACK! //The client may sometimes ignore the InventorySlotPacket for the offhand slot. @@ -482,7 +482,7 @@ class InventoryManager{ foreach($inventory->getContents(true) as $slot => $item){ $itemStack = TypeConverter::getInstance()->coreItemStackToNet($item); $info = $this->trackItemStack($entry, $slot, $itemStack, null); - $contents[] = new ItemStackWrapper($info->getStackId(), $info->getItemStack()); + $contents[] = new ItemStackWrapper($info->getStackId(), $itemStack); } if($entry->complexSlotMap !== null){ foreach($contents as $slotId => $info){ @@ -513,6 +513,7 @@ class InventoryManager{ } public function syncMismatchedPredictedSlotChanges() : void{ + $typeConverter = TypeConverter::getInstance(); foreach($this->inventories as $entry){ $inventory = $entry->inventory; foreach($entry->predictions as $slot => $expectedItem){ @@ -522,7 +523,7 @@ class InventoryManager{ //any prediction that still exists at this point is a slot that was predicted to change but didn't $this->session->getLogger()->debug("Detected prediction mismatch in inventory " . get_class($inventory) . "#" . spl_object_id($inventory) . " slot $slot"); - $entry->pendingSyncs[$slot] = $slot; + $entry->pendingSyncs[$slot] = $typeConverter->coreItemStackToNet($inventory->getItem($slot)); } $entry->predictions = []; @@ -541,8 +542,8 @@ class InventoryManager{ } $inventory = $entry->inventory; $this->session->getLogger()->debug("Syncing slots " . implode(", ", array_keys($entry->pendingSyncs)) . " in inventory " . get_class($inventory) . "#" . spl_object_id($inventory)); - foreach($entry->pendingSyncs as $slot){ - $this->syncSlot($inventory, $slot); + foreach($entry->pendingSyncs as $slot => $itemStack){ + $this->syncSlot($inventory, $slot, $itemStack); } $entry->pendingSyncs = []; } @@ -571,7 +572,7 @@ class InventoryManager{ $this->session->sendDataPacket(MobEquipmentPacket::create( $this->player->getId(), - new ItemStackWrapper($itemStackInfo->getStackId(), $itemStackInfo->getItemStack()), + new ItemStackWrapper($itemStackInfo->getStackId(), TypeConverter::getInstance()->coreItemStackToNet($playerInventory->getItemInHand())), $selected, $selected, ContainerIds::INVENTORY @@ -604,7 +605,7 @@ class InventoryManager{ private function trackItemStack(InventoryManagerEntry $entry, int $slotId, ItemStack $itemStack, ?int $itemStackRequestId) : ItemStackInfo{ //TODO: ItemStack->isNull() would be nice to have here - $info = new ItemStackInfo($itemStackRequestId, $itemStack->getId() === 0 ? 0 : $this->newItemStackId(), $itemStack); + $info = new ItemStackInfo($itemStackRequestId, $itemStack->getId() === 0 ? 0 : $this->newItemStackId()); return $entry->itemStackInfos[$slotId] = $info; } } diff --git a/src/network/mcpe/InventoryManagerEntry.php b/src/network/mcpe/InventoryManagerEntry.php index fa49baf87..ac2a40dfe 100644 --- a/src/network/mcpe/InventoryManagerEntry.php +++ b/src/network/mcpe/InventoryManagerEntry.php @@ -41,7 +41,7 @@ final class InventoryManagerEntry{ /** * @var int[] - * @phpstan-var array + * @phpstan-var array */ public array $pendingSyncs = []; diff --git a/src/network/mcpe/ItemStackInfo.php b/src/network/mcpe/ItemStackInfo.php index 630765bfa..ace138198 100644 --- a/src/network/mcpe/ItemStackInfo.php +++ b/src/network/mcpe/ItemStackInfo.php @@ -23,19 +23,14 @@ declare(strict_types=1); namespace pocketmine\network\mcpe; -use pocketmine\network\mcpe\protocol\types\inventory\ItemStack; - final class ItemStackInfo{ public function __construct( private ?int $requestId, - private int $stackId, - private ItemStack $itemStack + private int $stackId ){} public function getRequestId() : ?int{ return $this->requestId; } public function getStackId() : int{ return $this->stackId; } - - public function getItemStack() : ItemStack{ return $this->itemStack; } } diff --git a/src/network/mcpe/handler/InGamePacketHandler.php b/src/network/mcpe/handler/InGamePacketHandler.php index 68e939195..67d7ebb65 100644 --- a/src/network/mcpe/handler/InGamePacketHandler.php +++ b/src/network/mcpe/handler/InGamePacketHandler.php @@ -394,14 +394,11 @@ class InGamePacketHandler extends PacketHandler{ //it's technically possible to see this more than once, but a normal client should never do that. $inventory = $this->player->getInventory(); - $heldItemStack = $this->inventoryManager->getItemStackInfo($inventory, $inventory->getHeldItemIndex())?->getItemStack(); - if($heldItemStack === null){ - throw new AssumptionFailedError("Missing itemstack info for held item"); - } + $heldItemStack = TypeConverter::getInstance()->coreItemStackToNet($inventory->getItemInHand()); $droppedItemStack = $networkInventoryAction->newItem->getItemStack(); //because the client doesn't tell us the expected itemstack ID, we have to deep-compare our known //itemstack info with the one the client sent. This is costly, but we don't have any other option :( - if(!$heldItemStack->equalsWithoutCount($droppedItemStack) || $heldItemStack->getCount() < $droppedItemStack->getCount()){ + if($heldItemStack->getCount() < $droppedItemStack->getCount() || !$heldItemStack->equalsWithoutCount($droppedItemStack)){ return false; } From ee72e80fbbdd138e0f7130e2ea557e4ae6dd3a42 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 23:21:24 +0000 Subject: [PATCH 5/7] ItemStackResponseBuilder: removed incorrect code the client expects that all itemstacks must be acked by ItemStackResponse, regardless of whether the server changed them to some other item. We'll overwrite the item to the correct thing at the end of the tick anyway. --- src/network/mcpe/handler/ItemStackResponseBuilder.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/network/mcpe/handler/ItemStackResponseBuilder.php b/src/network/mcpe/handler/ItemStackResponseBuilder.php index 1c742077f..325a2b108 100644 --- a/src/network/mcpe/handler/ItemStackResponseBuilder.php +++ b/src/network/mcpe/handler/ItemStackResponseBuilder.php @@ -83,12 +83,6 @@ final class ItemStackResponseBuilder{ if($itemStackInfo === null){ throw new AssumptionFailedError("ItemStackInfo should never be null for an open inventory"); } - if($itemStackInfo->getRequestId() !== $this->requestId){ - //the itemstack may have been synced due to transaction producing results that the client did not - //predict correctly, which will wipe out the tracked request ID (intentionally) - //TODO: is this the correct behaviour? - continue; - } $item = $inventory->getItem($slot); $responseInfosByContainer[$containerInterfaceId][] = new ItemStackResponseSlotInfo( From ecc830a689df326ab88497de438657c8673c8b07 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 23:24:52 +0000 Subject: [PATCH 6/7] InventoryManager: avoid calling TypeConverter::getInstance() in a loop --- src/network/mcpe/InventoryManager.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/network/mcpe/InventoryManager.php b/src/network/mcpe/InventoryManager.php index 5aa22b002..0222d6d40 100644 --- a/src/network/mcpe/InventoryManager.php +++ b/src/network/mcpe/InventoryManager.php @@ -479,8 +479,9 @@ class InventoryManager{ $entry->predictions = []; $entry->pendingSyncs = []; $contents = []; + $typeConverter = TypeConverter::getInstance(); foreach($inventory->getContents(true) as $slot => $item){ - $itemStack = TypeConverter::getInstance()->coreItemStackToNet($item); + $itemStack = $typeConverter->coreItemStackToNet($item); $info = $this->trackItemStack($entry, $slot, $itemStack, null); $contents[] = new ItemStackWrapper($info->getStackId(), $itemStack); } From e7771d76f2fb9d0d9561f5c02a45035eab6c3dc4 Mon Sep 17 00:00:00 2001 From: "Dylan K. Taylor" Date: Mon, 20 Mar 2023 23:29:02 +0000 Subject: [PATCH 7/7] Cover buffered inventory sync in timings --- src/network/mcpe/NetworkSession.php | 7 ++++++- src/timings/Timings.php | 4 ++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/network/mcpe/NetworkSession.php b/src/network/mcpe/NetworkSession.php index 99ba222b2..2d628c839 100644 --- a/src/network/mcpe/NetworkSession.php +++ b/src/network/mcpe/NetworkSession.php @@ -1146,7 +1146,12 @@ class NetworkSession{ $attribute->markSynchronized(); } } - $this->invManager?->flushPendingUpdates(); + Timings::$playerNetworkSendInventorySync->startTiming(); + try{ + $this->invManager?->flushPendingUpdates(); + }finally{ + Timings::$playerNetworkSendInventorySync->stopTiming(); + } $this->flushSendBuffer(); } diff --git a/src/timings/Timings.php b/src/timings/Timings.php index 3359a2eeb..668860132 100644 --- a/src/timings/Timings.php +++ b/src/timings/Timings.php @@ -56,6 +56,9 @@ abstract class Timings{ /** @var TimingsHandler */ public static $playerNetworkSendEncrypt; + + public static TimingsHandler $playerNetworkSendInventorySync; + /** @var TimingsHandler */ public static $playerNetworkReceive; /** @var TimingsHandler */ @@ -170,6 +173,7 @@ abstract class Timings{ self::$playerNetworkSendCompressBroadcast = new TimingsHandler(self::INCLUDED_BY_OTHER_TIMINGS_PREFIX . "Player Network Send - Compression (Broadcast)", self::$playerNetworkSendCompress); self::$playerNetworkSendCompressSessionBuffer = new TimingsHandler(self::INCLUDED_BY_OTHER_TIMINGS_PREFIX . "Player Network Send - Compression (Session Buffer)", self::$playerNetworkSendCompress); self::$playerNetworkSendEncrypt = new TimingsHandler(self::INCLUDED_BY_OTHER_TIMINGS_PREFIX . "Player Network Send - Encryption", self::$playerNetworkSend); + self::$playerNetworkSendInventorySync = new TimingsHandler(self::INCLUDED_BY_OTHER_TIMINGS_PREFIX . "Player Network Send - Inventory Sync", self::$playerNetworkSend); self::$playerNetworkReceive = new TimingsHandler("Player Network Receive", self::$connection); self::$playerNetworkReceiveDecompress = new TimingsHandler(self::INCLUDED_BY_OTHER_TIMINGS_PREFIX . "Player Network Receive - Decompression", self::$playerNetworkReceive);