Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

[Warning] Don't use ANY tick limiting #52

Closed
sameer opened this issue Jan 31, 2016 · 17 comments
Closed

[Warning] Don't use ANY tick limiting #52

sameer opened this issue Jan 31, 2016 · 17 comments
Labels

Comments

@sameer
Copy link
Member

sameer commented Jan 31, 2016

TickProfiler is the bane of KCauldron's existence, and as noted by #51 , causes problems. It gives useless results because KCauldron lowers the tickrate for stuff when players are far away

@spannerman79 has brought to my attention the fact that Spigot's tick limiter is the bane of just about every mod's existence.

Alternatives Include:

  • Get Bukkit's ClearLag and occasionally run /lagg killmobs to get rid of mobs
  • Edit settings in tileentities.yml
  • Optimize your mob spawning settings in spigot.yml and bukkit.yml
  • Get a real profiler: VisualVM, YourKit, and WarmRoast are great choices
@sameer sameer added the wontfix label Jan 31, 2016
@sameer
Copy link
Member Author

sameer commented Jan 31, 2016

I just realized I still had it on one of my test servers and found it causing the server to lag with no players online. Again, I highly recommend you get rid of it.

@ghost
Copy link

ghost commented Jan 31, 2016

Did someone come here asking for you to fix the issue with TickProfiler?


EDIT: Nevermind, just read the issue with the person specifying that they're using TickProfiler.

@Yive
Copy link
Contributor

Yive commented Jan 31, 2016

It's recommended to added a patch similar to the one that Spigot did when EchoPets caused it to crash on boot. The class pretty much just stopped the plugin from loading at all.

@spannerman79
Copy link
Contributor

Spigot limits the maximum time a tile-entity can "tick", so no specific tile / entity will lag the server

Its broken http://aikar.co/2015/10/08/spigot-tick-limiter-dont-use-max-tick-time/ don't use it

@sameer
Copy link
Member Author

sameer commented Jan 31, 2016

@spannerman79 Did not realize this updating this post now.

@sameer sameer changed the title [Warning] Don't use TickProfiler [Warning] Don't use TickProfiler OR Spigot Tick-Limiting Jan 31, 2016
@spannerman79
Copy link
Contributor

Not many do, and in fact what @Yive did at 63564ff was correct on every count.

Most don't understand that the settings that was in dispute from @Bogdan-G that it doesn't cut the gameplay and yourself @robotia were in fact spawn rates per connected user, not server overall and other bugs that weren't fixed in Spigot 1.7.10 which we all know that KCauldron uses. Only "easy" way to fix Tick-Limiting is to set max-tick-time.tile & max-tick-time.entity to 1000 - effectively disabling the broken Spigot Tick Limiting.

sameer added a commit that referenced this issue Jan 31, 2016
@spannerman79
Copy link
Contributor

In fact if people want to have a better dynamic tick system take a look at @wildex999 mod TickDynamic https://github.com/wildex999/TickDynamic

@Bogdan-G
Copy link

@spannerman79 I made a change in tick-limiter in his mustache to check == on ! =, as it did in October spigot patch.

@LunNova
Copy link

LunNova commented Feb 6, 2016

Is there a simple way I can reproduce the issue?

Which KCauldron and TickProfiler builds are being used?

@LunNova
Copy link

LunNova commented Feb 6, 2016

The TE list cleanup code here: https://github.com/TCPR/Thermos/blob/master/patches/net/minecraft/world/World.java.patch#L931 is conflicting with TP's hook and resulting in the TE list being emptied if a TE is unloaded while ticking.

If a TE is unloaded while TP profiling is running, that will replace the loaded TE list with an empty list, resulting in everything stopping ticking, matching the report above.

I just realized I still had it on one of my test servers and found it causing the server to lag with no players online. Again, I highly recommend you get rid of it.

I can't replicate slow performance just from having TP installed - don't really understand how that would happen, as TP does almost nothing when not profiling. It only installs hooks when profiling is started. Can you share a sampling snapshot?

Why does the cleanup code copy into a new list, instead of using an iterator and calling iterator.remove() if marker is false? Sorry, that's a bad idea. iterator.remove() on an ArrayList is slow.

edit: I'm quite confused by why isGC/setGC is a thing. How was this benchmarked? Using removeAll the standard way should be faster.

@LunNova
Copy link

LunNova commented Feb 6, 2016

Saw the commit message for 2e86090 explaining reasoning.

Do you know what JRE version you saw poor removeAll performance on? In extremely old versions (anything older than JRE 6u15) it wasn't implemented in ArrayList, so the implementation in AbstractCollection was used.

On any up-to-date JRE it has much better performance, and is definitely faster than creating a new list. See https://bugs.openjdk.java.net/browse/JDK-6529800

edit: Sorry. I incorrectly understood the performance characteristics of ArrayList.removeAll.

If a significant portion (>~15%?)* of the elements in the list are removed, creating the new list would be faster. In other cases it would be slower. A heuristic could be used to get best performance in both cases. I'm not sure why ArrayList.removeAll doesn't do something like this already by creating a new backing array.

if (toRemove.size() > (current.size() / 7) {
    // new list
} else {
   // remove all
}

*15% is an estimate. Actual value likely to be toRemove * log(toRemove) > some constant * current

@ghost
Copy link

ghost commented Feb 6, 2016

Omg, it's @nallar! Hai nallar! :D

@sameer sameer changed the title [Warning] Don't use TickProfiler OR Spigot Tick-Limiting [Warning] Don't use ANY tick limiting Feb 9, 2016
@LunNova
Copy link

LunNova commented Feb 9, 2016

New TickProfiler builds should now work with Thermos. If anyone wants to test, they can add -Dthermos.tickprofiler.disable=false before the -jar in their launch script to allow TickProfiler to load.

@sameer
Copy link
Member Author

sameer commented Feb 9, 2016

Thanks nallar, this is awesome! If it works out, I'll remove the code that prevents it from loading asap.

@ghost
Copy link

ghost commented Feb 9, 2016

@nallar I noticed that you've recently pushed some commits to TickThreading, I thought you dropped TickThreading altogether?

@LunNova
Copy link

LunNova commented Feb 9, 2016

@iKaneki-Ken MinimallyCorrect/TickThreading#1248

@ghost
Copy link

ghost commented Feb 9, 2016

@nallar Oh thank you! And thank you for picking up TickThreading again! 1.7.10 has been hell trying to optimize it. I now actually look forward to actually playing Minecraft 1.8.9 on my server because of this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

5 participants