Refactor inventory cache updates
yueh opened this issue ยท 5 comments
We now have the chance to break the API do finally redesign how the inventory handling and the cache are updated as these will cause major breaks, which were not possible during 1.7.10 without breaking every addon at the same time.
The current approach mostly merges iventory handling and monitoring them into the same system. This certainly has some advantages in keeping it more simple, but we immediately hit the extremely limited inventory handling in minecraft itself.
To circumvent these limitations we are extremely defensive about it and prefer rebuilding the cache many more times than actually needed. Rebuilding it is really needed due to how many broken inventories are out there and how minecraft/forge treats them. Which basically means, if a single inventory changes, we have to rescan the whole world, because some mod had the great idea to teleport items around and forget to trigger a neighbor notification on the actual inventory storing the itemstack. Further trusting any IItemHandler is not possible as they do not return what is stored, but essentially it just tells us what to destroy. The inventory itself might have stored, trashed or teleported the item.
As the caches are never used to actually move items around, just mostly for things like the terminal list, we should separte it into a basic inventory handling, which just takes care of moving items around but no longer monitor it.
The caching layer should be a completely separate subsystem and ideally just listen for notifications send by the actual storage system.
But the "broken inventory" problem still exists. But we might be able to solve it by using more localised caches and only pass deltas around instead of rebuilding the cache completely.
Further we most likely have to split it into safe and unsafe inventories. For example our own cells/CellInventory can easily propagate events containing a delta without having to revalidate every existing cell. But any inventory attached to a storage bus most likely has to be reevaluated with every change. Of course these could be made more localised, like inserting a single piece of cobble will cause it to reevaluate the local cache of a storagebus (already exists) and then just posts a +1 cobblestone event to the network itself should the inventory now contains the actual item. When not it has to post a -1 cobblestone to indicate it was destroyed or at least the new location is unknown.
Instead of forcing the network to rebuild the whole cache.
The major downside of relying on correct delta update, is addons can easily break stuff. Because they do not read the docs stating you have to send explicitly send the update or just send it for simulate and modulate or whatever else. While currently is nearly impossible to break the cache due to its defensive way, that would clearly change with focusing on more performance.
I know Storage Drawers has long been on your list of broken inventories.
Storage drawers are a different kind of broken due to reporting non existant items as existing.
But the issue here is the whole inventory handling in minecraft and forge. Which pretty much is only designed for the "hopper dumps stuff into chest" use case. Everything which needs to know, if the item still exists somewhere is pretty much left in the cold and has to do the worst possible option in terms of performance. Searching every observable inventory in the universe for it.
Are you considering an alternate API for the storage bus that would let an inventory take more control to post inventory changes without needing to be polled and validated on every change? I certainly have the information available to tell you if something is accepted or voided or whatever, but no way to express that through a storage bus.
The first goal is to move to IItemHandlers for the basic inventory interactions. So we no longer need to provide our own solution for it and either have every storage mod support AE explicitely or we have to support every storage mod explicitely. With IItemHandler it just works out of the box.
But this is just the extract/insert part.
By default provide a basic caching layer on the storage bus level (it already exists), let it compare the expected inventory content and the actual one. Instead of having every cache rebuild itself afterwards, just push a "+1 cobble" event to them and apply it.
It might be an idea to have a cap like MEMonitorable to replace the basic monitor with a more specialised one. But caps are really limited for these use cases. They are not really designed in way that ever mod has to provide their own solution, actually they are to be injected by forge and the mod has to provide a default implementation. Which here is pretty much useless as all it would do is forward/delegete the method calls and the mod still has to ensure it forwards to correct calls to the cap. So it would certainly be easier to remove 1 useless abstraction layer to keep it simpler and let the author provide their own fitting implementation instead.
But this still places us at a risk of someone not reading the docs, doing stupid stuff and affected the state of AE itself, resulting in invalid inventory caches.
Slightly related, if you offered an interface that passed some sort of transaction ID with requests to simulate inventory changes, then even an inventory like the compacting drawers could play nicely with the auto-crafting system.
This will not solve anything. Transactions would have no time limit except until a server restart.
So until this is happening, you have to maintain a cached inventory for every single transaction you ever encountered. Correctly marking items as used for this transaction, while also synchronising every transaction. It can easily happen that you have a long running manual request being intercepted by an automatic one requesting all needed materials before the manual even reaches this itemtype. Which has to be synchronised to the manual transaction to indicate that these items are now missing. It even has to synchronise other transactions reservering items and as these have not time limit, it basically will result in starving the the inventory due to everything being claimed but never freed. Not to even speak about having to maintain thousands of copies of the inventory in different states and how it will affect memory and cpu usage.
As already said, the only sane option is to not report fake items. Of course it still runs into the issue when something "steals" the items while the job is calculating. But this is really uncommon, compared to pretty much always with fake items.
I know Storage Drawers has long been on your list of broken inventories. Are you considering an alternate API for the storage bus that would let an inventory take more control to post inventory changes without needing to be polled and validated on every change? I certainly have the information available to tell you if something is accepted or voided or whatever, but no way to express that through a storage bus.
Slightly related, if you offered an interface that passed some sort of transaction ID with requests to simulate inventory changes, then even an inventory like the compacting drawers could play nicely with the auto-crafting system.
SD also has voiding capabilities which is something you called out explicitly.
Okay, I guess it's non-viable if the simulation is long running (greater than one tick). I kind of assumed it was more localized and then the items get captured to crafting storage, in which case the transactions could be terminated by checking what tick you're on and wouldn't need synchronization. I'm a little surprised that item stealing wouldn't be a bigger problem if simulations can get stuck waiting a long time for something to complete.
So are you dropping your IMEInventory entirely in favor of IItemInventory? While it's a MAJOR improvement over IInventory and should even work with deep-slot inventories with a little bit of acrobatics, it's probably a substantial performance lose for inventories like SD which need to introduce virtual slots and would prefer to be treated more like a database than as a slotted inventory system. E.g. SD's internal caching makes a generic insertion or extraction a near-constant operation, but it's irrelevant if the consumer needs to scan the full slot list to determine for itself where it thinks the item should exist.
SD also has voiding capabilities which is something you called out explicitly.
That is not a SD issue, just minecraft/forge being extremely limited in terms of performance for anything more complex than hopper + chest. Just like any other cap it is purely designed to work between two tile entities but not on a global state.
simulation
These are even multithreaded, but use their own local cache taken as snapshot when they start. So actually it does not even reach out to anything and thus not even pass some transaction id along. Just a give me everything you have, maybe I use something of it.
IMEInventory
No these will stay. It is really just the external storage handlers. We essentially replace our system in with the exact same thing now provided by forge. So if SD provides a reasonable IItemHandler it will work out of the box. Maybe not the fastest one but it will work.
I highly doubt that IItemInventory will even scale with or provide the necessary features for AE2. Like range queries across the whole inventory.