Making this issue based issues a streamer playing the sky factory 3 modpack.
https://www.feed-the-beast.com/projects/ftb-presents-skyfactory-3

Lag Issue:

• Lag when importing into a 7x7 and 8x8
• https://www.twitch.tv/sevadus/v/113311965?t=06h54m00s
• Profiling with: https://github.com/nallar/TickProfiler

Other issues mentioned:

• Vein mining will cause diamond blocks to turn into iron blocks
• Using non-vanilla pick-axe causes lose of memory of items

Not sure if this is something that can be optimized or fixed.

Don't think this is single player only. Servers I heard are having issues as well.

Some extra results from tests:
LAGGY:
Refined Storage (Importer importing into the system, with a storage interface putting the items into the chest)
Translocators Item Translocators

NOT LAGGY:
ExtraUtils2 Item Transfer Node (Max upgraded)
Vanilla Hopper
EnderIO Item Conduit (Speed upgrades were not tested)

Chances are small that this can be fixed on our end, but I can try to come up with something in the future.

If it would be changed so that it only returns non-full slots, this would mean that mods wouldn't be able to see all stacks in an inventory anymore usinggetStackInSlot.

It would be changed so it only returns non-empty slots.

Furthermore, while insertion could be (slightly) improved, extraction would not work properly anymore, since all full slots would be excluded.

Full slots remain. Only empty slots will be ignored.

@raoulvdberge Ok, but in that case, how would an item transfer algorithm be able to insert items into fully empty slots? Because it would be able to see it.

You'd keep 1 empty slot available at all times.

@raoulvdberge That could indeed work.

There's still the issue with a lot of non-full, non-empty slots though. Let's say you have 1M slots with 32 non-egg items, and slot 1M+1 has 32 eggs. When a single egg would need to be inserted, an algorithm would still need to go over each of the 1M slots to check if the egg can be inserted in there.
I think, on average, this is the most typical case. So while your suggestion would indeed improve the performance in a way, it will still scale quite badly, unless the chest is very empty or very full.

That is something you can improve in your implementation: e.g. store a Map<Item, List<Integer>> to see which slots allow said item. I'm not sure if something like that would work, though.

@raoulvdberge That's indeed the problem. The current IItemHandler makes it only possible for the item handler interface consumer to implement such an optimization, which is not ideal because inventory contents might be volatile, and the mapping might get invalidated at any time.

I would have no way to expose such a mapping without adding a new method through a new capability, for instance.

Hm, I see.

Another option is to completely get rid of slot based storage of items. Instead, store a Multimap<Item, ItemStack>. It'd be easy to write a IItemHandler wrapper for that. Making it work with MC's container system might be more difficult, though.

@raoulvdberge Indeed, internally that could be a good way of handling it. I do however not think it would be good to force this datastructure on other mods, which is why the following methods as a simplified form of the IItemHandler might be prefered:

ItemStack insertItem(ItemStack stack, boolean simulate);
ItemStack extractItem(int amount, boolean simulate);
ItemStack extractItem(ItemStack matchStack, int matchFlags, boolean simulate);

These can easily implemented on top of an Multimap<Item, ItemStack>. (except for the extractItem matchFlags which could require special handling for certain flags)

I do however not think it would be good to force this datastructure on other mods

Ideally, that solution would be abstracted away with an IItemHandler.

While I like your suggested additions, I'd very much prefer a IItemHandler-only solution.

@raoulvdberge It should be doable to wrap such an interface into an IItemHandler, but not without loss of performance I think.
I'm currently working on implementing a wrapper the other way around, so putting an IItemHandler inside an ISlotlessItemHandler (yes, I just named it :p), this should only give an increase in performance.
I'll ping you once I have something working, might not be for today anymore though.

In its current state this mod is unusable in a modpack. It interacts extremely poorly with other mods and causes significant TPS lag for servers. There has to be something done about this if you want any chance of someone to use it.

The main problem here is that item handler capabilities are slot-based. And since Colossal Chests has a huge amount of slots, this does not scale well for this type of interface.

Ideally, I would provide a new type of capability that is not slot-based anymore, but provides some smarter indexing mechanism. This would however require all item transfer mods to support this type of interface as well, which may be problematic.

