Description (Required)

Some performance issues on build 353-357. When someone is mining with even a simple diamond pickaxe (guy who caused issue on pandas server had eff 5, unbreaking 4 dia pick).

Steps to reproduce the Issue (Required)

Run build 357, go to a desert with an eff 5 dia pick and just go ham. Seems to be a result of the new service stuff (0fd7892).

MC really sucks for performance stuff. I think Spigot/MC handles some of the NBT stuff really badly internally which means that it doesn't just instantly return false but instead does much more intense ops for a standard item. To be fair I don't think they ever expected so many calls but still, it should have better internal checks.

With build 352 a player can dig a ton, it may bring down tps a little but no where near as much.

Note, the server @J3fftw1 is terrible (damn shared hosts) so on a proper server this wouldn't be as much of an issue and likely wont be for others. But, it is still a big impact as the result of this 1 build.

Expected behavior (Required)

Not 2 TPS :LuL:

Server Log / Error Report

https://pastebin.com/zAxzuxrd

Environment (Required)

• Minecraft Version: 1.14.4
• Slimefun Version: 357 (can also repro as low as 353)
• CS-CoreLib Version: 78

The server I play on had a similar issue, but it happened before 357. The problem for us was the large number of ItemStack.getItemMeta calls in SlimefunItem.isDisabled and SlimefunItem.getByItem. Apparently getItemMeta can be a bit slow if the items have a fair bit of lore or whatever, and every interact, Slimefun calls it for each of the ~700 Slimefun items. Based on the thread dump, it seems like the same problem. It's odd that the problem goes away for you in 352.

I did experiment some with fixing this. At first, I started rewriting everything so a single ItemMeta could be created and passed through, but then I got more confident that it was safe to rely on a displayname-based lookup. The changes on https://github.com/creator3/Slimefun4/commits/item-registry are currently running on our server. The change basically just uses a Map on the displayname to narrow down the number of candidate items and then check each one of those normally (with SlimefunItem.isItem). After making this change, I was able to spam click a stick with an absurd amount of lore without causing any lag.

I can rebase on master and open a PR if that's something that might be accepted. I'm not really sure how it will affect addons, but they would have to do something pretty funky with the displayname to cause a problem. (A single item would have to have multiple names)

@creator3 Interesting.
If getItemMeta() is such a heavy call then we should maybe consider storing the ItemMeta instance for each SlimefunItem, it is immutable anyway, since it is just a snapshot basically.

I wonder if Bukkit overrides hashCode for ItemStack and how that's created. If we could have like a HashSet lookup to just see if it is an item that way. It would be a quick lookup but not sure of their implementation.

The biggest issue is just that it's called 700 times (with ExoticGarden installed). Or actually maybe 1,400 calls because a vanilla item will trigger calls to both isDisabled and getItemById. Once we stop doing that, I don't know how much of a difference eliminating a small number of calls by saving it for each SlimefunItem would make. I haven't checked, but I think it's also likely that getting the ItemMeta from the NMS-based CraftItemStacks is more expensive than the plain Bukkit ItemStacks, so getting the meta from the SlimefunItem side might not be as bad as getting it for the in-game stack being checked.

I don't think the hashCode approach would work well. There are still some items that ignore lore in their calls to SlimefunManager.isItemSimiliar (like chargeable items). That's why I only did the lookup based on the name and then allowed SlimefunItem.isItem to sort through the rest normally

Ideally, items would be identified solely by the ID stored in the ItemDataService, so I kinda feel like the whole thing is just for backwards-compatibility with old items

The lore should basically always be ignored. If something is soulbound it will fail the check if lore is tested. This is why the ID system is so critital it allows for a lot more to be done while keeping performance really good (as long as it isn't called 1000 times a second).

I think the issue though is that hashCode use the lore as part of the hash... which yeah would kill that idea.

Bukkit sucks man... I'm not sure what would be the best thing to do here.