Problem Details

While profiling our server, a load is observed from MetalChests even when they're not in use.

From a quick code review, we can see:
1- all chests are updated at the same time here:
https://github.com/T145/metalchests/blob/master/src/main/java/T145/metalchests/api/chests/ChestAnimator.java#L148
=> Consider smoothing the load over several ticks. Also, ones could use masking instead of a modulo operator.
For example, we could replace

if (!world.isRemote && world.getTotalWorldTime() % 20 == 0) {

with

if (!world.isRemote && ((world.getTotalWorldTime() + x + z) & 0x1F) == 0) {

2- Comparator update is done for every slot changed, it should be every tick
https://github.com/T145/metalchests/blob/master/src/main/java/T145/metalchests/tiles/TileMetalChest.java#L75-L78
=> Consider caching the need for comparator update and only do it during next tick.

3- chests are notifying of change systematically instead of just when their inventory did change
https://github.com/T145/metalchests/blob/master/src/main/java/T145/metalchests/api/chests/ChestAnimator.java#L69-L78
There's already a trigger to update comparator in the inventory itself, so do we actually need the notifying of change other than opening/closing the inventory?

Screenshots

Profiling over 10 mn with half a stack of MetalChest placed, no active update to their inventory:

Environment

• OS: CentOS
• Full Forge version: 1.12.2-14.23.5.2836
• Mod version: v4.6.9.g1f83dbf
• Others: spongeforge-1.12.2-2825-7.1.6-RC3716 + spark-1.2.0

Thanks for taking the time to review the mod's performance! As you can tell it is one of my priorities.

1. I agree. Using the coordinates adds unique tick schemes for each chest.
2. The comparator level only changes when the contents of the inventory do, so why should it be every tick? That just seems like unnecessary overhead. Based on your recommendation, I think you mean to not update the comparator level on every slot change, but rather have this set a flag that if true updates the comparator level on the next tick. This way comparator updates are further reduced.
3. I think you're confusing comparator updates w/ block updates here. That call is necessary for a trapped chest to update redstone. However, I can just put these calls in the openInventory and closeInventory methods so they don't happen when the chest syncs numPlayersUsing.

3. yes, I was a bit confused myself on what was going on there.

Calling TileEntity::markDirty() ensures the chunk will be saved on disk and update comparator output level. I think it would make more sense to call TileEntity::markDirty() instead of just World::updateComparatorOutputLevel().

Regarding World::notifyNeighborsOfStateChange(), Vanilla only calls it when opening/closing the chest. However, there's 2 calls:
A- A first call on the chest coordinates, independently of the chest type (trapped or not).
B- A second call on the block right below the chest, only for trapped chests.
Currently, the update proposal only does:
C- A single call on the chest coordinates, only for trapped chests.

I see TileMetalChest::update() calls ChestAnimator::tick() that periodically call WorldContextProvider.activateChest() that calls World.addBlockEvent().
Is that for updating the crystal chest (which is pending implemention)?

1. Well, forcing a chunk save that frequently sounds like it would cause unnecessary overhead. I'll leave that to the server.
2. Correct. And I do the same thing.
3. No, it's for updating numPlayersUsing.

And I'm not implementing "the crystal chest," it will be a crystal upgrade available for every variant.

As for performance, has everything improved?

1- it's not forcing a save, it's marking the chunk 'to be saved' in the next save cycle. Without it, you rely on other blocks in the same chunk to actually mark the chunk 'to be saved'.

2- comparator updates are fixed.

3- MetalChests doesn't do the same thing as vanilla in terms of World::notifyNeighborsOfStateChange(), see my previous comment.

4- World.addBlockEvent() is called periodically.
I understand it's not for the crystal upgrade, it's intended for updating numPlayersUsing.
Did I get that right?