A For loop can cause lag

Discussion in 'Spigot Plugin Development' started by JuanDouCore_, Aug 6, 2018.

  1. I was in doubt while creating my plugin.
    If I create the following loop:
    Code (Java):
    for(String name : players) {
    if(Bukkit.getServer().getPlayer(name) != null {
            //rest
        }
    }
    As you will see the loop extract Strings from a List, and then a check if the string is a connected player, then the process happens.
    Now my question is, if this verification happens with a List that has even 200 lines (that is, it will have to verify 200 lines) plus some players connected to the server (Like 100) this could cause some kind of lag?
     
  2. If it has a lot of checks what take a lot of time (like 1MS), Yes. But if just some simple if dtatements and tp’ing no
     
  3. Could you explain me better that you recommend me to do?
     
  4. Praya

    Benefactor

    lag or not is depends on your code.
    but if you want to loop all online players, better to use Bukkit.getOnlinePlayers()
     
  5. Praya

    Benefactor

  6. Would it be better to do so?
    Code (Java):
    for(Player p : Bukkit.getOnlinePlayers() {
       if(votantes.contains(p.getName()) {
          //rest
        }
     
  7. No, it is created only once
     
  8. like it might cause a small lag spike, but it'd be hard to notice it imo
     
  9. Praya

    Benefactor

    yes

    if only created once, it will not cause lag
     
  10. The best way to find out would be to measure how long it takes to run the method (let's say) 2000 times, and if it's small/tiny, it's ok.
    I can't really give you a recommendation, but keep in mind a tick is 50ms, it should definitely be shorter than that.
     
    • Optimistic Optimistic x 1
    • You should use if else rather than if, or even just create an array of Runnable so you don't have to do the if statements.
    • Why create Title instances over and over?
    • Name lookup is practically the same as looping over the online players and checking the name. So that is an O(n * m) operation (where n = votantes size and m = online player count). If you make votantes a Set and iterate over the Players (and check if they're in the Set), that would make it an O(m) operation, so much faster.
    • Use replace rather than replaceAll for literal replacements. replaceAll is for regex, and much slower.
    • Use a collection of UUIDs rather than a collection of Strings.
    • Nit: name your enclosing class Home rather than home, as PascalCase is the standard for class names.
     
    • Like Like x 1
    • Agree Agree x 1
  11. please keep in mind, servers are mostly running on good Computers, not on Potatos. If you'r not using a big loop every tick it should not cause any noticeable lag oder tps change.
     
  12. 300 entries is nothing for most computers, unless you're doing something really expensive that you did not show in the snippet above
     
    • Agree Agree x 1
  13. So we should deliberately write inefficient code and just rely on technology to be good enough so it doesn't matter?
    While one inefficient path in a plugin may not be an issue. However, when you start adding these paths together, across multiple plugins then you get "system level" performance issues.
    I'm not advocating premature optimization, or sacrificing readability over performance but I do not think it's something any programmer should simply ignore.
     
  14. ***EXPLOSION*** He was just saying that looping through all the online players shouldn't cause lag as we expect servers to run on decent hardware.
     
  15. You should consider to find a way to narrow down your player scope/iteration slice (example, iterate over 1/20th of the list & run a modulo operation to trim the number.) further more, not sure if you already did so, but i would consider to switch your string list to a set. You could save some more performance by using a Map<Integer, Set<String>> where you pre-hash your string set & can have an even quicker look up (mapped by hashes, use a collection to avoid losing values during hash collisions.)

    But yeah, that would be somewhat of a micro optimization. You sure you dont wanna save some metadata on the player?
     
  16. No, he is saying that when you actually need to perform a server-wide action such as a message broadcast, there is absolutely nothing you can do, you HAVE to iterate through all the players on the server.
     
  17. Sorry I wasn't clear I was pointing out that if something could be optimized it should be considered, in this case the OPs oringal approach had a look within a loop which as was pointed out:

    So in the case of 100 online players and a list of 2000 players you loop 200000 times doing string comparisions where as loading the players into a set and switching to using UUIDs you loop 100 times and do a check which is much cheaper (using a hashset will likely reduce the difference in cost between comparing UUIDs and strings due to the fact the hash will be the only thing to be checked most of the time anyway). Pulling a number out of thin air, if we say using a hashset makes the check 4 times quicker compared to a string comparision we can calculate our performance improvement:

    Cost of oringal approach 200000 * 4 = 800000
    Cost of improved approach 100 * 1 = 100

    800000 / 100 = 800% improvement

    Of course that's for specific values of online players and players in the "players" list/set but you get the idea.