From 38c626229deb9f0692de0d5748fa6621ccd280eb Mon Sep 17 00:00:00 2001 From: Aikar Date: Wed, 8 Apr 2020 21:07:59 -0400 Subject: [PATCH] Improve Optimize Memory use logic to make iterator safer and fix bad plugins like P2 Accessing world state async is bad mkay. But we know you didn't do it city <3 --- ...hunkMap-memory-use-for-visibleChunks.patch | 89 +++++++++++++++---- 1 file changed, 71 insertions(+), 18 deletions(-) diff --git a/Spigot-Server-Patches/0459-Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch b/Spigot-Server-Patches/0459-Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch index b5c1627e4..e5a00236f 100644 --- a/Spigot-Server-Patches/0459-Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch +++ b/Spigot-Server-Patches/0459-Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch @@ -1,4 +1,4 @@ -From d4a80e85b8785b079769629ecf004f73fde66efd Mon Sep 17 00:00:00 2001 +From cee599dd7b5c962429d797a091ab740da42179a3 Mon Sep 17 00:00:00 2001 From: Aikar Date: Wed, 8 Apr 2020 03:06:30 -0400 Subject: [PATCH] Optimize PlayerChunkMap memory use for visibleChunks @@ -12,7 +12,7 @@ as before with only 2 small objects created (FastIterator/MapEntry) This should result in siginificant memory use reduction and improved GC behavior. diff --git a/src/main/java/net/minecraft/server/ChunkProviderServer.java b/src/main/java/net/minecraft/server/ChunkProviderServer.java -index 1dcd0980ec..59055cccc5 100644 +index 1dcd0980ec..e627440c41 100644 --- a/src/main/java/net/minecraft/server/ChunkProviderServer.java +++ b/src/main/java/net/minecraft/server/ChunkProviderServer.java @@ -669,7 +669,7 @@ public class ChunkProviderServer extends IChunkProvider { @@ -20,7 +20,7 @@ index 1dcd0980ec..59055cccc5 100644 }; // Paper end - this.playerChunkMap.f().forEach((playerchunk) -> { -+ this.playerChunkMap.visibleChunks.values().forEach((playerchunk) -> { // Paper - no need to wrap iterator ++ this.playerChunkMap.forEachVisibleChunk((playerchunk) -> { // Paper - safe iterator incase chunk loads, also no wrapping Optional optional = ((Either) playerchunk.b().getNow(PlayerChunk.UNLOADED_CHUNK)).left(); if (optional.isPresent()) { @@ -38,7 +38,7 @@ index b9d5844520..9980e4c277 100644 List allChunks = new ArrayList<>(visibleChunks.values()); List players = world.players; diff --git a/src/main/java/net/minecraft/server/PlayerChunkMap.java b/src/main/java/net/minecraft/server/PlayerChunkMap.java -index e1e4ea793a..0aa6487d5b 100644 +index e1e4ea793a..ce645a69bd 100644 --- a/src/main/java/net/minecraft/server/PlayerChunkMap.java +++ b/src/main/java/net/minecraft/server/PlayerChunkMap.java @@ -56,7 +56,8 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { @@ -47,7 +47,7 @@ index e1e4ea793a..0aa6487d5b 100644 public final Long2ObjectLinkedOpenHashMap updatingChunks = new Long2ObjectLinkedOpenHashMap(); - public volatile Long2ObjectLinkedOpenHashMap visibleChunks; + public final Long2ObjectLinkedOpenHashMap visibleChunks = new Long2ObjectLinkedOpenHashMap(); // Paper - remove copying, make mt safe -+ public volatile Long2ObjectLinkedOpenHashMap visibleChunksClone; // Paper - remove copying, make mt safe ++ public transient Long2ObjectLinkedOpenHashMap visibleChunksClone; // Paper - remove copying, make mt safe private final Long2ObjectLinkedOpenHashMap pendingUnload; final LongSet loadedChunks; // Paper - private -> package public final WorldServer world; @@ -60,12 +60,29 @@ index e1e4ea793a..0aa6487d5b 100644 this.pendingUnload = new Long2ObjectLinkedOpenHashMap(); this.loadedChunks = new LongOpenHashSet(); this.unloadQueue = new LongOpenHashSet(); -@@ -262,8 +263,32 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -262,8 +263,49 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { return (PlayerChunk) this.updatingChunks.get(i); } + // Paper start - remove cloning of visible chunks unless accessed as a collection async + private static final boolean DEBUG_ASYNC_VISIBLE_CHUNKS = Boolean.getBoolean("paper.debug-async-visible-chunks"); ++ private boolean isIterating = false; ++ public void forEachVisibleChunk(java.util.function.Consumer consumer) { ++ org.spigotmc.AsyncCatcher.catchOp("forEachVisibleChunk"); ++ boolean prev = isIterating; ++ boolean wasUpdating = updatingChunksModified; ++ isIterating = true; ++ try { ++ for (PlayerChunk value : this.visibleChunks.values()) { ++ consumer.accept(value); ++ } ++ } finally { ++ this.isIterating = prev; ++ if (!this.isIterating && updatingChunksModified && !wasUpdating) { ++ this.updateVisibleChunks(); ++ } ++ } ++ } + public Long2ObjectLinkedOpenHashMap getVisibleChunks() { + if (Thread.currentThread() == this.world.serverThread) { + return this.visibleChunks; @@ -93,7 +110,7 @@ index e1e4ea793a..0aa6487d5b 100644 return (PlayerChunk) this.visibleChunks.get(i); } -@@ -444,8 +469,9 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -444,8 +486,9 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { // Paper end protected void save(boolean flag) { @@ -104,7 +121,7 @@ index e1e4ea793a..0aa6487d5b 100644 MutableBoolean mutableboolean = new MutableBoolean(); do { -@@ -473,7 +499,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -473,7 +516,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { // this.i(); // Paper - nuke IOWorker PlayerChunkMap.LOGGER.info("ThreadedAnvilChunkStorage ({}): All chunks are saved", this.w.getName()); } else { @@ -113,8 +130,14 @@ index e1e4ea793a..0aa6487d5b 100644 IChunkAccess ichunkaccess = (IChunkAccess) playerchunk.getChunkSave().getNow(null); // CraftBukkit - decompile error if (ichunkaccess instanceof ProtoChunkExtension || ichunkaccess instanceof Chunk) { -@@ -643,7 +669,14 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - if (!this.updatingChunksModified) { +@@ -639,11 +682,19 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { + }); + } + ++ protected boolean updateVisibleChunks() { return b(); } // Paper - OBFHELPER + protected boolean b() { +- if (!this.updatingChunksModified) { ++ if (!this.updatingChunksModified || this.isIterating) { // Paper return false; } else { - this.visibleChunks = this.updatingChunks.clone(); @@ -129,7 +152,7 @@ index e1e4ea793a..0aa6487d5b 100644 this.updatingChunksModified = false; return true; } -@@ -1104,12 +1137,12 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -1104,12 +1155,12 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { } protected Iterable f() { @@ -145,30 +168,60 @@ index e1e4ea793a..0aa6487d5b 100644 while (objectbidirectionaliterator.hasNext()) { Entry entry = (Entry) objectbidirectionaliterator.next(); diff --git a/src/main/java/org/bukkit/craftbukkit/CraftWorld.java b/src/main/java/org/bukkit/craftbukkit/CraftWorld.java -index 051506fce8..c0e8eb85d7 100644 +index 051506fce8..630d6470a4 100644 --- a/src/main/java/org/bukkit/craftbukkit/CraftWorld.java +++ b/src/main/java/org/bukkit/craftbukkit/CraftWorld.java -@@ -290,6 +290,7 @@ public class CraftWorld implements World { +@@ -74,6 +74,7 @@ import net.minecraft.server.GameRules; + import net.minecraft.server.GroupDataEntity; + import net.minecraft.server.IBlockData; + import net.minecraft.server.IChunkAccess; ++import net.minecraft.server.MCUtil; + import net.minecraft.server.MinecraftKey; + import net.minecraft.server.MovingObjectPosition; + import net.minecraft.server.PacketPlayOutCustomSoundEffect; +@@ -290,6 +291,7 @@ public class CraftWorld implements World { return ret; } public int getTileEntityCount() { -+ org.spigotmc.AsyncCatcher.catchOp("getTileEntityCount"); ++ return MCUtil.ensureMain(() -> { // We don't use the full world tile entity list, so we must iterate chunks Long2ObjectLinkedOpenHashMap chunks = world.getChunkProvider().playerChunkMap.visibleChunks; int size = 0; -@@ -306,6 +307,7 @@ public class CraftWorld implements World { +@@ -301,11 +303,13 @@ public class CraftWorld implements World { + size += chunk.tileEntities.size(); + } + return size; ++ }); + } + public int getTickableTileEntityCount() { return world.tileEntityListTick.size(); } public int getChunkCount() { -+ org.spigotmc.AsyncCatcher.catchOp("getChunkCount"); ++ return MCUtil.ensureMain(() -> { int ret = 0; for (PlayerChunk chunkHolder : world.getChunkProvider().playerChunkMap.visibleChunks.values()) { -@@ -432,6 +434,7 @@ public class CraftWorld implements World { +@@ -314,7 +318,7 @@ public class CraftWorld implements World { + } + } + +- return ret; ++ return ret; }); + } + public int getPlayerCount() { + return world.players.size(); +@@ -432,6 +436,14 @@ public class CraftWorld implements World { @Override public Chunk[] getLoadedChunks() { -+ org.spigotmc.AsyncCatcher.catchOp("getLoadedChunks"); // Paper ++ // Paper start ++ if (Thread.currentThread() != world.getMinecraftWorld().serverThread) { ++ synchronized (world.getChunkProvider().playerChunkMap.visibleChunks) { ++ Long2ObjectLinkedOpenHashMap chunks = world.getChunkProvider().playerChunkMap.visibleChunks; ++ return chunks.values().stream().map(PlayerChunk::getFullChunk).filter(Objects::nonNull).map(net.minecraft.server.Chunk::getBukkitChunk).toArray(Chunk[]::new); ++ } ++ } ++ // Paper end Long2ObjectLinkedOpenHashMap chunks = world.getChunkProvider().playerChunkMap.visibleChunks; return chunks.values().stream().map(PlayerChunk::getFullChunk).filter(Objects::nonNull).map(net.minecraft.server.Chunk::getBukkitChunk).toArray(Chunk[]::new); }