From ade297307f53b2c1681dfee49a088e7a3ae80885 Mon Sep 17 00:00:00 2001 From: Aikar Date: Wed, 20 May 2020 00:51:28 -0400 Subject: [PATCH] Fix pooled buffer leak resulting in dynmap black spots - Fixes #3386 Dynmap accessed the raw bytes because it utilized NBT locally, but the NBTTagcompound was garbage collected while the bytes were still being used. This will return getBytes() back to being safe, and add a new PoolSafe method that will prevent the additional allocations for general chunk loading. Also fixed applyPatches for people with paths in their working directory if they have mcdev sources built. --- ...ze-NibbleArray-to-use-pooled-buffers.patch | 89 ++++++++++++++++++- scripts/applyPatches.sh | 4 +- 2 files changed, 90 insertions(+), 3 deletions(-) diff --git a/Spigot-Server-Patches/Optimize-NibbleArray-to-use-pooled-buffers.patch b/Spigot-Server-Patches/Optimize-NibbleArray-to-use-pooled-buffers.patch index f1c051ce44..4fa16cd135 100644 --- a/Spigot-Server-Patches/Optimize-NibbleArray-to-use-pooled-buffers.patch +++ b/Spigot-Server-Patches/Optimize-NibbleArray-to-use-pooled-buffers.patch @@ -12,6 +12,32 @@ diff --git a/src/main/java/net/minecraft/server/ChunkRegionLoader.java b/src/mai index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 --- a/src/main/java/net/minecraft/server/ChunkRegionLoader.java +++ b/src/main/java/net/minecraft/server/ChunkRegionLoader.java +@@ -0,0 +0,0 @@ public class ChunkRegionLoader { + if (flag) { + if (nbttagcompound2.hasKeyOfType("BlockLight", 7)) { + // Paper start - delay this task since we're executing off-main +- NibbleArray blockLight = new NibbleArray(nbttagcompound2.getByteArray("BlockLight")); ++ // Pool safe get and clean ++ NBTTagByteArray blockLightArray = nbttagcompound2.getByteArrayTag("BlockLight"); ++ // NibbleArray will copy the data in the ctor ++ NibbleArray blockLight = new NibbleArray(blockLightArray.getBytesPoolSafe()); ++ blockLightArray.cleanPooledBytes(); + // Note: We move the block light nibble array creation here for perf & in case the compound is modified + tasksToExecuteOnMain.add(() -> { + lightengine.a(EnumSkyBlock.BLOCK, SectionPosition.a(chunkcoordintpair, b0), blockLight); +@@ -0,0 +0,0 @@ public class ChunkRegionLoader { + + if (flag2 && nbttagcompound2.hasKeyOfType("SkyLight", 7)) { + // Paper start - delay this task since we're executing off-main +- NibbleArray skyLight = new NibbleArray(nbttagcompound2.getByteArray("SkyLight")); ++ // Pool safe get and clean ++ NBTTagByteArray skyLightArray = nbttagcompound2.getByteArrayTag("SkyLight"); ++ // NibbleArray will copy the data in the ctor ++ NibbleArray skyLight = new NibbleArray(skyLightArray.getBytesPoolSafe()); ++ skyLightArray.cleanPooledBytes(); + // Note: We move the block light nibble array creation here for perf & in case the compound is modified + tasksToExecuteOnMain.add(() -> { + lightengine.a(EnumSkyBlock.SKY, SectionPosition.a(chunkcoordintpair, b0), skyLight); @@ -0,0 +0,0 @@ public class ChunkRegionLoader { } @@ -73,13 +99,74 @@ index 0000000000000000000000000000000000000000..00000000000000000000000000000000 + NBTTagByteArray nbt = new NBTTagByteArray(abyte); + if (abyte.length == 2048) { + // register cleaner -+ MCUtil.registerCleaner(nbt, abyte, NibbleArray::releaseBytes); ++ nbt.cleaner = MCUtil.registerCleaner(nbt, abyte, NibbleArray::releaseBytes); + } + return nbt; + // Paper end } @Override +@@ -0,0 +0,0 @@ public class NBTTagByteArray extends NBTList { + } + + public byte[] getBytes() { ++ // Paper start ++ Runnable cleaner = this.cleaner; ++ if (cleaner != null) { // This will only be possible for 2048 byte arrays ++ // cleaners are thread safe if it tries to run twice, if getBytes is accessed concurrently, worse ++ // case is multiple clones ++ this.data = this.data.clone(); ++ this.cleaner = null; ++ cleaner.run(); ++ } ++ if (this.data == null) { ++ new Throwable("Horrible thing happened! Something hooked into Chunk Loading internals and accessed NBT after chunk was done loading. Please tell plugin to stop doing this, clone the memory before hand.").printStackTrace(); ++ } ++ return this.data; ++ } ++ Runnable cleaner; ++ public void cleanPooledBytes() { ++ Runnable cleaner = this.cleaner; ++ if (cleaner != null) { // This will only be possible for 2048 byte arrays ++ this.cleaner = null; ++ this.data = null; ++ cleaner.run(); ++ } ++ } ++ /** ++ * Use ONLY if you know the life of your usage of the bytes matches the life of the nbt node itself. ++ * If this NBT node can go unreachable before your usage of the bytes is over with, DO NOT use this ++ */ ++ public byte[] getBytesPoolSafe() { ++ // Paper end + return this.data; + } + +diff --git a/src/main/java/net/minecraft/server/NBTTagCompound.java b/src/main/java/net/minecraft/server/NBTTagCompound.java +index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 +--- a/src/main/java/net/minecraft/server/NBTTagCompound.java ++++ b/src/main/java/net/minecraft/server/NBTTagCompound.java +@@ -0,0 +0,0 @@ public class NBTTagCompound implements NBTBase { + return new byte[0]; + } + ++ // Paper start ++ public NBTTagByteArray getByteArrayTag(String s) { ++ try { ++ if (this.hasKeyOfType(s, 7)) { ++ return ((NBTTagByteArray) this.map.get(s)); ++ } ++ } catch (ClassCastException classcastexception) { ++ throw new ReportedException(this.a(s, NBTTagByteArray.a, classcastexception)); ++ } ++ ++ return new NBTTagByteArray(new byte[0]); ++ } ++ // Paper end ++ + public int[] getIntArray(String s) { + try { + if (this.hasKeyOfType(s, 11)) { diff --git a/src/main/java/net/minecraft/server/NibbleArray.java b/src/main/java/net/minecraft/server/NibbleArray.java index 0000000000000000000000000000000000000000..0000000000000000000000000000000000000000 100644 --- a/src/main/java/net/minecraft/server/NibbleArray.java diff --git a/scripts/applyPatches.sh b/scripts/applyPatches.sh index ef34ca0105..e4c073fda2 100755 --- a/scripts/applyPatches.sh +++ b/scripts/applyPatches.sh @@ -92,14 +92,14 @@ echo "Importing MC Dev" ./scripts/importmcdev.sh "$basedir" || exit 1 # Apply paper -cd "$basedir" ( applyPatch "work/Spigot/Spigot-API" Paper-API HEAD && applyPatch "work/Spigot/Spigot-Server" Paper-Server HEAD + cd "$basedir" # if we have previously ran ./paper mcdev, update it if [ -d "$workdir/Minecraft/$minecraftversion/src" ]; then - $basedir/scripts/makemcdevsrc.sh $basedir + ./scripts/makemcdevsrc.sh "$basedir" fi ) || ( echo "Failed to apply Paper Patches"