Consider editing the Custom InventoryHolder Page within the Paper Documentation about docs HOT 8 CLOSED

from a code and management perspective, it is probably one of the more portable ways of handling custom inventories; avoiding snapshots can do some good in terms of performance cost at the expense of portability, I'm just not sure it's exactly a huge issue; ofc, there does come a level of pros and cons here, and generally, it's never really shown up in a manner that it's actually had some noticeable performance impact on normal servers

from docs.

Fixed by 3e39d2d

from docs.

If by portability you mean it doesn't work on some upstream version, I don't think that should be a concern for the paper docs. Adding false sounds good to me.

from docs.

from a code and management perspective, it is probably one of the more portable ways of handling custom inventories; avoiding snapshots can do some good in terms of performance cost at the expense of portability, I'm just not sure it's exactly a huge issue; ofc, there does come a level of pros and cons here, and generally, it's never really shown up in a manner that it's actually had some noticeable performance impact on normal servers

With the greatest of respect, I believe you are incorrect.

I investigated the matter and it seems to be the primary reason items with a lot of custom metadata are actually performance sinks in a lot of plugins.

The long and skinny is, imagine a plugin that uses metadata to create custom gradient lore for their items. And then imagine that another plugin entirely is the one actually causing those items to perform poorly, as on InventoryClickEvents are creating snapshots of entire inventories full of said items repeatedly... every single click...

I am not sure on the etiquette here, so i apologise in advanced if this goes against it, but if you wish more elaboration, please refer to this article i wrote on the spigot forums about this issue a few days ago: Link here

E.g: When investigating plugins that utilize this behaviour (such as core protect and brewery plugin) they could contribute as much as 7%-12% thread utilization when on a server with a lot of custom items.

By setting inventory.getHolder(false) within the brewery plugin specifically, utilization during instances of high item transfer went down to 0.7%.

I believe, truly, this is a common case of "because every plugin was doing it, we had no idea how much it actually impacted other plugins" .

from docs.

It was me and aikar who pushed for the snapshot less things to be added; I've yet to see there being any real issues behind a plugin having a single getHolder call in there, ofc, if you can avoid it that's good, and so that should be corrected, it can pile up but generally, the server kinda dies from that before plugins where ever a concern, maybe that's changed over the years, idk

ofc, if plugin devs can add some filtering to try to cut down on their calls to that method otherwise, that would be a good idea, but it's just never shown up in the community as being a big deal outside of the odd plugin which goes crazy with it, it's not ideal, however.

ofc, IH is just a "nice" way to throw together custom inventories, ideally people would handle a HashMap or something, if we're going to add a "this is a nice way to handle it but does have some performance caveats" message, then we should provide an alternative implementation suggestion which doesn't suffer from this

from docs.

It was me and aikar who pushed for the snapshot less things to be added; I've yet to see there being any real issues behind a plugin having a single getHolder call in there, ofc, if you can avoid it that's good, and so that should be corrected, it can pile up but generally, the server kinda dies from that before plugins where ever a concern, maybe that's changed over the years, idk

ofc, if plugin devs can add some filtering to try to cut down on their calls to that method otherwise, that would be a good idea, but it's just never shown up in the community as being a big deal outside of the odd plugin which goes crazy with it, it's not ideal, however.

ofc, IH is just a "nice" way to throw together custom inventories, ideally people would handle a HashMap or something, if we're going to add a "this is a nice way to handle it but does have some performance caveats" message, then we should provide an alternative implementation suggestion which doesn't suffer from this

I would like to propose you haven't taken into account other plugins in this equation.

Once again, this has quite literally been the primary cause of TPS issues with items that have lots of metadata.

It has never been the metadata itself, but the fact that, due to getHolder snapshotting the inventory on every click (When a tile entity is involved) it is reloading the metadata for these custom items on every click too...

Inventory of Custom Brews with Gradient Lore, NBT data, custom effects etc, being transfered to a shulker box?