@raoulvdberge I guess you also think that something like this is needed. If you agree with this idea, we should sit down and figure out a new type of capability soon, which still provides the same kinds of item handling.

@rubensworks

I was thinking about this, and while it will solve the external storage issues the problems with other mods will remain.

Lots of mods use slot iteration with IItemHandler#getSlots. The importer does this for example to know from what slots to extract according to the whitelist.

While such a special capability would solve the issue, I can't modify all the places in the RS codebase to use the special cap instead of getSlots.

We could also have a wrapper for the special capability, that still allows the usage of getSlots, but uses the smarter indexing mechanism instead.

But again, RS can use the wrapper, but other mods will still remain problematic.

One fix I suggest is to limit the number of slots returned in getSlots to match the actual "filled slots", and not include any empty slots. This would be an easy fix at first sight, but I don't know the actual codebase well enough.

Okay, I'm not trying to sound like a jerk on this, but that reply/message does not actually help when trying to pin down the exact issues. Saying it's unusable in a modpack is not completely true because I have used it in my worlds with hardly any issues. @rubensworks has actually done a lot of work so far in fixing these lag and performance issues and continues to do so.

It's also not always colossal chests that causes the issues you see with mod integration, it is sometimes the fault of the other mods and can only be fixed on their end. The dev has been working tirelessly to fix the mod, for free I might add, and improve interactions between his mod and other mods (as much as he can do on his end anyways). Keep in mind, mod integration is a two way street and it's not always Colossal Chests' fault.

A lot of the TPS server lag has been fixed, I am currently testing out development versions myself and know that it is much improved.

There are a lot of people who like to call out "Colossal Chests is hurting my server and it's awful" but I rarely see detailed reports, profiles, samples, etc. to help him locate the issues. If someone comes in here and reports "Mod A is not working with other mods", how would you expect him to know where to even begin? There are thousands of mods out there, it's literally impossible for him to find and fix all of those things alone. The very least anyone can do to help is to list out which mods/items/etc. are seemingly causing issues and report them here. Don't just call out vague issues and expect him to even know where in the thousands of lines of code they are.

@drekryan You may not vouch for the dev or the mod based on your personal experience. But, I can. I've been making modpacks and interacting for almost 3 years now and I'm going to say @rubensworks is a solid dev who works hard to improve his product. He communicates with other devs to find and resolve the issue and is willing to be a team-player. Sending someone negative messages like "In its current state this mod is unusable in a modpack" is not helpful and a very good way to discourage someone from modding. If you'd like this community, and specifically @rubensworks, to be happy and positive about his mod and making fixes then you need to give constructive feedback and criticism. If people told me day in and day out "your modpacks are terrible, they're unusable" I wouldn't be making modpacks for very long because there's not feedback on how to improve or what people are having issues with.

This community needs proactive, positive people reporting bugs and helping each other out. Not to call out devs because their mod sucks and expect them to fix it without reporting good details. Any time I have had issues with a mod, I have reached out to the dev. Sometimes they say no, but the majority of the time they say yes and are happy to fix it.

Again, I'm not trying to be a jerk. I'm just letting you know the community doesn't grow with those types of reports and communication. It grows when we help each other out and give the information needed to improve and relaying positivity to developers who are working their butts off. The only payment they get is the good feeling when someone enjoys their mod.

Just put yourself in that situation, if people told you what you were doing was bad but never offered information as to what you were doing "wrong".

@raoulvdberge You're right, other mods will still be less efficient when they don't support the capability. Unfortunately there's nothing I can do about it (please tell me if I'm missing something), unless I drastically reduce the number of slots per chest, which would defeat the purpose of the mod ;-)

The wrapper you suggest would have to be statefull if it would do internal indexing, which may or may not be a problem, this is something that needs to be investigated. It all depends if we want to put the effort of indexing on the storage or item handling end.

Changing the number of slots in getSlots won't work unfortunately, because it only returns a single number, not a list of slots. So in cases where the slot fill state is fragmented, this will not work.

I was thinking of an interface along the lines of:

