Merge pull request #131 from MineTheCube/fix/performance

Performance improvements & shop updater refactor

pom.xml

@@ -6,21 +6,30 @@
 
     <groupId>de.epiceric</groupId>
     <artifactId>ShopChest</artifactId>
-    <packaging>jar</packaging>
     <version>${version.final}</version>
-    <name>ShopChest</name>
+
+    <name>${project.artifactId}</name>
     <url>https://www.spigotmc.org/resources/shopchest.11431/</url>
     <description>Let your players create their own nice-looking shops to sell their stuff to other players!</description>
+    <packaging>jar</packaging>
 
     <properties>
-        <maven.compiler.source>1.7</maven.compiler.source>
+        <!-- Project Properties -->
-        <maven.compiler.target>1.7</maven.compiler.target>
+        <projectEncoding>UTF-8</projectEncoding>
+        <project.build.sourceEncoding>${projectEncoding}</project.build.sourceEncoding>
+        <project.build.outputEncoding>${projectEncoding}</project.build.outputEncoding>
 
+        <!-- JDK Version -->
+        <jdkVersion>1.7</jdkVersion>
+        <maven.compiler.source>${jdkVersion}</maven.compiler.source>
+        <maven.compiler.target>${jdkVersion}</maven.compiler.target>
+
+        <!-- Versioning -->
         <version.number>1.12.3</version.number>
         <version.final>${version.number}-${version.git}</version.final>
 
+        <!-- Others -->
         <github.global.server>github</github.global.server>
-
         <javadoc.opts><!-- Only jdk 1.8 and later --></javadoc.opts>
     </properties>
 
@@ -252,6 +261,7 @@
                             <shadedPattern>de.epiceric.shopchest.utils</shadedPattern>
                         </relocation>
                     </relocations>
+                    <createDependencyReducedPom>false</createDependencyReducedPom>
                 </configuration>
                 <executions>
                     <execution>

src/main/java/de/epiceric/shopchest/ShopChest.java