Yeah... you've just reconstructed that metadata 729 times in less than 2 seconds.

As stated, replacing getHolder within Brewery alone causes a decrease from 7-12% down to 0.7% thread utilisation...

Brewery was matching CoreProtect in this regard.

Landlord was matching CoreProtect and Brewery too. And Landlord is my plugin (where I added a single call of getHolder in an on click event, not even a functional getHolder, just literally calling the method.

I believe this may be a classic case of noone testing their plugins outside of their own test enviroments / considering how other plugins interact with this method.

The Blockstate being snapshotted for tile entities... which includes the inventory of the tile entity... is a very outside the box "oh god yeah thats a thing isn't it" issue imo.

from docs.

I was curious, so I did a little test with a very simple custom inventory. I created 3500 instances of this and verified that there were indeed 3500 listeners using /paper dumplisteners.

I then tried to recreate your example by creating a double chest full of shulker boxes filled with full written books, enough data that there is a noticable delay between opening the chest and the items appearing. Enough data in fact that picking up the shulkers disconnects the client due to a large packet.
If the cloning of the BlockState for getHolder really is an issue, that should show up noticably with a chest like this.
I then clicked and moved the shulkers in the inventory as fast I could.

I am unable to see the issue. The entire Inventory Click packet is taking up 0.42% of tick, with my inventory listener itself being at 0.02% - below what Spark can reliably measure. In fact, moving the shulkers into the hotbar and triggering the vanilla ItemStack#clone logic is a more intensive task than the listener.
Ignore the "not Paper" please, its my own fork and I am confident that I am not doing any changes to any related logic.

This also matches up with my experience from looking at way too many spark reports - the inventory events themselves are rarely the issue.

In case of your example with Brewery I suspect that the true cause of the issue is the potion conversion logic, considering that will do a large amount of component and metadata changes.
Or this is an issue only on Spigot, I did not test that.

I am not against adding a mention of the snapshot boolean to the docs, but this really does not seem to be a major issue.

from docs.

I was curious, so I did a little test with a very simple custom inventory. I created 3500 instances of this and verified that there were indeed 3500 listeners using /paper dumplisteners.

I then tried to recreate your example by creating a double chest full of shulker boxes filled with full written books, enough data that there is a noticable delay between opening the chest and the items appearing. Enough data in fact that picking up the shulkers disconnects the client due to a large packet. If the cloning of the BlockState for getHolder really is an issue, that should show up noticably with a chest like this. I then clicked and moved the shulkers in the inventory as fast I could.

I am unable to see the issue. The entire Inventory Click packet is taking up 0.42% of tick, with my inventory listener itself being at 0.02% - below what Spark can reliably measure. In fact, moving the shulkers into the hotbar and triggering the vanilla ItemStack#clone logic is a more intensive task than the listener. Ignore the "not Paper" please, its my own fork and I am confident that I am not doing any changes to any related logic.

This also matches up with my experience from looking at way too many spark reports - the inventory events themselves are rarely the issue.

In case of your example with Brewery I suspect that the true cause of the issue is the potion conversion logic, considering that will do a large amount of component and metadata changes. Or this is an issue only on Spigot, I did not test that.

I am not against adding a mention of the snapshot boolean to the docs, but this really does not seem to be a major issue.

I am not questioning what you did, but i find it highly circumspect your events are not actively snapshotting the tile entities on click. (i.e. event.getInventory().getHolder() is not present within your profiler. Indeed... your inventory click event is actually empty. Even if it was a minimal impact, you would at least expect it to show it making the snapshot for the shulker... it is not...

This is not typical paper behaviour. Hell it isnt typical minecraft behaviour... I have conducted my tests on a paper plugin within a paper server.

This is from a low-stress test from a few days ago.

This should, at the minimum, be what you are seeing when conducting this click event on a tile entity...

from docs.