Performance issue with disk manipulator importing with no internal storage
waggz81 opened this issue ยท 4 comments
Issue description:
Disk manipulator severe performance issue when no internal storage available
What happens:
When putting a storage disk in the disk manipulator to redistribute items into storage drawers to clean up disks I am getting 3 TPS/280ms ticks while alone on server. If I hadn't increased max tick the server would be crashing as it is 70s behind. Putting a storage disk in the disk drive alleviates the problem even though items are still being sent to the external storage. I have not tested if having items on the disk manipulator disk that have nowhere to go changes anything yet.
What you expected to happen:
Lesser impact on TPS when importing items from disk manipulator without available internal storage
Steps to reproduce:
- Have items on disk
- Have external storage for items
- Have no item storage available via storage blocks or disk drive
- Put item storage disk in disk manipulator to import to network
Version (make sure you are on the latest version before reporting):
- Minecraft: 1.16.5
- Forge: 36.1.16
- Refined Storage: 1.9.12
Does this issue occur on a server? [yes/no] Yes
If a (crash)log is relevant for this issue, link it here:
Do you use the drawer controller (instead of having like 50 external storages)?
Also for the exact usecase of such defragmentation there is a 'priority' setting on the external storage, so you can just have two disks, and put them into the disk drive one at a time (so that extras are put on the other disk, but drawers are prioritized), thats how I always did it
Other than that, I recall there were performance issues with storage drawers, a good workaround/hint is to set the whitelist on the external storage(s), so that RS system does not try to insert every item into the drawer, calling its not necessarily the most optimized code a lot of times.
From your sample:
This happens twice, so most of the time is spent in the storage drawers code, and while RS does call it a lot - see the above workaround/explanation.
edit: so while I said all of the above, this still totally happens, prioritized defragmentation with disk space available as I've described goes totally fine while when no disk space is left the tps goes way down
Do you use the drawer controller (instead of having like 50 external storages)?
Also for the exact usecase of such defragmentation there is a 'priority' setting on the external storage, so you can just have two disks, and put them into the disk drive one at a time (so that extras are put on the other disk, but drawers are prioritized), thats how I always did it
Other than that, I recall there were performance issues with storage drawers, a good workaround/hint is to set the whitelist on the external storage(s), so that RS system does not try to insert every item into the drawer, calling its not necessarily the most optimized code a lot of times.
Yeah this is how I had mine set up, the drawers were on a single drawer controller with a single external storage set to a higher priority than the disk drive. I like using the compacting drawers for a lot of stuff because of the way it makes nuggets/ingots/blocks without crafting between them. I had to abandon the use of the drawers though because I got to the point where I am using the creative destructor from Cable Tiers mod and the RS system was getting overloaded with the amount of stuff it was trying to import so quickly. I know this is all related to other mods but appreciate you looking into it.
Ok, Storage Drawers call ItemStack.copy on each getStackInSlot call, which gets called a lot by RS, and calling ItemStack.copy is stupidly expensive because Forge decided so.
Copying an ItemStack on each getStackInSlot call is pretty meh, so atm I am trying to make a little caching patch to storage drawers to see if it fixes this, and if it does I'll make a PR to storage drawers. Even if it does not fix this completely (which it might) it still would improve storage drawers performance noticeably so I would make that PR anyway.
But there is nothing (significant) to fix in RS I think, maybe a couple of copies could be cached somehow as well but I don't see anything immediate, like that copying in storage drawers.
tl;dr; what happens from RS side and why it is not really an RS issue but rather Storage Drawers one (I think)
RS walks through each item type (and there can be a lot) in the disk and tries to insert (some number of, depending on upgrades) it into the network, and if it inserts succesfully it stops (breaks the loop) until next tick.
Inserting into the network consists of walking through storages in priority order and trying to insert into them.
So lets say storages are drawer controller (limited by drawer item types) and a disk (not limited by types).
On first item you try to import it tries to insert into drawer, say it fails, then it tries to insert into a network disk and succeeds - and because of that the whole insert operation succeeded and RS stops doing anything until next tick (thus not tanking TPS).
But now, same scenario without a unconstrained disk - RS tries to insert into drawers, fails, and since insertion failed it tries next item from the disk - it fails again and continues looping through all item types in the disk until it finds whatever fits into the drawers.
All of these failed tries call Storage Drawers implementation of getStackInSlot which in turn copies ItemStacks which is slow.
Relatively slow, of course, but given how many times a tick it happens it actually tanks TPS.