@@ -185,7 +185,7 @@ public class ShopChest extends JavaPlugin {
 
         if (updater != null) {
             debug("Stopping updater");
-            updater.cancel();
+            updater.stop();
         }
 
         if (database != null) {
@@ -433,13 +433,6 @@ public class ShopChest extends JavaPlugin {
         return updater;
     }
 
-    /**
-     * Set the {@link ShopUpdater} that schedules hologram and item updates
-     */
-    public void setUpdater(ShopUpdater updater) {
-        this.updater = updater;
-    }
-
     /**
      * @return Whether the plugin 'AreaShop' is enabled
      */

src/main/java/de/epiceric/shopchest/listeners/ShopItemListener.java

@@ -4,9 +4,7 @@ import de.epiceric.shopchest.ShopChest;
 import de.epiceric.shopchest.shop.Shop;
 import de.epiceric.shopchest.utils.ShopUtils;
 import org.bukkit.Bukkit;
-import org.bukkit.Location;
 import org.bukkit.Material;
-import org.bukkit.World;
 import org.bukkit.block.Block;
 import org.bukkit.block.BlockFace;
 import org.bukkit.block.BlockState;
@@ -15,7 +13,8 @@ import org.bukkit.event.EventHandler;
 import org.bukkit.event.EventPriority;
 import org.bukkit.event.Listener;
 import org.bukkit.event.block.*;
-import org.bukkit.event.player.*;
+import org.bukkit.event.player.PlayerBucketEmptyEvent;
+import org.bukkit.event.player.PlayerQuitEvent;
 import org.bukkit.event.world.StructureGrowEvent;
 
 public class ShopItemListener implements Listener {
@@ -34,6 +33,9 @@ public class ShopItemListener implements Listener {
             if (shop.getItem() != null) {
                 shop.getItem().setVisible(e.getPlayer(), false);
             }
+            if (shop.getHologram() != null) {
+                shop.getHologram().hidePlayer(e.getPlayer());
+            }
         }
     }
 

src/main/java/de/epiceric/shopchest/listeners/ShopUpdateListener.java

@@ -3,14 +3,11 @@ package de.epiceric.shopchest.listeners;
 import de.epiceric.shopchest.ShopChest;
 import de.epiceric.shopchest.shop.Shop;
 import de.epiceric.shopchest.utils.Callback;
-import de.epiceric.shopchest.utils.ShopUpdater;
+import org.bukkit.Location;
 import org.bukkit.entity.Player;
 import org.bukkit.event.EventHandler;
 import org.bukkit.event.EventPriority;
 import org.bukkit.event.Listener;
-import org.bukkit.event.player.PlayerChangedWorldEvent;
-import org.bukkit.event.player.PlayerJoinEvent;
-import org.bukkit.event.player.PlayerRespawnEvent;
 import org.bukkit.event.player.PlayerTeleportEvent;
 import org.bukkit.event.world.WorldLoadEvent;
 import org.bukkit.scheduler.BukkitRunnable;
@@ -24,45 +21,38 @@ public class ShopUpdateListener implements Listener {
     }
 
     @EventHandler(priority = EventPriority.MONITOR, ignoreCancelled = true)
-    public void onPlayerTeleport(final PlayerTeleportEvent e) {
+    public void onPlayerTeleport(PlayerTeleportEvent e) {
+        Location from = e.getFrom();
+        Location to = e.getTo();
+        final Player p = e.getPlayer();
+
         // Wait till the chunk should have loaded on the client
+        // Update IF worlds are different OR chunks are different (as many teleports are in same chunk)
+        if (!from.getWorld().equals(to.getWorld())
+                || from.getChunk().getX() != to.getChunk().getX()
+                || from.getChunk().getZ() != to.getChunk().getZ()) {
+            // Wait for 15 ticks before we actually put it in the queue
             new BukkitRunnable() {
                 @Override
                 public void run() {
-                hideShops(e.getPlayer(), true);
+                    plugin.getUpdater().beforeNext(new Runnable() {
-                plugin.getShopUtils().updateShops(e.getPlayer(), true);
+                        @Override
+                        public void run() {
+                            if (p.isOnline()) {
+                                for (Shop shop : plugin.getShopUtils().getShops()) {
+                                    if (shop.getItem() != null) {
+                                        shop.getItem().setVisible(p, false);
+                                    }
+                                    if (shop.getHologram() != null) {
+                                        shop.getHologram().hidePlayer(p);
+                                    }
+                                }
+                            }
+                        }
+                    });
                 }
             }.runTaskLater(plugin, 15L);
         }
-
-    @EventHandler(priority = EventPriority.MONITOR)
-    public void onPlayerJoin(PlayerJoinEvent e) {
-        restartShopUpdater(e.getPlayer());
-    }
-
-    // The Bukkit::getOnlinePlayers() list does not include players that
-    // are currently respawning or chaning worlds, so when only one player is
-    // online and is currently respawning, the updater will think that no player
-    // is online, so it will stop. To prevent that, a delay of 1 tick is needed.
-
-    @EventHandler
-    public void onPlayerChangedWorld(final PlayerChangedWorldEvent e) {
-        new BukkitRunnable() {
-            @Override
-            public void run() {
-                restartShopUpdater(e.getPlayer());
-            }
-        }.runTaskLater(plugin, 1L);
-    }
-
-    @EventHandler
-    public void onPlayerRespawn(final PlayerRespawnEvent e) {
-        new BukkitRunnable() {
-            @Override
-            public void run() {
-                restartShopUpdater(e.getPlayer());
-            }
-        }.runTaskLater(plugin, 1L);
     }
 
     @EventHandler
@@ -81,24 +71,4 @@ public class ShopUpdateListener implements Listener {
             }
         });
     }
-
-    private void restartShopUpdater(Player p) {
-        if (!plugin.getUpdater().isRunning()) {
-            plugin.setUpdater(new ShopUpdater(plugin));
-            plugin.getUpdater().start();
-        }
-
-        hideShops(p, false);
-    }
-
-    private void hideShops(Player p, boolean onlyItem) {
-        for (Shop shop : plugin.getShopUtils().getShops()) {
-            if (!onlyItem) {
-                if (shop.getHologram() != null) shop.getHologram().hidePlayer(p);
-            }
-
-            if (shop.getItem() != null) shop.getItem().setVisible(p, false);
-        }
-    }
-
 }

src/main/java/de/epiceric/shopchest/nms/Hologram.java

@@ -2,26 +2,28 @@ package de.epiceric.shopchest.nms;
 
 import de.epiceric.shopchest.ShopChest;
 import de.epiceric.shopchest.config.Config;
+import org.bukkit.Bukkit;
 import org.bukkit.ChatColor;
 import org.bukkit.Location;
 import org.bukkit.entity.ArmorStand;
 import org.bukkit.entity.Player;
 import org.bukkit.scheduler.BukkitRunnable;
 
