CyclopsMC / IntegratedDynamics

A Minecraft mod to take full and automated control of your appliances.
http://cyclopsmc.github.io/IntegratedDynamics/
MIT License
131 stars 62 forks source link

Lags after update mods #1147

Closed DrSigma4164 closed 2 years ago

DrSigma4164 commented 2 years ago

Issue type:


Short description:

Updated Cyclopscore and Integrateddynamics to the latest version. After that, lags began. That I am not from the first time I go to the portal in the AD, then that I must prove it three times, in particular Ancient Debree, the bench raises ....

Steps to reproduce the problem:

1.Update Cyclops Core and Integrated Dynamics to the latest version.

  1. See lags

Versions:

Profiler output:

I use Spark Mod for profiling, but if you are more convenient to use SamPler mod, tell me it. Spark server profile: https://spark.lucko.me/kEyfN7QLnw Spark client profile: https://spark.lucko.me/vSRVvdkP8g

rubensworks commented 2 years ago

Thanks for reporting!

DrSigma4164 commented 2 years ago

And these lags are more visible on the server part

Speiger commented 2 years ago

Also to append to this. We have a Spark report where Integrated dynamics is very slow.

We have roughly 200 integrated dynamic blocks placed. Nothing really complex just some basic transport. And its eating almost as much as IC2Classic, which has roughly 2.5k TileEntities placed (that tile tick) and have a profiler running through every single one of them.

https://spark.lucko.me/iAkURwaYEH

spark
spark is a performance profiler for Minecraft clients, servers and proxies.

One of the reasons this method is so slow is: https://github.com/CyclopsMC/IntegratedDynamics/blob/master-1.16/src/main/java/org/cyclops/integrateddynamics/core/TickHandler.java#L47

You are copying a HashSet to make it unmodifiable and then iterate over a hashset. And it is Synchronized which has a big overhead too Use LinkedHashSets from FastUtil their iteration performance is a LOT Faster.

Here are benchmarks if you want to see how well fast util performs against java (and my FastUtil clone) https://github.com/Speiger/Primitive-Collections-Benchmarks/blob/master/BENCHMARKS.md

