From b1d551395f3c6917a4d0c28f339894802f6a8933 Mon Sep 17 00:00:00 2001 From: MrPowerGamerBR Date: Thu, 23 Nov 2023 14:43:30 -0300 Subject: [PATCH] Optimize "canSee" checks The "canSee" checks is in a hot path, invoked by each entity for each player on the server if they are in tracking range, so optimizing it is pretty nice First, we change the original "HashMap" to fastutil's "Object2ObjectOpenHashMap", because the containsKey throughput is better Then, we add a "isEmpty()" check before attempting to check if the map contains something This seems stupid, but it does seem that it improves the performance a bit, and it makes sense, "containsKey(...)" does not attempt to check the map size before attempting to check if the map contains the key We also create a "canSee" method tailored for "ChunkMap#updatePlayer()", a method without the equals check (the "updatePlayer()" already checks if the entity is the same entity) and we cache the "isVisibleByDefault()" result between runs (this also seems a bit overkill because "isVisibleByDefault()" is just a method that returns a boolean, but because this is a hotpath, we need all optimizations that we can get --- README.md | 5 ++ .../server/0016-Optimize-canSee-checks.patch | 75 +++++++++++++++++++ ...Track-how-much-MSPT-each-world-used.patch} | 0 ...atch => 0018-Parallel-world-ticking.patch} | 0 4 files changed, 80 insertions(+) create mode 100644 patches/server/0016-Optimize-canSee-checks.patch rename patches/server/{0016-Track-how-much-MSPT-each-world-used.patch => 0017-Track-how-much-MSPT-each-world-used.patch} (100%) rename patches/server/{0017-Parallel-world-ticking.patch => 0018-Parallel-world-ticking.patch} (100%) diff --git a/README.md b/README.md index b88f531..0c45d41 100644 --- a/README.md +++ b/README.md @@ -60,6 +60,11 @@ SparklyPaper's config file is `sparklypaper.yml`, the file is, by default, place * The `getChunkKey(...)` call is a bit expensive, using 0.24% of CPU time with 19k chunks loaded. * So instead of paying the price on each tick, we pay the price when the chunk is loaded. * Which, if you think about it, is actually better, since we tick chunks more than we load chunks. +* Optimize `canSee(...)` checks + * The `canSee(...)` checks is in a hot path (`ChunkMap#updatePlayers()`), invoked by each entity for each player on the server if they are in tracking range, so optimizing it is pretty nice. + * First, we change the original `HashMap` to fastutil's `Object2ObjectOpenHashMap`, because fastutil's `containsKey` throughput is better. + * Then, we add a `isEmpty()` check before attempting to check if the map contains something. This seems stupid, but it does seem that it improves the performance a bit, and it makes sense, `containsKey(...)` does not attempt to check the map size before attempting to check if the map contains the key. + * We also create a `canSee` method tailored for `ChunkMap#updatePlayer()`, a method without the equals check (the `updatePlayer()` already checks if the entity is the same entity) and we cache the `isVisibleByDefault()` result between runs (this also seems a bit overkill because `isVisibleByDefault()` is just a method that returns a boolean, but because this is a hot path, we need all optimizations that we can get!Draem). * Check how much MSPT (milliseconds per tick) each world is using in `/mspt` * Useful to figure out which worlds are lagging your server. ![Per World MSPT](docs/per-world-mspt.png) diff --git a/patches/server/0016-Optimize-canSee-checks.patch b/patches/server/0016-Optimize-canSee-checks.patch new file mode 100644 index 0000000..c8f0c9a --- /dev/null +++ b/patches/server/0016-Optimize-canSee-checks.patch @@ -0,0 +1,75 @@ +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 +From: MrPowerGamerBR +Date: Thu, 23 Nov 2023 14:36:47 -0300 +Subject: [PATCH] Optimize "canSee" checks + +The "canSee" checks is in a hot path, invoked by each entity for each player on the server if they are in tracking range, so optimizing it is pretty nice + +First, we change the original "HashMap" to fastutil's "Object2ObjectOpenHashMap", because the containsKey throughput is better + +Then, we add a "isEmpty()" check before attempting to check if the map contains something + +This seems stupid, but it does seem that it improves the performance a bit, and it makes sense, "containsKey(...)" does not attempt to check the map size before attempting to check if the map contains the key + +We also create a "canSee" method tailored for "ChunkMap#updatePlayer()", a method without the equals check (the "updatePlayer()" already checks if the entity is the same entity) and we cache the "isVisibleByDefault()" result between runs (this also seems a bit overkill because "isVisibleByDefault()" is just a method that returns a boolean, but because this is a hotpath, we need all optimizations that we can get + +diff --git a/src/main/java/net/minecraft/server/level/ChunkMap.java b/src/main/java/net/minecraft/server/level/ChunkMap.java +index caa73632aee15583c6b6ed12a668c8f49b794708..6fff96c1c0fc1442803718fa36ac30a35b6ae2d7 100644 +--- a/src/main/java/net/minecraft/server/level/ChunkMap.java ++++ b/src/main/java/net/minecraft/server/level/ChunkMap.java +@@ -1326,6 +1326,7 @@ public class ChunkMap extends ChunkStorage implements ChunkHolder.PlayerProvider + private final int range; + SectionPos lastSectionPos; + public final Set seenBy = new it.unimi.dsi.fastutil.objects.ReferenceOpenHashSet<>(); // Paper - optimise map impl ++ boolean visibleByDefault; // SparklyPaper - optimize canSee checks + + public TrackedEntity(Entity entity, int i, int j, boolean flag) { + this.serverEntity = new ServerEntity(ChunkMap.this.level, entity, j, flag, this::broadcast, this.seenBy); // CraftBukkit +@@ -1340,6 +1341,7 @@ public class ChunkMap extends ChunkStorage implements ChunkHolder.PlayerProvider + final void updatePlayers(com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet newTrackerCandidates) { + com.destroystokyo.paper.util.misc.PooledLinkedHashSets.PooledObjectLinkedOpenHashSet oldTrackerCandidates = this.lastTrackerCandidates; + this.lastTrackerCandidates = newTrackerCandidates; ++ this.visibleByDefault = entity.visibleByDefault; // SparklyPaper - optimize canSee checks + + if (newTrackerCandidates != null) { + Object[] rawData = newTrackerCandidates.getBackingSet(); +@@ -1436,7 +1438,7 @@ public class ChunkMap extends ChunkStorage implements ChunkHolder.PlayerProvider + // Paper end - check Y + + // CraftBukkit start - respect vanish API +- if (flag && !player.getBukkitEntity().canSee(this.entity.getBukkitEntity())) { // Paper - only consider hits ++ if (flag && !player.getBukkitEntity().canSeeChunkMapUpdatePlayer(this.entity.getBukkitEntity(), visibleByDefault)) { // Paper - only consider hits // SparklyPaper - optimize canSee checks + flag = false; + } + // CraftBukkit end +diff --git a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java +index 83aaf3e6e377d731ce02f779f80b7bf5db46f89f..3a02f92b57563b42680f4b867d681d2232c748cc 100644 +--- a/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java ++++ b/src/main/java/org/bukkit/craftbukkit/entity/CraftPlayer.java +@@ -183,7 +183,7 @@ public class CraftPlayer extends CraftHumanEntity implements Player { + private boolean hasPlayedBefore = false; + private final ConversationTracker conversationTracker = new ConversationTracker(); + private final Set channels = new HashSet(); +- private final Map>> invertedVisibilityEntities = new HashMap<>(); ++ private final Map>> invertedVisibilityEntities = new it.unimi.dsi.fastutil.objects.Object2ObjectOpenHashMap<>(); // SparklyPaper - optimize canSee checks + private final Set unlistedEntities = new HashSet<>(); // Paper + private static final WeakHashMap> pluginWeakReferences = new WeakHashMap<>(); + private int hash = 0; +@@ -2070,9 +2070,16 @@ public class CraftPlayer extends CraftHumanEntity implements Player { + + @Override + public boolean canSee(org.bukkit.entity.Entity entity) { +- return this.equals(entity) || entity.isVisibleByDefault() ^ this.invertedVisibilityEntities.containsKey(entity.getUniqueId()); // SPIGOT-7312: Can always see self ++ return this.equals(entity) || entity.isVisibleByDefault() ^ (!invertedVisibilityEntities.isEmpty() && this.invertedVisibilityEntities.containsKey(entity.getUniqueId())); // SPIGOT-7312: Can always see self // SparklyPaper - optimize canSee checks + } + ++ // SparklyPaper - optimize canSee checks ++ // The check in ChunkMap#updatePlayer already rejects if it is the same entity, so we don't need to check it twice, especially because CraftPlayer's equals check is a bit expensive ++ public boolean canSeeChunkMapUpdatePlayer(org.bukkit.entity.Entity entity, boolean visibleByDefault) { ++ return visibleByDefault ^ (!invertedVisibilityEntities.isEmpty() && this.invertedVisibilityEntities.containsKey(entity.getUniqueId())); // SPIGOT-7312: Can always see self // SparklyPaper - optimize canSee checks ++ } ++ // SparklyPaper end ++ + public boolean canSee(UUID uuid) { + org.bukkit.entity.Entity entity = this.getServer().getPlayer(uuid); + if (entity == null) { diff --git a/patches/server/0016-Track-how-much-MSPT-each-world-used.patch b/patches/server/0017-Track-how-much-MSPT-each-world-used.patch similarity index 100% rename from patches/server/0016-Track-how-much-MSPT-each-world-used.patch rename to patches/server/0017-Track-how-much-MSPT-each-world-used.patch diff --git a/patches/server/0017-Parallel-world-ticking.patch b/patches/server/0018-Parallel-world-ticking.patch similarity index 100% rename from patches/server/0017-Parallel-world-ticking.patch rename to patches/server/0018-Parallel-world-ticking.patch