From 690f01f6044b830bddd12374b061aa3382f2679a Mon Sep 17 00:00:00 2001 From: Aikar Date: Wed, 20 May 2020 22:22:47 -0400 Subject: [PATCH] Protect the visible chunk map from plugins touching it, trim Timing Errors Blow up if a plugin tries to mutate visibleChunks directly and prevent them from doing so. Also provide a safe get call if any plugins directly call get on it so that it uses the special logic to check pending. Also restores ABI for the visibleChunks field back to what it was too. Additionally, remove the stack trace from Timings Stack Corruption for any error thrown on Minecraft Timings, and tell them to get the error ABOVE this instead, so people stop giving us useless error reports. Also fixes a memory leak when the source map down sizes but dest map didn't, which resulted in lingering references to old chunk holders. Fixes #3414 --- Spigot-API-Patches/Timings-v2.patch | 7 +-- ...hunkMap-memory-use-for-visibleChunks.patch | 56 ++++++++++++++----- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/Spigot-API-Patches/Timings-v2.patch b/Spigot-API-Patches/Timings-v2.patch index 765708d8a..50cb9c110 100644 --- a/Spigot-API-Patches/Timings-v2.patch +++ b/Spigot-API-Patches/Timings-v2.patch @@ -608,13 +608,12 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + TimingHandler last; + while ((last = TIMING_STACK.removeLast()) != this) { + last.timingDepth = 0; -+ String reportTo; + if ("Minecraft".equalsIgnoreCase(last.identifier.group)) { -+ reportTo = "Paper! This is a potential bug in Paper"; ++ Logger.getGlobal().log(Level.SEVERE, "TIMING_STACK_CORRUPTION - Look above this for any errors and report this to Paper unless it has a plugin in the stack trace (" + last.identifier + " did not stopTiming)"); + } else { -+ reportTo = "the plugin " + last.identifier.group + "(Look for errors above this in the logs)"; ++ Logger.getGlobal().log(Level.SEVERE, "TIMING_STACK_CORRUPTION - Report this to the plugin " + last.identifier.group + " (Look for errors above this in the logs) (" + last.identifier + " did not stopTiming)", new Throwable()); + } -+ Logger.getGlobal().log(Level.SEVERE, "TIMING_STACK_CORRUPTION - Report this to " + reportTo + " (" + last.identifier + " did not stopTiming)", new Throwable()); ++ + boolean found = TIMING_STACK.contains(this); + if (!found) { + // We aren't even in the stack... Don't pop everything 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 bec73302e..1bc10e94d 100644 --- a/Spigot-Server-Patches/Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch +++ b/Spigot-Server-Patches/Optimize-PlayerChunkMap-memory-use-for-visibleChunks.patch @@ -24,16 +24,16 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 +public class Long2ObjectLinkedOpenHashMapFastCopy extends Long2ObjectLinkedOpenHashMap { + + public void copyFrom(Long2ObjectLinkedOpenHashMapFastCopy map) { -+ if (key.length < map.key.length) { ++ if (key.length != map.key.length) { + key = null; + key = new long[map.key.length]; + } -+ if (value.length < map.value.length) { ++ if (value.length != map.value.length) { + value = null; + //noinspection unchecked + value = (V[]) new Object[map.value.length]; + } -+ if (link.length < map.link.length) { ++ if (link.length != map.link.length) { + link = null; + link = new long[map.link.length]; + } @@ -48,6 +48,13 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + this.maxFill = map.maxFill; + this.containsNullKey = map.containsNullKey; + } ++ ++ @Override ++ public Long2ObjectLinkedOpenHashMapFastCopy clone() { ++ Long2ObjectLinkedOpenHashMapFastCopy clone = (Long2ObjectLinkedOpenHashMapFastCopy) super.clone(); ++ clone.copyFrom(this); ++ return clone; ++ } +} diff --git a/src/main/java/net/minecraft/server/ChunkProviderServer.java b/src/main/java/net/minecraft/server/ChunkProviderServer.java index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 @@ -85,10 +92,33 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 public static final int GOLDEN_TICKET = 33 + ChunkStatus.b(); - public final Long2ObjectLinkedOpenHashMap updatingChunks = new Long2ObjectLinkedOpenHashMap(); - public volatile Long2ObjectLinkedOpenHashMap visibleChunks; -+ 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 ++ // Paper start - faster copying ++ public final Long2ObjectLinkedOpenHashMap updatingChunks = new com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy<>(); // Paper - faster copying ++ public final Long2ObjectLinkedOpenHashMap visibleChunks = new ProtectedVisibleChunksMap(); // Paper - faster copying ++ ++ private class ProtectedVisibleChunksMap extends com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy { ++ @Override ++ public PlayerChunk put(long k, PlayerChunk playerChunk) { ++ throw new UnsupportedOperationException("Updating visible Chunks"); ++ } ++ ++ @Override ++ public PlayerChunk remove(long k) { ++ throw new UnsupportedOperationException("Removing visible Chunks"); ++ } ++ ++ @Override ++ public PlayerChunk get(long k) { ++ return PlayerChunkMap.this.getVisibleChunk(k); ++ } ++ ++ public PlayerChunk safeGet(long k) { ++ return super.get(k); ++ } ++ } ++ // Paper end ++ 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 com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy 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; @@ -120,7 +150,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + } finally { + this.isIterating = prev; + if (!this.isIterating && this.hasPendingVisibleUpdate) { -+ this.visibleChunks.copyFrom(this.pendingVisibleChunks); ++ ((ProtectedVisibleChunksMap)this.visibleChunks).copyFrom(this.pendingVisibleChunks); + this.pendingVisibleChunks.clear(); + this.hasPendingVisibleUpdate = false; + } @@ -133,7 +163,7 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + synchronized (this.visibleChunks) { + if (DEBUG_ASYNC_VISIBLE_CHUNKS) new Throwable("Async getVisibleChunks").printStackTrace(); + if (this.visibleChunksClone == null) { -+ this.visibleChunksClone = this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.clone() : this.visibleChunks.clone(); ++ this.visibleChunksClone = this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.clone() : ((ProtectedVisibleChunksMap)this.visibleChunks).clone(); + } + return this.visibleChunksClone; + } @@ -147,10 +177,10 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + // Paper start - mt safe get + if (Thread.currentThread() != this.world.serverThread) { + synchronized (this.visibleChunks) { -+ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : this.visibleChunks.get(i)); ++ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : ((ProtectedVisibleChunksMap)this.visibleChunks).safeGet(i)); + } + } -+ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : this.visibleChunks.get(i)); ++ return (PlayerChunk) (this.hasPendingVisibleUpdate ? this.pendingVisibleChunks.get(i) : ((ProtectedVisibleChunksMap)this.visibleChunks).safeGet(i)); + // Paper end } @@ -184,11 +214,11 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + synchronized (this.visibleChunks) { + if (isIterating) { + hasPendingVisibleUpdate = true; -+ this.pendingVisibleChunks.copyFrom(this.updatingChunks); ++ this.pendingVisibleChunks.copyFrom((com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy)this.updatingChunks); + } else { + hasPendingVisibleUpdate = false; + this.pendingVisibleChunks.clear(); -+ this.visibleChunks.copyFrom(this.updatingChunks); ++ ((ProtectedVisibleChunksMap)this.visibleChunks).copyFrom((com.destroystokyo.paper.util.map.Long2ObjectLinkedOpenHashMapFastCopy)this.updatingChunks); + this.visibleChunksClone = null; + } + }