-import java.util.ArrayList;
+import java.util.*;
-import java.util.List;
+import java.util.concurrent.ConcurrentHashMap;
 
 public class Hologram {
 
     private static List<Hologram> holograms = new ArrayList<>();
 
+    private final Set<UUID> visibility = Collections.newSetFromMap(new ConcurrentHashMap<UUID, Boolean>());
+    private final List<ArmorStandWrapper> wrappers = new ArrayList<>();
+    private final Location location;
+    private final ShopChest plugin;
+    private final Config config;
+
     private boolean exists = false;
-    private List<ArmorStandWrapper> wrappers = new ArrayList<>();
     private ArmorStandWrapper interactArmorStandWrapper;
-    private Location location;
-    private List<Player> visible = new ArrayList<>();
-    private ShopChest plugin;
-    private Config config;
 
     public Hologram(ShopChest plugin, String[] lines, Location location) {
         this.plugin = plugin;
@@ -66,11 +68,14 @@ public class Hologram {
         wrappers.add(line, wrapper);
 
         if (forceUpdateLine) {
-            for (Player player : visible) {
+            for (UUID uuid : visibility) {
+                Player player = Bukkit.getPlayer(uuid);
+                if (player != null) {
                     wrapper.setVisible(player, true);
                 }
             }
         }
+    }
 
     public void setLine(int line, String text) {
         if (text == null ||text.isEmpty()) {
@@ -148,6 +153,7 @@ public class Hologram {
      * @param p Player to which the hologram should be shown
      */
     public void showPlayer(final Player p) {
+        if (!isVisible(p)) {
             new BukkitRunnable() {
                 @Override
                 public void run() {
@@ -161,13 +167,15 @@ public class Hologram {
                 }
             }.runTaskAsynchronously(plugin);
 
-        visible.add(p);
+            visibility.add(p.getUniqueId());
+        }
     }
 
     /**
      * @param p Player from which the hologram should be hidden
      */
     public void hidePlayer(final Player p) {
+        if (isVisible(p)) {
             new BukkitRunnable() {
                 @Override
                 public void run() {
@@ -181,7 +189,8 @@ public class Hologram {
                 }
             }.runTaskAsynchronously(plugin);
 
-        visible.remove(p);
+            visibility.remove(p.getUniqueId());
+        }
     }
 
     /**
@@ -189,7 +198,7 @@ public class Hologram {
      * @return Whether the hologram is visible to the player
      */
     public boolean isVisible(Player p) {
-        return visible.contains(p);
+        return visibility.contains(p.getUniqueId());
     }
 
     /**
@@ -247,7 +256,9 @@ public class Hologram {
      */
     public static Hologram getHologram(ArmorStand armorStand) {
         for (Hologram hologram : holograms) {
-            if (hologram.contains(armorStand)) return hologram;
+            if (hologram.contains(armorStand)) {
+                return hologram;
+            }
         }
 
         return null;
@@ -258,12 +269,7 @@ public class Hologram {
      * @return Whether the armor stand is part of a hologram
      */
     public static boolean isPartOfHologram(ArmorStand armorStand) {
-        for (Hologram hologram : holograms) {
+        return getHologram(armorStand) != null;
-            if (hologram.contains(armorStand)) {
-                return true;
-            }
-        }
-        return false;
     }
 
 }

src/main/java/de/epiceric/shopchest/shop/ShopItem.java

@@ -2,6 +2,7 @@ package de.epiceric.shopchest.shop;
 
 import de.epiceric.shopchest.ShopChest;
 import de.epiceric.shopchest.utils.Utils;
+import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.entity.Player;
 import org.bukkit.inventory.ItemStack;
@@ -9,15 +10,17 @@ import org.bukkit.inventory.ItemStack;
 import java.lang.reflect.Constructor;
 import java.lang.reflect.Field;
 import java.lang.reflect.InvocationTargetException;
-import java.util.HashMap;
+import java.util.Collections;
-import java.util.Map;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.ConcurrentHashMap;
 
 public class ShopItem {
 
-    private ShopChest plugin;
+    private final ShopChest plugin;
-    private Map<Player, Boolean> visible = new HashMap<>();
+    private final Set<UUID> visibility = Collections.newSetFromMap(new ConcurrentHashMap<UUID, Boolean>());
-    private ItemStack itemStack;
+    private final ItemStack itemStack;
-    private Location location;
+    private final Location location;
 
     private Object entityItem;
     private int entityId;
@@ -88,8 +91,9 @@ public class ShopItem {
     }
 
     public void remove() {
-        for (Player p : visible.keySet()) {
+        for (UUID uuid : visibility) {
-            if (isVisible(p)) setVisible(p, false);
+            Player p = Bukkit.getPlayer(uuid);
+            if (p != null) setVisible(p, false);
         }
     }
 
@@ -103,7 +107,7 @@ public class ShopItem {
     }
 
     public boolean isVisible(Player p) {
-        return visible.get(p) == null ? false : visible.get(p);
+        return visibility.contains(p.getUniqueId());
     }
 
     public void setVisible(final Player p, boolean visible) {
@@ -114,6 +118,7 @@ public class ShopItem {
             for (Object packet : this.creationPackets) {
                 Utils.sendPacket(plugin, packet, p);
             }
+            visibility.add(p.getUniqueId());
         } else {
             try {
                 if (p.isOnline()) {
@@ -125,9 +130,8 @@ public class ShopItem {
                 plugin.debug("Failed to destroy shop item with reflection");
                 plugin.debug(e);
             }
+            visibility.remove(p.getUniqueId());
         }
-
-        this.visible.put(p, visible);
     }
 
 

src/main/java/de/epiceric/shopchest/utils/ShopUpdater.java

@@ -3,11 +3,12 @@ package de.epiceric.shopchest.utils;
 import de.epiceric.shopchest.ShopChest;
 import org.bukkit.Bukkit;
 import org.bukkit.entity.Player;
-import org.bukkit.scheduler.BukkitRunnable;
+import org.bukkit.scheduler.BukkitTask;
 
-import java.util.Collection;
+import java.util.Queue;
+import java.util.concurrent.ConcurrentLinkedQueue;
 
-public class ShopUpdater extends BukkitRunnable {
+public class ShopUpdater {
 
     public enum UpdateQuality {
         SLOWEST(31L),
@@ -18,7 +19,7 @@ public class ShopUpdater extends BukkitRunnable {
         FASTER(4L),
         FASTEST(1L);
 
-        private long interval;
+        private final long interval;
 
         UpdateQuality(long interval) {
             this.interval = interval;
@@ -29,47 +30,73 @@ public class ShopUpdater extends BukkitRunnable {
         }
     }
 
-    private ShopChest plugin;
+    private final ShopChest plugin;
+    private final Queue<Runnable> beforeNext = new ConcurrentLinkedQueue<>();
 
-    private boolean running;
+    private volatile BukkitTask running;
-    private long interval;
 
     public ShopUpdater(ShopChest plugin) {
         this.plugin = plugin;
-        setInterval(plugin.getShopChestConfig().update_quality.getInterval());
     }
 
-    public synchronized void setInterval(long interval) {
+    /**
-        this.interval = interval;
+     * Start task, except if it is already
-    }
+     */
-
+    public void start() {
-    public synchronized void start() {
+        if (!isRunning()) {
-        super.runTaskTimerAsynchronously(plugin, interval, interval);
+            long interval = plugin.getShopChestConfig().update_quality.getInterval();
-        running = true;
+            running = Bukkit.getScheduler().runTaskTimerAsynchronously(plugin, new ShopUpdaterTask(), interval, interval);
-    }
-
-    @Override
-    public synchronized void cancel() {
-        if (running) {
-            running = false;
-            super.cancel();
         }
     }
 
+    /**
+     * Stop any running task then start it again
+     */
+    public void restart() {
+        stop();
+        start();
+    }
+
+    /**
+     * Stop task properly
+     */
+    public void stop() {
+        if (running != null) {
+            running.cancel();
+            running = null;
+        }
+    }
+
+    /**
+     * @return whether task is running or not
+     */
     public boolean isRunning() {
-        return running;
+        return running != null;
     }
 
+    /**
+     * Register a task to run before next loop
+     *
+     * @param runnable task to run
+     */
+    public void beforeNext(Runnable runnable) {
+        beforeNext.add(runnable);
+    }
+
+    private class ShopUpdaterTask implements Runnable {
+
         @Override
         public void run() {
-        Collection<? extends Player> players = Bukkit.getOnlinePlayers();
+            if (!beforeNext.isEmpty()) {
-
+                for (Runnable runnable : beforeNext) {
-        if (players.isEmpty()) {
+                    runnable.run();
-            cancel();
+                }
+                beforeNext.clear();
             }
 
-        for (Player p : players) {
+            for (Player p : Bukkit.getOnlinePlayers()) {
                 plugin.getShopUtils().updateShops(p);
             }
         }
     }
+}

src/main/java/de/epiceric/shopchest/utils/ShopUtils.java

@@ -13,16 +13,14 @@ import org.bukkit.inventory.InventoryHolder;
 import org.bukkit.permissions.PermissionAttachmentInfo;
 import org.bukkit.util.Vector;
 
-import java.util.Collection;
+import java.util.*;
-import java.util.HashMap;
-import java.util.HashSet;
-import java.util.Set;
 
 public class ShopUtils {
 
-    private HashMap<Location, Shop> shopLocation = new HashMap<>();
+    private final HashMap<Location, Shop> shopLocation = new HashMap<>();
-    private HashMap<Player, Location> playerLocation = new HashMap<>();
+    private final Collection<Shop> shopLocationValues = Collections.unmodifiableCollection(shopLocation.values());
-    private ShopChest plugin;
+    private final HashMap<UUID, Location> playerLocation = new HashMap<>();
+    private final ShopChest plugin;
 
     public ShopUtils(ShopChest plugin) {
         this.plugin = plugin;
@@ -51,12 +49,11 @@ public class ShopUtils {
     }
 
     /**
-     * Get all Shops
+     * Get all shops
-     * @return Array of all Shops
+     * @return Read-only collection of all shops, may contain duplicates
      */
-    public Shop[] getShops() {
+    public Collection<Shop> getShops() {
-        Collection<Shop> shops = shopLocation.values();
+        return shopLocationValues;
-        return shops.toArray(new Shop[shops.size()]);
     }
 
     /**
@@ -218,10 +215,7 @@ public class ShopUtils {
         if (reloadConfig) {
             plugin.getShopChestConfig().reload(false, true, showConsoleMessages);
             plugin.getHologramFormat().reload();
-            plugin.getUpdater().cancel();
+            plugin.getUpdater().restart();
-
-            plugin.setUpdater(new ShopUpdater(plugin));
-            plugin.getUpdater().start();
         }
 
         plugin.getShopDatabase().connect(new Callback(plugin) {
@@ -277,7 +271,7 @@ public class ShopUtils {
      * @param force Whether update should be forced even if player has not moved
      */
     public void updateShops(Player player, boolean force) {
-        if (!force && player.getLocation().equals(playerLocation.get(player))) {
+        if (!force && player.getLocation().equals(playerLocation.get(player.getUniqueId()))) {
             // Player has not moved, so don't calculate shops again.
             return;
         }
@@ -314,12 +308,10 @@ public class ShopUtils {
                 }
             }
         } else {
-            for (Shop shop : getShops()) {
+            updateNearestShops(player);
-                updateShop(shop, player);
-            }
         }
 
-        playerLocation.put(player, player.getLocation());
+        playerLocation.put(player.getUniqueId(), player.getLocation());
     }
 
     private Set<Shop> getShopsInSight(Player player) {
@@ -370,27 +362,36 @@ public class ShopUtils {
         double holoDistSqr = Math.pow(plugin.getShopChestConfig().maximal_distance, 2);
         double itemDistSqr = Math.pow(plugin.getShopChestConfig().maximal_item_distance, 2);
 
+        updateShop(shop, player, holoDistSqr, itemDistSqr);
+    }
+
+    private void updateNearestShops(Player p) {
+        double holoDistSqr = Math.pow(plugin.getShopChestConfig().maximal_distance, 2);
+        double itemDistSqr = Math.pow(plugin.getShopChestConfig().maximal_item_distance, 2);
+
+        for (Shop shop : getShops()) {
+            updateShop(shop, p, holoDistSqr, itemDistSqr);
+        }
+    }
+
+    private void updateShop(Shop shop, Player player, double holoDistSqr, double itemDistSqr) {
         if (player.getLocation().getWorld().getName().equals(shop.getLocation().getWorld().getName())) {
             double distSqr = shop.getLocation().distanceSquared(player.getLocation());
 
+            if (shop.getHologram() != null) {
                 if (distSqr <= holoDistSqr) {
-                if (shop.getHologram() != null) {
-                    if (!shop.getHologram().isVisible(player)) {
                     shop.getHologram().showPlayer(player);
-                    }
-                }
                 } else {
-                if (shop.getHologram() != null) {
                     shop.getHologram().hidePlayer(player);
                 }
             }
 
-            if (distSqr <= itemDistSqr) {
             if (shop.getItem() != null) {
+                if (distSqr <= itemDistSqr) {
                     shop.getItem().setVisible(player, true);
-                }
                 } else {
-                if (shop.getItem() != null) shop.getItem().setVisible(player, false);
+                    shop.getItem().setVisible(player, false);
+                }
             }
         }
     }