(Yeah I won't look deeper into the mod this looks already horrible)

PoolloverNathan commented 2 years ago

Does this happen on a new world?

DrSigma4164 commented 2 years ago

It seems no, maybe it happens where there are some circuits and cables. But in the world where I saw this error, there were not so many schemes, well, maybe 8 different things, and one sorting room for 9 chests.

Speiger commented 2 years ago

we have 2 people using it, the mod was added after the fact. Roughly 10-12 rightclickers, and 1 small sorting system (1 machine feeding into 3 different chests and 1 storage drawer system which is 8 drawers), and 1 transport system putting items into a machine automatically. So not that much and it takes almost as much lag as an entire mod that was used EVERYWHERE...

The mod has serious performance issues in general, especially since if you have to high complexity it is a straight out server killer...

rubensworks commented 2 years ago

@Speiger Just so you know, but your comments are coming across as really offensive. So I would appreciate it if you would tone it down a bit. Feedback on the mod and general issues are always welcome, but I would appreciate at least a little bit of politeness. I happily fix any issues in my mods, but comments as yours really suck out my motivation.

This mod has been used by many players for many years, and it's the first time I hear someone calling it a "server killer". Performance has always been a priority for me, so this looks like nothing more than a recent regression.

rubensworks commented 2 years ago

Note to self: Screenshot 2022-03-25 at 17 50 19

Speiger commented 2 years ago

Just so you know, but your comments are coming across as really offensive

Oh yeah, I can get really rude if I see really bad code and see comments "Performance is a priority" and when pointing out that the fix so far won't do anything but be a drop on a hot stone (you know what i mean) that "This is enough if you want more implement it yourself" as a comment is really infuriating.

Also your mod falls under the same category as Logistic Pipes, Redpower pipes, Applied Energistics, Refined Storage and any other logistic mod which has complex routing.

All these mods have the potential (quite easily actually) to kill a server, because automatic routing is in general really expensive. I am speaking from experience here.

I mean your 26 megabyte packet that you send over from a server to the client with your mod hardcrashed the server. And the auto reboot system failed, but I am very sure that wasn't because of the packet.

In general the mod is pretty bloated if you just profile the ItemNBT data that exists even if it is thanks to forges ItemCapability system doesn't have to be serailized/deserialized, though that doesn't come free ofcourse. While it is no longer a concern in older mc versions the ItemNBT limit was at 32KB, which would cause a permaban until the item was deleted, which with this mods NBT usage would have been reached easily, nowdays it is at 2MB thanks to shulkerboxes existing.

And yes I am very critical, maybe I am moving into the rude territory. But ever thought of optimizing your NBT usage? For example you can control how much data is synced between server and client (it does all by default)

Or how about you ran a Profiler over your NBT and displayed the actual NBT size of the item. Here is a example: image That tells me how many bytes are actually synced between server and client and how "wasteful" i actually am.

here is the visualization code. (Shows only client side but nothing stops you from implementing a packet for debugging)

if(Screen.hasControlDown() && stack.hasTag())
{
    ByteArrayOutputStream out = new ByteArrayOutputStream();
    try
    {
        CompressedStreamTools.write(stack.getTag(), new DataOutputStream(out));
    }
    catch(Exception ignored)
    {
    }
    if(out.size() > 10000) event.getToolTip().add(new StringTextComponent("NBTSize: "+(out.size()/1000)+" KB").withStyle(TextFormatting.GOLD));
    else event.getToolTip().add(new StringTextComponent("NBTSize: "+out.size()+" Bytes").withStyle(TextFormatting.GOLD));
    Logger logger = (Logger)LogManager.getLogger(CompoundNBT.class);
    Level before = logger.getLevel();
    if(Screen.hasShiftDown()) event.getToolTip().addAll(NBTUtils.convertNBT(stack.getTag()));
    else event.getToolTip().add(stack.getTag().getPrettyDisplay());
    logger.setLevel(before);
}

Same could be implemented in a Block. But you would need to send a packet. But it comes down to the same.

Here is half a Minecraft chest, full of dirt. 1.3KB of Data. image

Here is a Personal Chest (my mod) that is in size a double chest of minecraft. image

I implemented a compression algorythm that trades a tiny bit of CPU time (during save) for massive compression improvements that in its worst case store exactly as much data as minecrafts chest.

Also a CPU profiler could be implemented into your mod showing the lag that you cause in each step. Maybe per networking grid so you can isolate grids.

Right now IC2C you press the "hud Key (X)" and you get a profiler that shows you:

javaw_ecrx4ujzrL

This is my test world and i have roughly 100-200 machines placed down. Which is barely anything. But every player can see in a press of a button what cause the lag. Yes they have to look at it but they don't have to look at a confusing tool or anything to see what causes the lag.

This isn't performance free but that allowed ME to see how laggy each setup is.

You say you care about performance? Now take all I gave you just now and make sure you hold up to your word. Because all this is more then enough to go through your own mod and profile the heck out of and find problem spots.

I know I am rough in some areas. But I have stuff to show for it.

Speiger commented 2 years ago

Huh, Major props for you. I assume you missclicked.

DrSigma4164 commented 2 years ago

One of the reasons why I lagged was that I connected the 0 channel to the box controller with StorageDrawers mode interface, and also to the same controller pao the same channel connected the exporter, just this system was part of the sorting system and I forgot to pick up the interface after how I changed the sorting algorithm))))

DrSigma4164 commented 2 years ago

Let's present a situation: there is a chest to which the interface and the extractor are connected, and there are one more chests simply with the interface. Everything is tuned to channel 0 and the extractor pulls everything out in a row. In this case, it will lag (as I said in a previous post). Is it possible to make this extractor not able to extract things from the box to which it is connected and the interface, because things do not move from that box to the same and will lag. That is, when, for example, a list of items to be transferred is formed, this box to be transferred to will not take into account the interface that is connected to the same box.

I hope he expressed his opinion normally. If you do not understand, then tell about it.

rubensworks commented 2 years ago

Note to self: this recent change might also be related to the issue: https://github.com/CyclopsMC/IntegratedDynamics/commit/d2246651e570373700abb8b45ee5de2165709bbc

@DoctorSigma Can you confirm that downgrading to 1.10.7 resolves your lag? If so, then I'll remove (or hide behind flag) this specific change.

DrSigma4164 commented 2 years ago

When I lowered the version to what you said, nothing changed, but what I wrote above helped, that is, I took from one box the interface tuned to the same channel with the extractor, which was directed to the same chest. That is, my extractor wanted to shove into the chest what was in this chest. I drew in paint what it looked like: image

rubensworks commented 2 years ago

@DoctorSigma Thanks for checking. So the problem is not related to the update then, so that's good.

