From 587c1e02a52133715908c1a48a73ee50335c1f19 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 | 71 ++++++++++++++++--- 1 file changed, 62 insertions(+), 9 deletions(-) diff --git a/Spigot-Server-Patches/Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch b/Spigot-Server-Patches/Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch index 284d05804b..8efb6738bf 100644 --- a/Spigot-Server-Patches/Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch +++ b/Spigot-Server-Patches/Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch @@ -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 @@ -0,0 +0,0 @@ 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 @@ -0,0 +0,0 @@ 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; @@ -66,6 +66,23 @@ index e1e4ea793a..0aa6487d5b 100644 + // 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; @@ -114,7 +131,13 @@ index e1e4ea793a..0aa6487d5b 100644 if (ichunkaccess instanceof ProtoChunkExtension || ichunkaccess instanceof Chunk) { @@ -0,0 +0,0 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - if (!this.updatingChunksModified) { + }); + } + ++ 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(); @@ -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 +@@ -0,0 +0,0 @@ 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; @@ -0,0 +0,0 @@ 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; @@ -0,0 +0,0 @@ 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()) { @@ -0,0 +0,0 @@ public class CraftWorld implements World { + } + } + +- return ret; ++ return ret; }); + } + public int getPlayerCount() { + return world.players.size(); +@@ -0,0 +0,0 @@ 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); }