From 0ed6da7ab2a309973e4b3bbe9e7abe56ea93fe2a Mon Sep 17 00:00:00 2001 From: Aikar Date: Fri, 10 Apr 2020 14:11:46 -0400 Subject: [PATCH] Fix issues with 167 causing crashes due to missing chunks - Fixes #3122 Forgot to flip the pending boolean back to false, causing it to copy empty data on the next tick if nothing else triggered a load. haven't managed to actually reproduce the crash others got, but did verify that the bad copy was occurring erasing the data. also fixed a bug with chunk load callback not executing before another one was scheduled. --- ...hunkMap-memory-use-for-visibleChunks.patch | 101 +++++++++---- ...essing-of-chunk-loads-and-generation.patch | 137 +----------------- 2 files changed, 79 insertions(+), 159 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 e5a00236f..8b14b9488 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 cee599dd7b5c962429d797a091ab740da42179a3 Mon Sep 17 00:00:00 2001 +From 4134905d5d67b984c29785aa279cd0fb91c73140 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 @@ -11,6 +11,44 @@ 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/com/destroystokyo/paper/util/map/Long2ObjectLinkedOpenHashMapFastCopy.java b/src/main/java/com/destroystokyo/paper/util/map/Long2ObjectLinkedOpenHashMapFastCopy.java +new file mode 100644 +index 0000000000..e0ad725b2e +--- /dev/null ++++ b/src/main/java/com/destroystokyo/paper/util/map/Long2ObjectLinkedOpenHashMapFastCopy.java +@@ -0,0 +1,32 @@ ++package com.destroystokyo.paper.util.map; ++ ++import it.unimi.dsi.fastutil.longs.Long2ObjectLinkedOpenHashMap; ++ ++public class Long2ObjectLinkedOpenHashMapFastCopy extends Long2ObjectLinkedOpenHashMap { ++ ++ public void copyFrom(Long2ObjectLinkedOpenHashMapFastCopy map) { ++ if (key.length < map.key.length) { ++ key = null; ++ key = new long[map.key.length]; ++ } ++ if (value.length < map.value.length) { ++ value = null; ++ //noinspection unchecked ++ value = (V[]) new Object[map.value.length]; ++ } ++ if (link.length < map.link.length) { ++ link = null; ++ link = new long[map.link.length]; ++ } ++ System.arraycopy(map.key, 0, this.key, 0, map.key.length); ++ System.arraycopy(map.value, 0, this.value, 0, map.value.length); ++ System.arraycopy(map.link, 0, this.link, 0, map.link.length); ++ this.size = map.size; ++ this.mask = map.mask; ++ this.first = map.first; ++ this.last = map.last; ++ this.n = map.n; ++ this.maxFill = map.maxFill; ++ this.containsNullKey = map.containsNullKey; ++ } ++} diff --git a/src/main/java/net/minecraft/server/ChunkProviderServer.java b/src/main/java/net/minecraft/server/ChunkProviderServer.java index 1dcd0980ec..e627440c41 100644 --- a/src/main/java/net/minecraft/server/ChunkProviderServer.java @@ -38,20 +76,23 @@ 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..ce645a69bd 100644 +index e1e4ea793a..e61ddeb1ff 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 { +@@ -55,8 +55,10 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { + private static final Logger LOGGER = LogManager.getLogger(); public static final int GOLDEN_TICKET = 33 + ChunkStatus.b(); - public final Long2ObjectLinkedOpenHashMap updatingChunks = new Long2ObjectLinkedOpenHashMap(); +- public final Long2ObjectLinkedOpenHashMap updatingChunks = new Long2ObjectLinkedOpenHashMap(); - public volatile Long2ObjectLinkedOpenHashMap visibleChunks; -+ public final Long2ObjectLinkedOpenHashMap visibleChunks = new Long2ObjectLinkedOpenHashMap(); // Paper - remove copying, make mt safe -+ public transient Long2ObjectLinkedOpenHashMap visibleChunksClone; // Paper - remove copying, make mt safe ++ public final com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy updatingChunks = new com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy<>(); // Paper - faster copying ++ public final com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy visibleChunks = new com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy<>(); // Paper - faster copying ++ public final com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy pendingVisibleChunks = new com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy<>(); // Paper - this is used if the visible chunks is updated while iterating only ++ public transient Long2ObjectLinkedOpenHashMap visibleChunksClone; // Paper - used for async access of visible chunks, clone and cache only when needed private final Long2ObjectLinkedOpenHashMap pendingUnload; final LongSet loadedChunks; // Paper - private -> package public final WorldServer world; -@@ -170,7 +171,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -170,7 +172,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { public PlayerChunkMap(WorldServer worldserver, File file, DataFixer datafixer, DefinedStructureManager definedstructuremanager, Executor executor, IAsyncTaskHandler iasynctaskhandler, ILightAccess ilightaccess, ChunkGenerator chunkgenerator, WorldLoadListener worldloadlistener, Supplier supplier, int i) { super(new File(worldserver.getWorldProvider().getDimensionManager().a(file), "region"), datafixer); @@ -60,17 +101,17 @@ index e1e4ea793a..ce645a69bd 100644 this.pendingUnload = new Long2ObjectLinkedOpenHashMap(); this.loadedChunks = new LongOpenHashSet(); this.unloadQueue = new LongOpenHashSet(); -@@ -262,8 +263,49 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -262,9 +264,52 @@ 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; ++ private boolean hasPendingVisibleUpdate = 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()) { @@ -78,8 +119,10 @@ index e1e4ea793a..ce645a69bd 100644 + } + } finally { + this.isIterating = prev; -+ if (!this.isIterating && updatingChunksModified && !wasUpdating) { -+ this.updateVisibleChunks(); ++ if (!this.isIterating && this.hasPendingVisibleUpdate) { ++ this.visibleChunks.copyFrom(this.pendingVisibleChunks); ++ this.pendingVisibleChunks.clear(); ++ this.hasPendingVisibleUpdate = false; + } + } + } @@ -90,7 +133,7 @@ index e1e4ea793a..ce645a69bd 100644 + synchronized (this.visibleChunks) { + if (DEBUG_ASYNC_VISIBLE_CHUNKS) new Throwable("Async getVisibleChunks").printStackTrace(); + if (this.visibleChunksClone == null) { -+ this.visibleChunksClone = this.visibleChunks.clone(); ++ this.visibleChunksClone = this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.clone() : this.visibleChunks.clone(); + } + return this.visibleChunksClone; + } @@ -100,17 +143,19 @@ index e1e4ea793a..ce645a69bd 100644 + @Nullable public PlayerChunk getVisibleChunk(long i) { // Paper - protected -> public +- return (PlayerChunk) this.visibleChunks.get(i); + // Paper start - mt safe get + if (Thread.currentThread() != this.world.serverThread) { + synchronized (this.visibleChunks) { -+ return (PlayerChunk) this.visibleChunks.get(i); ++ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : this.visibleChunks.get(i)); + } + } ++ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : this.visibleChunks.get(i)); + // Paper end - return (PlayerChunk) this.visibleChunks.get(i); } -@@ -444,8 +486,9 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { + protected IntSupplier c(long i) { +@@ -444,8 +489,9 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { // Paper end protected void save(boolean flag) { @@ -121,7 +166,7 @@ index e1e4ea793a..ce645a69bd 100644 MutableBoolean mutableboolean = new MutableBoolean(); do { -@@ -473,7 +516,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -473,7 +519,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 { @@ -130,29 +175,29 @@ index e1e4ea793a..ce645a69bd 100644 IChunkAccess ichunkaccess = (IChunkAccess) playerchunk.getChunkSave().getNow(null); // CraftBukkit - decompile error if (ichunkaccess instanceof ProtoChunkExtension || ichunkaccess instanceof Chunk) { -@@ -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 +@@ -643,7 +689,20 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { + if (!this.updatingChunksModified) { return false; } else { - this.visibleChunks = this.updatingChunks.clone(); + // Paper start - stop cloning visibleChunks + synchronized (this.visibleChunks) { -+ this.visibleChunks.clear(); -+ this.visibleChunks.putAll(this.updatingChunks); -+ this.visibleChunksClone = null; ++ if (isIterating) { ++ hasPendingVisibleUpdate = true; ++ this.pendingVisibleChunks.copyFrom(this.updatingChunks); ++ } else { ++ hasPendingVisibleUpdate = false; ++ this.pendingVisibleChunks.clear(); ++ this.visibleChunks.copyFrom(this.updatingChunks); ++ this.visibleChunksClone = null; ++ } + } + // Paper end + this.updatingChunksModified = false; return true; } -@@ -1104,12 +1155,12 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -1104,12 +1163,12 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { } protected Iterable f() { diff --git a/Spigot-Server-Patches/0462-Speed-up-processing-of-chunk-loads-and-generation.patch b/Spigot-Server-Patches/0462-Speed-up-processing-of-chunk-loads-and-generation.patch index 2c0a6c89d..3009eff80 100644 --- a/Spigot-Server-Patches/0462-Speed-up-processing-of-chunk-loads-and-generation.patch +++ b/Spigot-Server-Patches/0462-Speed-up-processing-of-chunk-loads-and-generation.patch @@ -1,4 +1,4 @@ -From e245683a13582175b68dbfa9babb6bef2b3d2fd5 Mon Sep 17 00:00:00 2001 +From 665aad14dce0eb101630440132805dccc9f2aefe Mon Sep 17 00:00:00 2001 From: Aikar Date: Thu, 9 Apr 2020 00:09:26 -0400 Subject: [PATCH] Speed up processing of chunk loads and generation @@ -44,46 +44,8 @@ index 69e26a8267..434833d50e 100644 public static final Timing playerListTimer = Timings.ofSafe("Player List"); public static final Timing commandFunctionsTimer = Timings.ofSafe("Command Functions"); public static final Timing connectionTimer = Timings.ofSafe("Connection Handler"); -diff --git a/src/main/java/com/destroystokyo/paper/util/map/Long2ObjectLinkedOpenHashMapFastCopy.java b/src/main/java/com/destroystokyo/paper/util/map/Long2ObjectLinkedOpenHashMapFastCopy.java -new file mode 100644 -index 0000000000..e0ad725b2e ---- /dev/null -+++ b/src/main/java/com/destroystokyo/paper/util/map/Long2ObjectLinkedOpenHashMapFastCopy.java -@@ -0,0 +1,32 @@ -+package com.destroystokyo.paper.util.map; -+ -+import it.unimi.dsi.fastutil.longs.Long2ObjectLinkedOpenHashMap; -+ -+public class Long2ObjectLinkedOpenHashMapFastCopy extends Long2ObjectLinkedOpenHashMap { -+ -+ public void copyFrom(Long2ObjectLinkedOpenHashMapFastCopy map) { -+ if (key.length < map.key.length) { -+ key = null; -+ key = new long[map.key.length]; -+ } -+ if (value.length < map.value.length) { -+ value = null; -+ //noinspection unchecked -+ value = (V[]) new Object[map.value.length]; -+ } -+ if (link.length < map.link.length) { -+ link = null; -+ link = new long[map.link.length]; -+ } -+ System.arraycopy(map.key, 0, this.key, 0, map.key.length); -+ System.arraycopy(map.value, 0, this.value, 0, map.value.length); -+ System.arraycopy(map.link, 0, this.link, 0, map.link.length); -+ this.size = map.size; -+ this.mask = map.mask; -+ this.first = map.first; -+ this.last = map.last; -+ this.n = map.n; -+ this.maxFill = map.maxFill; -+ this.containsNullKey = map.containsNullKey; -+ } -+} diff --git a/src/main/java/net/minecraft/server/ChunkProviderServer.java b/src/main/java/net/minecraft/server/ChunkProviderServer.java -index e627440c41..15450f36a4 100644 +index e627440c41..bacfc4cba6 100644 --- a/src/main/java/net/minecraft/server/ChunkProviderServer.java +++ b/src/main/java/net/minecraft/server/ChunkProviderServer.java @@ -602,6 +602,7 @@ public class ChunkProviderServer extends IChunkProvider { @@ -119,7 +81,7 @@ index e627440c41..15450f36a4 100644 } } }); -@@ -893,6 +896,39 @@ public class ChunkProviderServer extends IChunkProvider { +@@ -893,6 +896,38 @@ public class ChunkProviderServer extends IChunkProvider { super.executeTask(runnable); } @@ -132,8 +94,7 @@ index e627440c41..15450f36a4 100644 + try { + // always try to load chunks as long as we aren't falling behind, restrain generation/other updates only. + boolean execChunkTask = com.destroystokyo.paper.io.chunk.ChunkTaskManager.pollChunkWaitQueue() || ChunkProviderServer.this.world.asyncChunkTaskManager.pollNextChunkTask(); // Paper -+ ChunkProviderServer.this.tickDistanceManager(); -+ if (execChunkTask) { ++ if (ChunkProviderServer.this.tickDistanceManager() || execChunkTask) { + continue; + } + long now = System.nanoTime(); @@ -202,96 +163,10 @@ index 06c395000f..936434110c 100644 protected TickTask postToMainThread(Runnable runnable) { return new TickTask(this.ticks, runnable); diff --git a/src/main/java/net/minecraft/server/PlayerChunkMap.java b/src/main/java/net/minecraft/server/PlayerChunkMap.java -index ce645a69bd..32fed8a39a 100644 +index e61ddeb1ff..92c9ab43d7 100644 --- a/src/main/java/net/minecraft/server/PlayerChunkMap.java +++ b/src/main/java/net/minecraft/server/PlayerChunkMap.java -@@ -55,8 +55,9 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - - private static final Logger LOGGER = LogManager.getLogger(); - public static final int GOLDEN_TICKET = 33 + ChunkStatus.b(); -- public final Long2ObjectLinkedOpenHashMap updatingChunks = new Long2ObjectLinkedOpenHashMap(); -- public final Long2ObjectLinkedOpenHashMap visibleChunks = new Long2ObjectLinkedOpenHashMap(); // Paper - remove copying, make mt safe -+ public final com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy updatingChunks = new com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy<>(); -+ public final com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy visibleChunks = new com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy<>(); // Paper - remove copying, make mt safe -+ public final com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy pendingVisibleChunks = new com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy<>(); // Paper - remove copying, this is used if the visible chunks is updated while iterating only - public transient Long2ObjectLinkedOpenHashMap visibleChunksClone; // Paper - remove copying, make mt safe - private final Long2ObjectLinkedOpenHashMap pendingUnload; - final LongSet loadedChunks; // Paper - private -> package -@@ -266,10 +267,10 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - // 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; -+ private boolean hasPendingVisibleUpdate = 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()) { -@@ -277,8 +278,9 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - } - } finally { - this.isIterating = prev; -- if (!this.isIterating && updatingChunksModified && !wasUpdating) { -- this.updateVisibleChunks(); -+ if (!this.isIterating && hasPendingVisibleUpdate) { -+ this.visibleChunks.copyFrom(this.pendingVisibleChunks); -+ this.pendingVisibleChunks.clear(); - } - } - } -@@ -289,7 +291,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - synchronized (this.visibleChunks) { - if (DEBUG_ASYNC_VISIBLE_CHUNKS) new Throwable("Async getVisibleChunks").printStackTrace(); - if (this.visibleChunksClone == null) { -- this.visibleChunksClone = this.visibleChunks.clone(); -+ this.visibleChunksClone = this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.clone() : this.visibleChunks.clone(); - } - return this.visibleChunksClone; - } -@@ -302,11 +304,11 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - // Paper start - mt safe get - if (Thread.currentThread() != this.world.serverThread) { - synchronized (this.visibleChunks) { -- return (PlayerChunk) this.visibleChunks.get(i); -+ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : this.visibleChunks.get(i)); - } - } -+ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : this.visibleChunks.get(i)); - // Paper end -- return (PlayerChunk) this.visibleChunks.get(i); - } - - protected IntSupplier c(long i) { -@@ -682,16 +684,21 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { - }); - } - -- protected boolean updateVisibleChunks() { return b(); } // Paper - OBFHELPER - protected boolean b() { -- if (!this.updatingChunksModified || this.isIterating) { // Paper -+ if (!this.updatingChunksModified) { - return false; - } else { - // Paper start - stop cloning visibleChunks - synchronized (this.visibleChunks) { -- this.visibleChunks.clear(); -- this.visibleChunks.putAll(this.updatingChunks); -- this.visibleChunksClone = null; -+ if (isIterating) { -+ hasPendingVisibleUpdate = true; -+ this.pendingVisibleChunks.copyFrom(this.updatingChunks); -+ } else { -+ hasPendingVisibleUpdate = false; -+ this.pendingVisibleChunks.clear(); -+ this.visibleChunks.copyFrom(this.updatingChunks); -+ this.visibleChunksClone = null; -+ } - } - // Paper end - -@@ -785,7 +792,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { +@@ -793,7 +793,7 @@ public class PlayerChunkMap extends IChunkLoader implements PlayerChunk.d { CompletableFuture> ret = new CompletableFuture<>(); Consumer chunkHolderConsumer = (ChunkRegionLoader.InProgressChunkHolder holder) -> {