Real Filing Cabinet

Real Filing Cabinet

11M Downloads

I bring bad news - Exploits , Performance Issues and Bugs

Funwayguy opened this issue ยท 0 comments

commented

This is probably going to be a long report so I apologise in advance. I did also privately decompile the 1.12.2 code to debug some of this considering the 1.12.2 isn't listed here publicly.

The Discovery

During development of SkyFactory 4, @Darkosto had asked some of us developers to look into a particular Java profiling log to try and find the source of an odd drop in TPS performance. At first everything seemed pretty understandable but there was one place where there was an unusual number of item stacks being created in a time consuming manner. A little digging turned up cabinets were creating items during item insertion by using string names no less. Considering automation deals with 100s of items at a time this was a little concerning to say the least and warranted some deeper digging.

javaw_2018-07-30_11-43-16

A Look Under The Hood

Determined to look into the reasoning behind this, and finding that 1.12.2 code was not public, I took it upon myself to deobfuscate and decompile the mod to get a better idea of how bad the issue was and what could be done to fix it. First impressions were... not great to be honest. Seems that whenever an ItemFolder is queried it spends a considerable amount of time sending the raw name string to the item, block, fluid or entity registry to search for and convert back into an object, sometimes multiple times. Not only is this slow, especially with thousands of registered items and blocks to search through, but it also doesn't make proper use of the registry names in places (more on this later). The getObject() method is also used a number places it probably shouldn't be such as twice for ItemStack tooltips which fires every frame the player is hovering over it in the inventory.

image

Corruption & Exploitation

While sifting through the code and see what could be done to fix this I found something VERY concerning in setObject(). It looks that fluid blocks were using the localised name rather than the registry name. I'm not sure if you're aware of how bad this is for someone who plays with a different language or worse, what happens when someone installs a malicious .lang to manipulate the stored ID names.

javaw_2018-07-30_12-20-51

There also appears to be no attempt to preserve the NBT properties of entities placed in folders. This is important for anything dependent on NBT such as fluid cows, ore sheep or any other entity with stored capability data. This could also be exploited for any entity from another mod that spawns with randomised equipment or properties.

Damage Control

Now I don't want to go full doomsday and say this needs a complete rewrite but it's going take some work. Here's some suggestions on where to start:

  • Unless you're saving/loading from NBT, don't convert the objects in the folders to their registry names. It's much easier and faster to be able to copy/split stacks directly from a reference copy (you can store the actual amount in a separate variable). It also saves you have to look them up for tooltips and other purposes.
  • Please don't use the localised name, unlocalised name, or class names when saving to NBT. Those are subject to change and unreliable.
  • If you've changed to using reference copies of objects, you shouldn't have an issue with entities losing their NBT data. You could probably not have to worry about removing their equipment either because that will be accurately preserved in NBT too.
  • Test this with large scale automation and storage networks, particularly with several auto-crafting machines that search and request several item types at a time before outputting back into the cabinets (lots of I/O stress testing).
  • Just a suggestion but you may want to look into using Java profilers like VisualVM to find hotspots and sources of latency during your testing. Helps a lot in these kinds of cases where it's difficult to find what exactly is slow when it isn't immediately obvious in small scale tests. It's how we stubled upon this rabbit hole.

Misc

MC: 1.12.2
Forge: 2745
Filing Cabinets: 0.1.57

I didn't want to rip on the mod too hard so I haven't gone through all the files but I am on Discord if you want to discuss it more or want some suggestions.