Your picture seems to indicate a loop, so that may indeed explain the TPS impact. In any case, there are some optimizations possible there, so that the TPS impact is lowered. Your spark log will definitely help with that. Thanks for all the input!

DrSigma4164 commented 2 years ago

Please. I really like your mod. You can do a lot with it. For example, I made a scheme that tracks me when allowed in the mine and helps clean inventory, and the like. Thanks for the mod)

Geo25rey commented 2 years ago

Hey @rubensworks! First, I want to thank you for making this amazing and complex mod. @Speiger is wrong to be outstandingly rude to you. He obviously has a lot to learn in terms of people skills.

I have been working with semi-large networks stressing what Integrated Dynamics and Tunnels can do. Out of curiosity, I ran a profile on my world and it does in-fact seem that there can be big performance improves by just changing a few methods. Here is my profile.

I am more than willing to submit some PRs for any performance improvements I can make on my own. I just wanted to try and prompt a discussion first as you recommend in your README.

By the way, same versions as @DoctorSigma

Speiger commented 2 years ago

@Geo25rey just to point out, if a mod creates almost as much lag but just got started used, compared to a mod that was being actively used on a server that ran for 1.5 months at that time and also INCLUDES a transport/request system, then you have a giant problem.

I mean the mod (IC2C) that I compare it against is having a Fluid/Item/Electric Transport Grid, Mining Equipment, Hundreds of Machines running, A couple thousand Crops that use TileEntityTicks to grow etc.

And then this giant mod (IC2C) is only 600-1000ms (out of 30.000ms) slower then a mod that just got started getting used (Integrated Dynamics) and was barely used in comparison.

And then you have a ModDev that says "Performance is imporant to us" but when pointed out what they have done is not even a drop in a bucket you get this "You fix it".

Calling it "caring" is something different.

About people skills, oh yeah you are right, I am not a nice person, but I have at least the facts to back up my claims. And that the dev never heard that his mod is a server killer (even so modpacks warn about this mod causing performance issues) shows only 2 things, either living under a rock (which is at least reasonable), or ignorance.

And receiving i assume a "Oops Ban" for providing ways to improve performance/showing performance problem spots, is not showing a nice light on him either.

Edit: lets see how long it takes to either receive a ban or have this comment closed so NOBODY sees it xD

rubensworks commented 2 years ago

Thanks for the profile @Geo25rey, I'll have a more in-depth look at it once I find some time for it. Do you have any specific improvements in mind already? If so, feel free to open a dedicated issue where we can discuss them.


@Speiger please keep this discussion on the topic of this issue (see the issue title). I'm marking your comment as off-topic. I would prefer not to ban people, but I will if I have to continue marking your comments as off-topic.

Speiger commented 2 years ago

@rubensworks so far the topic was about performance and our actions regarding that. Right on point. And the comments you were marking as "offtopic" were suggestions how you can display "performance problems" to the end user that they can give you more information what actually causes the issue, but it looks like that is "to offtopic" for you. Performance is not only limited to CPU Cycles by the way, Ram Usage/PCIExpress Usage also factor into it.

rubensworks commented 2 years ago

I'm sorry @Speiger, blocking you due to your hostile/condescending comments and continuously moving this issue off-topic. You can still e-mail me in case you need to contact me.

Just as an FYI to you directly, I know a thing or two about performance (have a look at my resume). Not implementing all performance improvements doesn't mean I don't care about them. I just don't have enough time to look into everything (unfortunately). I have to prioritize my time for issues that I think will benefit the majority of people. And that does unfortunately mean that some issues will remain (partially) unhandled. If I had all the time in the world, I would be more than happy to invest much more time into this mod's development. But since I have many more obligations than just modding (such as earning enough money to bring food to the table for my family), this is not an option unfortunately.

rubensworks commented 2 years ago

@DoctorSigma I've analyzed your profile more in-depth, and it looks like the TPS issue was caused by a single IT importer (as you've said). The loop that was created in that build basically causes items to be moved each tick, which in its turn causes IT's internal caches to be invalidated, which requires re-indexing that chest each tick.

Since these cases are probably always due to a player error, it seems unnecessary to optimize these cases. Instead, I think it's more productive to restore the network diagnostics. As a bonus, I can look into adding a system that detects parts that have excessive tick times, and somehow throttles those and reports them to the player.

Feel free to add additional comments if you think I've missed anything here in my analysis.

DrSigma4164 commented 2 years ago

As for a system that detects parts that have excessive time, and somehow slows them down and communicates them to the player - this is a great idea. And it will be very useful to know what exactly is lagging in the system.