Cyclops Core

Cyclops Core

93M Downloads

CapabilityConstructorRegistry performance

sfPlayer1 opened this issue ยท 21 comments

commented

The map lookup in org.cyclops.cyclopscore.modcompat.capabilities.CapabilityConstructorRegistry.onLoad dominates the time spent in ItemStack copies with > 99.9%. With other mods doing huge quantities of ItemStack.copy() calls this becomes a major contributor to server lag as observed with SkyFactory3 + refined storage, 6.3 ms per tick on average. In other words doing any work of significance in AttachCapabilitiesEvent.Item will greatly affect the footprint of an originally very cheap method call.

With a quick search I did't see any use for CapabilityConstructorRegistry's indirection that can't be replaced by direct capability instantiation on the item. The only item using it is apparently owned by CyclopsMC.

Getting rid of CapabilityConstructorRegistry is by far the best solution for the issue, alternatively IdentityHashMap<Class<...>, List<...>> wouldn't be quite as bad as the current Multimap.

commented

I wasn't aware of the performance issue, thanks for reporting this!

Removal of CapabilityConstructorRegistry is however not a solution.
I'll look into improving the lookup efficiency of the Multimaps, an identity-based mapping should indeed already help a lot.

commented

Note to self for microbenchmark results when copying 10M ItemStacks:

ImmutableMultimap: ~1500ms (original datastructure)
HashMultimap: ~5000ms
IdentityMultimap (with IdentityHashSet for keys): ~5500ms
IdentityHashMap (with ArrayList for values): ~1400ms (new datastructure)

This change slightly improves efficiency.

@sfPlayer1 How exactly did you benchmark this? Can I reproduce this?
If you want, you could try the latest dev build to see if this change results in any major performance boost for your test case.
https://dl.bintray.com/cyclopsmc/dev/org/cyclops/cyclopscore/CyclopsCore/1.9.4-0.9.0-468/CyclopsCore-1.9.4-0.9.0-468.jar

commented

Cool, I can test this in SF3 if needed. I've been given a few crash reports that are related to this.

commented

@Darkosto Could you send me those crash logs? This should only be a performance issue, if it crashes, something else is probably wrong.

commented

http://paste.feed-the-beast.com/view/c23e7247

What is the bug? My sub server was at 1 tps and when we got rid of all the colossal chests, it went up to 10. Colossal chests also crash the server often when people have them hooked up to Refined Storage systems

commented

@Darkosto This doesn't look related to CyclopsCore, @raoulvdberge it looks like RS is attempting to insert an item for a null item handler capability?

commented

That's not really a crash, but a stall for > 60s, so essentially also a performance issue. The stack trace is from MC's watch dog, which can be disabled in server.properties maxTickTime=-1 or similar.

commented

@Darkosto The Colossal Chests inefficiency is related to #41, waiting on @raoulvdberge to implement the optimization I provide.

commented

@rubensworks
I'm not sure if your optimization is a good enough solution. There will be also lag with for example EiO conduits. Those use ItemHandlerHelper#insertItem as well which will loop over every possible slot too, causing the same performance issues as RS is having.

commented

Correction: ItemHandlerHelper#insertItem will short-circuit as soon as it finds an empty slot, so that might not cause perf issues. But other mods can still iterate over any slot and still cause the perf issue as well.

commented

What I'm getting at is that it might be better to not include unused slots in IItemHandler#getSlots.

commented

@raoulvdberge The optimization is only good for the continuous change detection in RS networks. For the item insertion you're right, but for that there's not much I can do, the large amount of slots is simply an inherent property of Colossal Chests.

commented

@rubensworks
External Storage is not the only type of block doing this. Let's take the importer. The importer has to go over every slot to extract the items from the chest. It needs to do this because it has to know the slots to extract from for the whitelist/blacklist.

commented

The data was from live observations on regular SkyFactory 3 gameplay, not sure which setup specifically reproduces it since the world is already quite developed. Presumably large colossal chests with lots of different(?) contents connected to refined storage, which I guess copies the inventory contents for change detection. Unfortunately it doesn't show too consistently, so it's hard to compare.

commented

I'll have a look to see if I can recreate such a context myself.

@raoulvdberge Do you by any chance provide an easy method for adding a large amount of items to RS networks for debugging?

commented

@rubensworks Give yourself a storage disk with meta 5.

commented

You do need to test with an external storage though, normal RS storages don't do any change detection and don't cause stack copying.

commented

@Darkosto The latest dev build reduces the overhead of the CapabilityConstructorRegistry, which should remove constant server lag in RS networks.
I tested this in a dev environment with only RS and CyclopsCore loaded, could you have a look to see if this has any positive impact in realistic SkyFactory3 worlds?

Note that this will not fix the Refined Storage-Colossal Chests lag problem, this is a different issue. @raoulvdberge and I should sit down together to figure out what the best solution would be for that non-trivial issue.

commented

So far so good, I ran it on a server last night. I'm going to keep messing with it and see how well it does :) Thank you!

commented

I've identified an unused capability that produced unecessary overhead in the CapabilityConstructorRegistry , which will be removed in the next CommonCapabilities update: CyclopsMC/CommonCapabilities@877de02

commented

CyclopsCore 0.9.1 has been released, which contains performance improvements related to this issue.
In case further performance problems would arise related to this, please let me know here.