Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary durability check in ItemStack#isSimilar #9979

Merged
merged 2 commits into from
Dec 2, 2023

Conversation

MrPowerGamerBR
Copy link
Sponsor Contributor

@MrPowerGamerBR MrPowerGamerBR commented Nov 26, 2023

I know that this looks like a stupid optimization that doesn't do anything, but hear me out: getDurability() ain't free, when you call getDurability(), it calls getItemMeta(), which then allocates a new ItemMeta or clones the current item's item meta.

However, this means that we are unnecessarily allocating useless ItemMeta objects if we are comparing one or more items that don't have any durability (if they had durability, they would have a non-null meta), and this impact can be noticed if one of your plugins has a canHoldItem function that checks if a inventory can hold an item by looping thru the player's inventory, especially if the plugin is checking this multiple times in a tick.

To avoid this, we can just... not check for the item's durability! Don't worry, the durability of the item is checked when it checks if both item metas are equal.

This is a leftover from when checking for the item's durability was "free" because the durability was stored in the ItemStack itself, this was changed in Minecraft 1.13, however the isSimilar(...) method hasn't been changed since it was added in 2012.

Of course, maybe this patch is a bit useless? There is a performance benefit if you remove that call since you avoid initializing/cloning a ItemMeta if one or both of the items have a null ItemMeta + removes a deprecated call + improves the code to match how items work in a post-1.13 world, but the benefit is so so so smol (really only helps plugins that are constantly checking if an item is similar to another, which is my case, where one of my plugins allows players to "quick harvest" their farms, and for each crop it needs to check if the drops fits on their inventory) that maybe it could just be ignored, after all, there's also the work of maintaining this diff to fix something that just boils down to "well now its correct and there is a super tiny perf improvement". (Maybe this should be upstreamed to Spigot since they are the ones who are maintaining Bukkit? but paper my beloved tho...)

(The reason I found out that this had a performance impact was because the getDurability() was using 0.08ms each tick in one of my plugins according to spark... yeah, sadly it ain't a super big crazy optimization, the performance impact would be bigger if you have more plugins using isSimilar(...) tho, something something pick a function and make it fast or something like that)

Keep in mind that CraftItemStack does override isSimilar(), and the overriden method also checks getDurability(). However, CraftItemStack getDurability() actually gets the damage directly from the NBT tags. Maybe both checks should be removed? "oh but if CraftItemStack overrides this method then your patch is USELESS!!!" actually not because you initialize the ItemStack class directly, and CraftItemStack delegates to ItemStack's isSimilar() if the stack being compared is not a CraftItemStack, see this debug log: [org.bukkit.craftbukkit.v1_20_R2.inventory.CraftItemStack] CraftItemStack isSimilar comparing ItemStack{CRAFTING_TABLE x 1} (class org.bukkit.craftbukkit.v1_20_R2.inventory.CraftItemStack) to ItemStack{BEETROOT x 1} (class org.bukkit.inventory.ItemStack)

Example of a plugin code that uses isSimilar(...):

fun Inventory.canHoldItem(stack: ItemStack?): Boolean {
	if (stack == null) { return true }
	if (stack.type == Material.AIR) { return true }
	val storageContents = this.storageContents
	if (storageContents != null) {
		for (invItem in storageContents) {
			if (invItem == null) return true // Empty slot!
			if (invItem.isSimilar(stack)) { // 
				if (invItem.maxStackSize >= invItem.amount + stack.amount) {
					// Yes, we can add the items to this stack!
					return true
				}
			}
		}
	}
	return false
}

@MrPowerGamerBR MrPowerGamerBR requested a review from a team as a code owner November 26, 2023 23:17
@MrPowerGamerBR MrPowerGamerBR changed the title Optimize ItemStack#isSimilar(...) by removing the unnecessary durability checks Remove unnecessary durability check in ItemStack#isSimilar(...) Nov 26, 2023
Copy link
Member

@Machine-Maker Machine-Maker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Confirmed that isSimilar still functions correctly when comparing damaged items.

It did make me go looking for other places ItemStack#getDurability is still used, and there are several which probably should also be reworked at some point, but that doesn't have to be here.

@Machine-Maker Machine-Maker changed the title Remove unnecessary durability check in ItemStack#isSimilar(...) Remove unnecessary durability check in ItemStack#isSimilar Dec 2, 2023
@Machine-Maker Machine-Maker merged commit 8bda1f7 into PaperMC:master Dec 2, 2023
1 check passed
lynxplay pushed a commit to lynxplay/paper that referenced this pull request Feb 23, 2024
)

* Optimize "ItemStack#isSimilar(...)" by removing the unnecessary durability check

* Change patch name and add a better commit message
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants