From e1c451968c2ac156d00edf01a8b9d824b63259ad Mon Sep 17 00:00:00 2001 From: Aikar Date: Fri, 22 May 2020 18:58:26 -0400 Subject: [PATCH] Fix high memory use of non ticking chunks The nibble pooling for NBT Tags was 'semi' leaked from loaded chunks as we store the NBT Tag of Tile Entities in a Chunk, but don't process them and remove them until chunk reaches Entity Ticking status.... This caused some phantom references to persist causing high memory use of these chunks. So I just got rid of pooling from NBT deserialization and we'll have to take the hit on memory allocations there because too many cascading concerns with anyone using NBT Tag Byte Arrays. Fixes #3431 --- ...ze-NibbleArray-to-use-pooled-buffers.patch | 201 +++++++----------- 1 file changed, 73 insertions(+), 128 deletions(-) diff --git a/Spigot-Server-Patches/0522-Optimize-NibbleArray-to-use-pooled-buffers.patch b/Spigot-Server-Patches/0522-Optimize-NibbleArray-to-use-pooled-buffers.patch index 87917a7f79..d0c978a22c 100644 --- a/Spigot-Server-Patches/0522-Optimize-NibbleArray-to-use-pooled-buffers.patch +++ b/Spigot-Server-Patches/0522-Optimize-NibbleArray-to-use-pooled-buffers.patch @@ -9,50 +9,20 @@ an object pool for these. Uses lots of advanced new capabilities of the Paper codebase :) diff --git a/src/main/java/net/minecraft/server/ChunkRegionLoader.java b/src/main/java/net/minecraft/server/ChunkRegionLoader.java -index e625842e524f18e469f7695b27d52d4d04892266..49d95bb12083c306c8d257b202735066bad4388b 100644 +index e625842e524f18e469f7695b27d52d4d04892266..d752e793f13a9caf5d255293f7ce9d562fd50064 100644 --- a/src/main/java/net/minecraft/server/ChunkRegionLoader.java +++ b/src/main/java/net/minecraft/server/ChunkRegionLoader.java -@@ -105,7 +105,11 @@ 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().markPoolSafe().cloneAndSet(blockLightArray.getBytesPoolSafe()); // This is going to light engine which handles releasing -+ 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); -@@ -115,7 +119,11 @@ 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().markPoolSafe().cloneAndSet(skyLightArray.getBytesPoolSafe()); // This is going to light engine which handles releasing -+ 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); -@@ -387,11 +395,15 @@ public class ChunkRegionLoader { +@@ -387,11 +387,11 @@ public class ChunkRegionLoader { } if (nibblearray != null && !nibblearray.c()) { - nbttagcompound2.setByteArray("BlockLight", nibblearray.asBytes()); -+ byte[] bytes = nibblearray.getCloneIfSet(); // Paper -+ nbttagcompound2.setByteArray("BlockLight", bytes); // Paper -+ MCUtil.registerCleaner(nbttagcompound2, bytes, NibbleArray::releaseBytes); // Paper - we can't really hook when this chunk is done without a ton of yuck efort, so just reclaim it once GC gets to it. ++ nbttagcompound2.setByteArray("BlockLight", nibblearray.asBytesPoolSafe().clone()); // Paper } if (nibblearray1 != null && !nibblearray1.c()) { - nbttagcompound2.setByteArray("SkyLight", nibblearray1.asBytes()); -+ byte[] bytes = nibblearray1.getCloneIfSet(); // Paper -+ nbttagcompound2.setByteArray("SkyLight", bytes); // Paper -+ MCUtil.registerCleaner(nbttagcompound2, bytes, NibbleArray::releaseBytes); // Paper - we can't really hook when this chunk is done without a ton of yuck efort, so just reclaim it once GC gets to it. ++ nbttagcompound2.setByteArray("SkyLight", nibblearray1.asBytesPoolSafe().clone()); // Paper } nbttaglist.add(nbttagcompound2); @@ -92,7 +62,7 @@ index 278aec8846d3bd448e359095063a711e78213ee5..f17b16d5c52cd77dd53807222dff4631 } diff --git a/src/main/java/net/minecraft/server/LightEngineStorageSky.java b/src/main/java/net/minecraft/server/LightEngineStorageSky.java -index 06bc8371fe9de4d23fdd47e5a3919541bb399fd8..c15fbd8602d33955d8a625d4a86cdcd8ca7489f5 100644 +index 06bc8371fe9de4d23fdd47e5a3919541bb399fd8..097f58e9ac3f4096d3b9dad75b6ebe76021fa92c 100644 --- a/src/main/java/net/minecraft/server/LightEngineStorageSky.java +++ b/src/main/java/net/minecraft/server/LightEngineStorageSky.java @@ -166,9 +166,9 @@ public class LightEngineStorageSky extends LightEngineStorage { - com.google.common.base.Preconditions.checkArgument( j < 1 << 24); // Spigot +@@ -197,7 +197,7 @@ public class LightEngineStorageSky extends LightEngineStorage { - } - - 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 02a2ed1baa3f82d302432b7bc627f3179751f886..5f38c962115f732fae20b61410dfc35b09248f4c 100644 ---- a/src/main/java/net/minecraft/server/NBTTagCompound.java -+++ b/src/main/java/net/minecraft/server/NBTTagCompound.java -@@ -298,6 +298,20 @@ 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)) { +- Arrays.fill(this.a(i, true).asBytes(), (byte) -1); ++ Arrays.fill(this.a(i, true).asBytesPoolSafe(), (byte) -1); // Paper + k = SectionPosition.c(SectionPosition.b(i)); + l = SectionPosition.c(SectionPosition.c(i)); + int i1 = SectionPosition.c(SectionPosition.d(i)); diff --git a/src/main/java/net/minecraft/server/NibbleArray.java b/src/main/java/net/minecraft/server/NibbleArray.java -index 996c8326387b5a7fe62db6a76e000144565cb85b..073e16a4f08bf00b7d9187a5bfdfdbf84ed05593 100644 +index 996c8326387b5a7fe62db6a76e000144565cb85b..8cedfdd820cc02a76607b53e0b054fc74654f907 100644 --- a/src/main/java/net/minecraft/server/NibbleArray.java +++ b/src/main/java/net/minecraft/server/NibbleArray.java -@@ -1,16 +1,73 @@ +@@ -1,16 +1,75 @@ package net.minecraft.server; +import com.destroystokyo.paper.util.pooled.PooledObjects; // Paper ++ ++import javax.annotation.Nonnull; import javax.annotation.Nullable; public class NibbleArray { @@ -272,7 +168,7 @@ index 996c8326387b5a7fe62db6a76e000144565cb85b..073e16a4f08bf00b7d9187a5bfdfdbf8 if (abyte.length != 2048) { throw (IllegalArgumentException) SystemUtils.c(new IllegalArgumentException("ChunkNibbleArrays should be 2048 bytes not: " + abyte.length)); } -@@ -44,7 +101,8 @@ public class NibbleArray { +@@ -44,7 +103,8 @@ public class NibbleArray { public void a(int i, int j) { // PAIL: private -> public if (this.a == null) { @@ -282,17 +178,36 @@ index 996c8326387b5a7fe62db6a76e000144565cb85b..073e16a4f08bf00b7d9187a5bfdfdbf8 } int k = this.d(i); -@@ -65,7 +123,8 @@ public class NibbleArray { - +@@ -66,14 +126,36 @@ public class NibbleArray { public byte[] asBytes() { if (this.a == null) { -- this.a = new byte[2048]; + this.a = new byte[2048]; ++ } else { // Paper start ++ // Accessor may need this object past garbage collection so need to clone it and return pooled value ++ // If we know its safe for pre GC access, use asBytesPoolSafe(). If you just need read, use getIfSet() ++ Runnable cleaner = this.cleaner; ++ if (cleaner != null) { ++ this.a = this.a.clone(); ++ cleaner.run(); // release the previously pooled value ++ this.cleaner = null; ++ } ++ } ++ // Paper end ++ ++ return this.a; ++ } ++ ++ @Nonnull ++ public byte[] asBytesPoolSafe() { ++ if (this.a == null) { + this.a = BYTE_2048.acquire(); // Paper -+ registerCleaner();// Paper ++ registerCleaner(); // Paper } ++ //noinspection ConstantConditions return this.a; -@@ -73,7 +132,7 @@ public class NibbleArray { + } ++ // Paper end public NibbleArray copy() { return this.b(); } // Paper - OBFHELPER public NibbleArray b() { @@ -302,9 +217,18 @@ index 996c8326387b5a7fe62db6a76e000144565cb85b..073e16a4f08bf00b7d9187a5bfdfdbf8 public String toString() { diff --git a/src/main/java/net/minecraft/server/NibbleArrayFlat.java b/src/main/java/net/minecraft/server/NibbleArrayFlat.java -index 67c960292db9d99ac85b5d0dda50ae48ef942c1b..f7641156beea365a91a935667abf8c9539896dc0 100644 +index 67c960292db9d99ac85b5d0dda50ae48ef942c1b..5e3efa1fa6c089df35971ce5c83da384f7dbd402 100644 --- a/src/main/java/net/minecraft/server/NibbleArrayFlat.java +++ b/src/main/java/net/minecraft/server/NibbleArrayFlat.java +@@ -8,7 +8,7 @@ public class NibbleArrayFlat extends NibbleArray { + + public NibbleArrayFlat(NibbleArray nibblearray, int i) { + super(128); +- System.arraycopy(nibblearray.asBytes(), i * 128, this.a, 0, 128); ++ System.arraycopy(nibblearray.getIfSet(), i * 128, this.a, 0, 128); // Paper + } + + @Override @@ -18,7 +18,7 @@ public class NibbleArrayFlat extends NibbleArray { @Override @@ -410,3 +334,24 @@ index cd1ad45469aa163b9bc41774ae80adfa617fd97b..1946aae7424593100279834baa89c19f } else { this.d &= ~(1 << k); if (nibblearray != null) { +diff --git a/src/main/java/org/bukkit/craftbukkit/CraftChunk.java b/src/main/java/org/bukkit/craftbukkit/CraftChunk.java +index 39ef95cbbb1d1d049354ae1e8991309e918d0462..ec3f560556c3056bdfc11cc6a7eb76420f13abad 100644 +--- a/src/main/java/org/bukkit/craftbukkit/CraftChunk.java ++++ b/src/main/java/org/bukkit/craftbukkit/CraftChunk.java +@@ -271,14 +271,14 @@ public class CraftChunk implements Chunk { + sectionSkyLights[i] = emptyLight; + } else { + sectionSkyLights[i] = new byte[2048]; +- System.arraycopy(skyLightArray.asBytes(), 0, sectionSkyLights[i], 0, 2048); ++ System.arraycopy(skyLightArray.getIfSet(), 0, sectionSkyLights[i], 0, 2048); // Paper + } + NibbleArray emitLightArray = lightengine.a(EnumSkyBlock.BLOCK).a(SectionPosition.a(x, i, z)); + if (emitLightArray == null) { + sectionEmitLights[i] = emptyLight; + } else { + sectionEmitLights[i] = new byte[2048]; +- System.arraycopy(emitLightArray.asBytes(), 0, sectionEmitLights[i], 0, 2048); ++ System.arraycopy(emitLightArray.getIfSet(), 0, sectionEmitLights[i], 0, 2048); // Paper + } + } + }