ItemStack insertItem(ItemStack stack, boolean simulate);
ItemStack extractItem(int amount, boolean simulate);
ItemStack extractItem(ItemStack matchStack, int matchFlags, boolean simulate);

This should be sufficient for most item transfer mods to interact with item storage, as far as I can see.
The default IItemHandler capability would still remain available as a fallback, for the cases where a complete overview of the inventory would be required for instance.

Changing the number of slots in getSlots won't work unfortunately, because it only returns a single number, not a list of slots. So in cases where the slot fill state is fragmented, this will not work.

You can change the methods in IItemHandler to use the new index, based on the ommited slots.

@raoulvdberge Changing the semantics like that could be quite dangerous. Some mods might do some internal caching of slots, which could lead to broken item transfer algorithms, because slot x in tick i, may not be the same slot anymore in tick i+1 because of that change.

Eitherway, such a change could indeed improve efficiency in some cases, but not for the majority of cases as far as I can see. If all slots of a chest for instance would be half full, an item transfer algorithm might still need to iterate over all slots.

I understand the chest has ALOT of slots. I'm not a mod developer here so I can't begin to offer suggestions to fixes however I know mods such as refined storage or applied energistics have a lot of storage, slots, whatever you want to call it without affecting server performance significantly (so maybe look into something similar to that). I can and will provide the results from my server here later today so you can look at them but the cause of the performance issues is definitely what is stated on the can in this issue. Importing and exporting to these chests can take anywhere from 16-30ms bringing server TPS low enough to cause the server to crash. I have tried Refined Storage cables and item translocators from a system working with a lot of items at a time (Chickens or Resource Crop farms). Also of issue but may be related is the performance costs of trying to access the chests inventory from a crafting interface or shift-click crafting from one.

I would find it hard if you are unable to reproduce this in some way but if that is the case I would be happy to upload a backup of an affected world upon request.

I'm not meaning to sound like a lot of work hasn't gone into the mod or the development of the packs it is in but it is a huge let down to hear a reply similar to "I don't think much can be done" when you are out looking for solutions to the problem. I just hate to see this brushed aside.

What about "sharding" the chest and requiring that on each level you need to have an equal amount of import / export points? So each inventory would be internally a large inventory, but to outside mods, it would be 6 smaller inventories for a 6x6 and 10 inventories for a 10x10 size chest.

This could allow for the mods to almost have a "async" nature when doing search patterns.

@drekryan

I'm not meaning to sound like a lot of work hasn't gone into the mod or the development of the packs it is in but it is a huge let down to hear a reply similar to "I don't think much can be done" when you are out looking for solutions to the problem. I just hate to see this brushed aside.

If you were a programmer, you would know how hard this problem is to fix actually. Making assumptions like that is usually very rude.

@rubensworks

@raoulvdberge Changing the semantics like that could be quite dangerous. Some mods might do some internal caching of slots, which could lead to broken item transfer algorithms, because slot x in tick i, may not be the same slot anymore in tick i+1 because of that change.

That is not our problem. Slots can change, caches need to expect that. The Refined Storage external storage cache handles it correctly, for example.

Eitherway, such a change could indeed improve efficiency in some cases, but not for the majority of cases as far as I can see.

To be honest, according to me this should fix all issues as you're limiting the amount of actual slots, it'll have to iterate over 1000 slots instead of 100 000 for example.

If all slots of a chest for instance would be half full, an item transfer algorithm might still need to iterate over all slots.

Why? The item transfer algo will use the capability. It shouldn't care about empty slots.

@raoulvdberge I still don't see how changing the semantics of getSlots() could work without breaking a lot of things. If it would be changed so that it only returns non-full slots, this would mean that mods wouldn't be able to see all stacks in an inventory anymore usinggetStackInSlot. Furthermore, while insertion could be (slightly) improved, extraction would not work properly anymore, since all full slots would be excluded.

If I were to implement the capability I mentioned before, would you be willing to support it in RS (possibly as a wrapper)? I could make an initial example implementation in Integrated Tunnels as a proof-of-concept, possibly with some scalability tests. I think this would be the only sufficient generic solution that will remove the performance problems in all cases.

