Thread Safety

Discussion in 'Plugin Development' started by RealDope, Feb 25, 2013.

Thread Status:
Not open for further replies.
  1. Offline

    RealDope

    I want to run this code, but it seems like it will be quite resource intensive, running every half second? (If this isn't true, tell me and this can be solved right here lol).

    Anyways, I wanted to run this Async, but I have no clue about what is safe to run Async and what isn't. Is it already safe? Can I tweak it to be safe without changing what the code does?

    Code:
    Code:JAVA
    1.  
    2. public void startRegen() {
    3. new BukkitRunnable() {
    4. public void run() {
    5. for(Player p : plugin.getServer().getOnlinePlayers()) {
    6. String name = p.getName();
    7. long time = System.currentTimeMillis();
    8. if(lastCombatTimes.containsKey(name)) {
    9. long lastTime = lastCombatTimes.get(name);
    10. int reset = plugin.getConfig().getInt("Regen_Reset");
    11. if(time - lastTime >= reset) {
    12. Champion c = plugin.getChampion(name);
    13. c.setHp(c.getHp() + c.getRegen()); // Add amount of regen to hp
    14. }
    15. }
    16. }
    17. }
    18. }.runTaskTimer(plugin, 10, 10); // Run every half second
    19. }
    20.  


    EDIT:

    Additionally, as you can see I'm having players regen their hp. (Not actually hp, a different setup I have for it). Does anybody know a better way to have hp regen occur at different time for each player without running a ton of schedulers? This method will be off up to a half a second as to when the player gets their regen after combat, which isn't really a huge deal, but would be nice to fix if anyone has an easy way.

    TL;DR Read the whole post
     
  2. Offline

    H2NCH2COOH

    Code:
    Map<String,BukkitRunnable> runningCountdown=Collections.synchronizedMap(new HashMap<String,BukkitRunnable());
    Set<String> regenSet=Collections.synchronizedSet(new HashSet<String>());
    public void enterCombat(Player player)
    {
        if(runningCountdown.containsKey(player.getName())
        {
            runningCountdown.get(player.getName()).cancel();
        }
        regenSet.remove(player.getName());
    }
     
    public void leaveCombat(final Player player)
    {
        task=new BukkitRunnable()
        {
            @Override
            public void run()
            {
                regenSet.add(player.getName());
            }
        }.runTaskLaterAsynchronously(plugin,plugin.getConfig().getInt("Regen_Reset")/*in ticks*/);
        runningCountdown.put(p.getName(),task);
    }
     
    public void startRegen()
    {
        new BukkitRunnable() {
            public void run() {
                for(Player p : plugin.getServer().getOnlinePlayers()) {
                    String name = p.getName();
                    if(regenSet.contains(name)) {
                        Champion c = plugin.getChampion(name);
                        c.setHp(c.getHp() + c.getRegen()); // Add amount of regen to hp
                    }
                }
            }
        }.runTaskTimerAsynchronously(plugin, 10, 10); // Run every half second
    }
    This still uses a lot of scheduler, but Async. And did some change on structure to make it cleaner.
    Make sure your c.setHp(c.getHp() + c.getRegen()) doesn't call Bukkit API.

    A second one, with every player a task.....
    Code:
    Map<String,BukkitRunnable> runningRegen=Collections.synchronizedMap(new HashMap<String,BukkitRunnable());
    Map<String,BukkitRunnable> runningCountdown=Collections.synchronizedMap(new HashMap<String,BukkitRunnable());
     
    public void enterCombat(Player player)
    {
        if(runningCountdown.contains(player,getName())
        {
            runningCountdown.remove(player.getName()).cancel();
        }
       
        if(runningRegen.containsKey(player.getName())
        {
            runningRegen.remove(player.getName()).cancel();
        }
    }
     
    public void leaveCombat(final Player player)
    {
        BukkitRunnable task=new BukkitRunnable()
        {
            @Override
            public void run()
            {
                BukkitRunnable regen=new BukkitRunnable()
                {
                    Player p=player;
                   
                    @Override
                    public void run()
                    {
                        Champion c = plugin.getChampion(name);
                        c.setHp(c.getHp() + c.getRegen()); // Add amount of regen to hp
                    }
                }.runTaskTimerAsynchronously(plugin,0,10)
               
                runningRegen.put(player.getName(),regen);
            }
        }.runTaskLaterAsynchronously(plugin,plugin.getConfig().getInt("Regen_Reset")/*in ticks*/);
    }
    I strongly recommend remove all the "Asynchroously", there's no use creating that much threads.

    EDIT by Moderator: merged posts, please use the edit button instead of double posting.
     
    Last edited by a moderator: May 31, 2016
  3. Offline

    RealDope

    H2NCH2COOH
    Do you think it will be resource-intensive to run this on the the main thread?
     
  4. Offline

    Milkywayz

    Iterating the online player list asynchronously is not safe, you should make a local copy of it.
    Setting the 'regen' and 'xp' are likely bukkit methods, which are too not thread safe.

    You can make some of your code multithreaded, but the real issue is what you are doing that requires it. Databases, URL connections, mass file handling should require multithreading. I'd recommend improving the efficiency of your program before trying to complicate it with threads which will fail if you use the bukkit api asynchronously you will likely run into problems, unless it's one of the like 8 methods that are thread safe.

    Also beware working with multithreading behaves differently when testing on a 1 player dev server and on a live server with many players online.
     
  5. Offline

    RealDope

    Milkywayz
    Actually setting the regen and hp are not bukkit methods.
    Okay, I'll copy the player list.

    Can you recommend anything to improve efficiency while still accomplishing the same thing?

    Also I imagine using the FileConfiguration API isn't safe since that's part of Bukkit? Is there any way I can run this Async? It will be saving many files frequently, and I'd rather not lag the server every 60 seconds while it does this:

    Code:JAVA
    1.  
    2. private HashMap<String, Champion> champions = new HashMap<String, Champion>();
    3. public void onEnable() {
    4. new BukkitRunnable() {
    5. public void run() {
    6. for (Champion c : champions.values()) {
    7. c.saveStatsToFile();
    8. }
    9. }
    10. }.runTaskTimer(this, 20*60, 20*60);
    11. }
    12.  


    Code:JAVA
    1.  
    2. public void saveStatsToFile() {
    3. if (!hasFile()) createFile();
    4. File file = new File("plugins/LegionChamps/Champions/" + name + ".yml");
    5. FileConfiguration yml = YamlConfiguration.loadConfiguration(file);
    6. yml.set("Strength", strength);
    7. yml.set("Agility", agility);
    8. yml.set("Endurance", endurance);
    9. yml.set("Precision", precision);
    10. yml.set("Health", hp);
    11. yml.set("MaxHealth", maxHp);
    12. yml.set("Regen", regen);
    13. yml.set("Level", level);
    14. yml.set("Exp", exp);
    15. yml.set("CritChance", crit);
    16. yml.set("DodgeChance", dodge);
    17. yml.set("ItemFind", itemFind);
    18. yml.set("GoldFind", goldFind);
    19.  
    20. yml.set("DamageDone", damageDone);
    21. yml.set("DamageTaken", damageTaken);
    22. yml.set("ExpGained", expGained);
    23. yml.set("PlayersKilled", playersKilled);
    24. yml.set("KillStreaks", killStreaks);
    25. yml.set("HighestKillStreak", highestStreak);
    26. yml.set("TotalGold", totalGold);
    27. yml.set("GoldFromMonsters", goldFromMonsters);
    28. yml.set("GoldFromShops", goldFromShops);
    29. yml.set("GoldFromTrade", goldFromTrade);
    30. yml.set("GoldSpent", goldSpent);
    31. yml.set("TotalDeaths", totalDeaths);
    32. yml.set("DeathsToPlayers", deathsToPlayers);
    33. yml.set("DeathsToMonsters", deathsToMonsters);
    34. yml.set("DeathsToEnvironment", deathsToEnvironment);
    35. yml.set("TotalMobsKilled", mobsKilled);
    36. yml.set("BossesKilled", bossesKilled);
    37. try {
    38. yml.save(file);
    39. } catch (IOException e) {
    40. plugin.getLogger().severe("Error saving Champion file for: " + name);
    41. e.printStackTrace();
    42. }
    43. }
    44.  
     
  6. Offline

    Milkywayz

    Code:
    int reset = plugin.getConfig().getInt("Regen_Reset");
    Caching that line, could help. (I know you initially cache it here as a var, but do it before this as this method runs every half second, and the var is essentially global)

    Improve the iteration speed of:
    Code:
    plugin.getChampion(name)
    Checks like comparing strings, if you know the strings are equal case then use .equals instead of() .equalsIgnoreCase().

    Micro optimizing sound silly, but can help and they give you higher quality code in the long run. It's recommended you don't optimize too early, because it is detrimental to the project.
     
  7. RealDope
    Setting hp is actually a Bukkit method because you're using the Player interface which is from Bukkit.
    Unless you're setting an imaginary HP that does not affect the actual player...

    Non-Bukkit API refers to stuff like reading files, URLs, doing complex math, etc.

    Doing this as a separate thread will do more harm than good, the code is pretty fast and trying to go backwards-forwards between threads to get players and give them health is just not worth it.

    Do it in the main thread, loop through your stored players and set their HP.
     
    Milkywayz likes this.
  8. Offline

    RealDope

    Yeah, actually it is an imaginary HP.
    Code:JAVA
    1.  
    2. public void setHp(int value) {
    3. if(value > getMaxHp()) value = getMaxHp(); // Don't let hp go above max
    4. hp = value;
    5. }
    6.  

    Champion is a class that I use to store stats for every player. Although now that I think about it, I will be using the default health bar to display percentage of health, so I guess I'll still have to set hp, which won't be thread safe.

    Okay, if this code won't be a problem running every half a second, then that's fine for me.

    Thanks for all the help and information.
     
Thread Status:
Not open for further replies.

Share This Page