
Massive Drawer Storage cause tps lag
zxexuxs opened this issue ยท 15 comments
Setup:
11x11 Cube of Storagedrawers, one storage controller in the middle.
MK2 Chassis with itemsink module(default route: yes) and a Provider module Mk2.
Connected to the main network via Inventory system connecter and an ender chest.
If I remove the connection one tick needs ~12ms
With the connection: 200-300ms (Probably only wenn the main network sends item for storage)
This storagecube with AE2 works fine with zero lag(But i only test with a minimal setup)
I can confirm this interaction, though I'm not sure on which side the problem lies, or whether it's worth looking into, as it's a pretty absurd set up....
Anyways, I made the described set-up, and stuck an item sink set to default route against the drawer controller in the middle, then deposited a bunch of random junk in it, proceeding to turn on an infinite stream of gravel depositing into it.
Sure enough, when turned on, the time spent on the tick is nearly 3x higher then when disconnected.
I hooked up jvsualVM to it, and I couldn't really notice much different with the sampler. I was going to attach the profiler to get some good stats, but I have too much stuff in this minecraft instance, and it just froze forever, and eventually crashed.
@1n5aN1aC could you test with this please: https://dl.dropboxusercontent.com/u/33855125/LogisticsPipes/logisticspipes-0.9.3.local.drawer.jar
The AE2 integration was updated a while ago to use a new generator-based accessor API which would avoid searching through every drawer in sequence to place or extract items. Besides performance, I think it would fix the remaining behavioral quirks with LP.
I put in a PR with the changes I think are needed to put it on par with AE2, although I admit my understanding of the LP API contract is fuzzy. I haven't actually tested or built the changes, so I'd appreciate it if someone in this thread could do so and review the behavioral changes. I have no working knowledge of LP systems.
It may improve performance. There's a lot of different operations implemented in storage handler, and not all of them are updated. Searching for items that exist, searching for available space, searching for items to remove, should all complete faster. There is no change to enumerating items, and searching for items that don't exist will only be faster in storage banks that don't have many empty slots.
I agree that creating a massive cube is a bit absurd, though it's not the first I've seen. Any improvement beyond what might be included in this PR would be an unreasonable expectation.
@davboecki thanks for compiling it for me, I figured I was gunna have to do that myself!
I'll check it out in a couple hours when I get home.
Alright, Testing with:
StorageDrawers 1.7.10-1.9.2 (Latest as of this post)
and LP "0.9.3.local.drawer.jar" (Build from @davboecki with PR #921)
Good news is all the SD - LP integration I've tested so far work great.
Bad news is the specific case this issue was talking about doesn't really perform any better than it did before. (5.4ms off) vs. (13.3ms running)
So... I don't know. Sounds like we might want this anyways to unify the interaction more? If so, I could do somewhat more extensive testing. Load it up on a couple of my saves with quite large LP networks (that interface with SD) and see what happens?
Pulling in this change should solve the remaining issues in jaquadro/StorageDrawers#113.
As for performance, I guess I'm a little disappointed the results aren't better for insertion, but I don't know what parts of the storage API are getting utilized. Maybe @davboecki could describe the process that would be applied to this kind of setup?
@1n5aN1aC Do you know how to use VisuaolVM to sample minecraft? If not could you descripe your setup quickly so that I can reproduce the setup and sample it myself to see where the problem is.
Most likely:
- 11x11x11 with a controller centered on one face.
- mk2 pipe with insert/provider moduals
- set up infinite item products going into storage cube.
Yeah, @bookerthegeek is basically right. I just tossed up a giant cube, put a controller in the middle, with a chassis module, put in a item sink default route, and a provider mk2.
Then I tossed up a JABBA / SD with infinite upgrade, and a mk5 chassis, full of mk2 extractors to simulate lots of small inserts, and waited a bit.
Anyways, @davboecki I had attached jvisualVM to it previously (during the original testing) but using just the sampler, I couldn't really tell where it was being spent.... I tried to attach the profiler to get some good stats, but I guess I had too many mods, so it locked up for a looong time, and eventually crashed.
I'll check the new version with the sampler, but unsure if I'll see much.
@jaquadro The performance issue is:
https://github.com/jaquadro/StorageDrawers/blob/master/src/com/jaquadro/minecraft/storagedrawers/block/tile/TileEntityController.java#L615
I can reduce the calls to that method a little bit from LP's side but all ways LP interacts with SD lead to that method as the performance intensive one.
You should probably cache that TE and if you are worried about invalidated TE's just check if the TE is still valid before returning it and if it isn't just check the coordinates for the new TE at that position.
Just as a reference: From the 22ms it took to call getStackInSlot on the controller 19ms belonge to the getTileEntity call.
And of the 3ms our roomForItem methed it took 1.9ms for the getTileEntitiy call.
(Those are not times based on one call but on multiple calls)
The access without caching is especialy bad if you Iterate over the inventory stots by calling getStackInSlot several times. In that time the TE will not change, but you still ask MC for the TE.
I'll see what I can do on the SD side. I never paid much attention to getTileEntity as a bottleneck, but that seems reasonable.
That's why I wanted more insight from you as to what API calls are actually used. I thought eliminating most of the lookup calls in the insert/exists/extract operations could have a big impact, but it's not as helpful if the inventory still gets enumerated frequently or in the same operations. It changes where I need to focus attention, anyway.
if you wana cache the TE and be sure it's still valid that tick you could only retrieve it once every tick and store the last tick you fetched it on so you only fetch it once when something loops through all slots
This 055cc18 will probably solve most of the issues with performance but getTileEntity is still the most performance intentive method.
(https://github.com/jaquadro/StorageDrawers/releases/tag/sd-1.9.3) should avoid most of the getTileEntity calls.