@raoulvdberge I just implemented everything required for using the ISlotlessItemHandler.
More details on its usage can be found here: https://github.com/CyclopsMC/CommonCapabilitiesAPI/wiki/Slotless-Item-Handler

The latest dev build of CyclopsMC/CommonCapabilities@2ebd61c provides this new capability.
CyclopsMC/CyclopsCore@f01eb2a and 4bf524e have also been update in-dev to provide an efficient implementation of this capability.
CyclopsMC/IntegratedTunnels@034c933 has been updated as well to make use of this capability.

Relevant dev builds:
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/commoncapabilities/CommonCapabilities/1.9.4-1.2.2-74/
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/cyclopscore/CyclopsCore/1.9.4-0.9.0-470/
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/colossalchests/ColossalChests/1.10.2-1.4.2-141/
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/integratedtunnels/IntegratedTunnels/1.10.2-1.0.3-35/

This, together with the IInventoryState capability should make RS<->CC interaction highly efficient.

Like I said, while I could patch the external storage to use this instead of an IItemHandler, I'm not planning to change importers or other code that references IItemHandler#getSlots, since that's way too many work. Thus, the issue persists. Why couldn't you make them an IItemHandler under the hood? I'm sure it's possible to do without performance penalties.

@raoulvdberge Because the whole problem is that IItemHandler is slot-based, which does not scale for large inventories. ISlotlessItemHandler can wrap around IItemHandler, but not the other way around, since the former is more generic.

This is not the change I had in mind, it requires loads of patching code. It is possible to still have an IItemHandler that still uses slots, but is fast. With a "slotless approach" I meant that you wouldn't care about slots internally, but still handle them for the interface.

@raoulvdberge Could you provide me with such an efficient IItemHandler implementation that wraps around an ISlotlessItemHandler then? Because I don't see how it can be done.

There is a naive implementation of such a wrapper,
but as you can see, this will result in exactly the same scalability issues as those we are experiencing now.

I'm not convinced that this would require too many changes though. With the right abstractions, this implementation should be trivial. But I don't know the RS codebase very well, so perhaps I'm missing something.

If you want, we can discuss this further on IRC this evening. That might be easier.

@raoulvdberge I plan to release these changes this weekend, so if you need any other changes from my end, please let me know in time.

I'll look into changing my code now. I'm not at all happy with me having to patch parts of the code that normally work perfectly fine though...

@raoulvdberge Is that required to patch parts of your code though? I don't know exactly what the RS codebase looks like, but I thought @way2muchnoise said this could be done as an independent modcompat component.

It can't.

@raoulvdberge Perhaps some new abstractions in your code could help? I'd be happy to help you implementing this if needed. I just really want to resolve this performance issue asap, so let me know if there's any way I could help.

I'm doing another 1.10 release in an hr or so, the release after that will have the changes so I can start with a clean slate.

Heya @rubensworks ,

Just want to catch up real quickly with you on this. I see you've located the translocators issue and figured out a solution. Will this help with the Actually Additions laser relay as well? I believe the issue is it's connecting to every single inventory slot at the same time? Let me know if there's any more information that will help out :)

Darkosto

@Darkosto All item transfer mods can definitely benefit from this new capability.
I just opened an issue at the Actually Additions issue tracker regarding this.

If you discover any other item transfer mods having performance issues with Colossal Chests, be sure to let me know, so I can contact the mod authors to implement support for this capability :-)

Awesome! Thank you very much!! I have a pack update coming out next week, I can't wait to get the players something even better :)

@Darkosto Note that at the moment, the only mod supporting this new capability (AFAIK) is Integrated Tunnels, so only for that mod the performance increase will be apparent.
But the new capability is very simple, so hopefully other modders will start adopting it soon as well.

@raoulvdberge any update on the implementation?

@Runesmacher the implementation has been in since 1.2.19

Awesome :D so we can close this issue then? @Darkosto You can update the pack now :D i would also sugest that we release a updated integrated dynamics with the fix for the gui not opening on servers.

@Runesmacher I want to fix some other issues first before making a new release, but in the meantime I'll close this one.

Yup @Runesmacher , I'm also waiting on a couple of other fixes or else the pack would have been updated by now. I didn't want to put 2 updates out in close proximity. Thanks for the heads up though!