Out Of Memory
Mattabase opened this issue · 29 comments
@raoulvdberge I have two ideas for a chest change capability:
Using the observer pattern:
public interface IInventoryChangeSubject {
public void addObserver(IInventoryChangeObserver observer);
public void removeObserver(IInventoryChangeObserver observer);
public Collection<IInventoryChangeObserver> getObservers();
public static class IInventoryChangeObserver {
public void onChange();
}
}
Using a simple hash:
public interface IInventoryState {
/**
* This method must be able to calculate the hash very quickly.
* @return The hash representing the current state of the inventory.
*/
public int getHash();
}
I think the former will be more efficient, but possibly more complicated to handle in the context of chunk (un)loading.
The latter will still require you to get the capability and call that method every tick, but the hash value is pre-calculated, so it should be pretty fast.
Which of the two do you think would be better?
The former is much more clean, but like you said, it might cause issues with observers resetting when the chunk unloads/loads. So, I'd opt for the latter.
That is strange...
Does this crash happen every time the server is started? Or is this a random occurence?
It might just be the server running out of memory.
I see Refined Storage in the stacktrace, @raoulvdberge, is this something that could be caused from your end?
This will probably be related: refinedmods/refinedstorage#325
It happens over time, but it took 3ish hours to consume 6GB of Ram and crash the server.
RS ext. storages do a slot lookup every tick to calculate the items stored... is this slow / memory intensive with Colossal Chests?
because of the inv size of Colossal Chests this will become very slow / memory intensive you might want to thread that or only re scan the inv on inv change?
@raoulvdberge It shouldn't be slow no, it's a simple array lookup, so that should happen in O(1).
I could provide an API for these chests that would allow you to listen to chest changes, this might improve your scanning algorithm.
From the stacktrace I can see that RS connects to a chest interface instead of the chest core. This should still be sufficiently fast, but I'll have a look if I can further optimize this.
@Mattabase Could you try connecting your RS storage bus directly to the colossal chest core instead of the interface? If that doesn't solve the problem, then we know for sure that the RS inv scanning is causing the problem.
I'm still wondering how a heap size out of memory error can happen, as RS doesn't allocate any memory when calculating the stored count
Did some small changes that should improve performance.
You can try the dev build here: https://dl.bintray.com/cyclopsmc/dev/org/cyclops/colossalchests/ColossalChests/1.9.4-1.3.9-95/ColossalChests-1.9.4-1.3.9-95.jar
Make sure to backup before using this.
I still think the main bottleneck is RS inventory scanning though. So @raoulvdberge, I suggest that we discuss this on IRC sometime soon. (Not today anymore though ;-) )
Note to self: Add capability to get a hash of the current state of the chest. This will make it possible to quickly detect if something has been changed in the inventory.
@raoulvdberge
I've implemented the second approach as a capability here: CyclopsMC/CommonCapabilitiesAPI@81d9390
I've added it to the Common Capabilities API instead of Colossal Chests, to make it possible for other modders to easily use it as well.
The API can be added as a git submodule (as is explain in the repo's README).
This capability is registered when the Common Capabilities mod is present.
This capability will automatically be present for all inventories in all my mods: CyclopsMC/CyclopsCore@f981b67
So you can use the latest CyclopsCore dev build with the latest public Colossal Chests release to test this.
Let me know if there's anything else you need.
@raoulvdberge What's the status on this? Do you need any help with the usage of the capability?
Still have to begin working on it. We've been really busy with autocrafting this version, so it's not that high of a priority.
@Reko420 This should actually be fixed with the latest RS and CC versions.
Any more information you can give?
@raoulvdberge What was your reason for removing it? Did it cause any issues?
It didn't support ore dictionary extraction, so it still fell back to slow extraction for autocrafting for example.
It also made the codebase overall much uglier, because I had to special case for Cyclops everywhere, while it wasn't even that much of a used external storage.
It also didn't have any methods to get a list of items in the storage, leading to a dependency on CyclopsCore.
Due to this dependency, normal slotless item handlers were also treated as CyclopsCore tiles, causing crashes, and special casing those would even add more code bloat.
It didn't support ore dictionary extraction, so it still fell back to slow extraction for autocrafting for example.
Perhaps I should look into making some kind of capability-based item handler. So you could request access based on certain indexing types, such as oredict, NBT, item, ... Will think about this a bit.
It also didn't have any methods to get a list of items in the storage
For this, you would have to fallback to the regular item handlers, which Colossal Chests also expose.
normal slotless item handlers were also treated as CyclopsCore tiles
Ah yes, this is not a guarantee ;-)
I think basic support for slotless item handler would still be useful, as it's the best we currently have at the moment. And issues will start flowing in when people will start using RS and CC again in 1.12 packs. But of course, there is always room for improvement. I'll try to think a bit on a good solution that is expressive and flexible enough but not too difficult to implement and support.