#971: Remove strong chunk reference in PDC

A previous fix for SPIGOT-6814 implemented a callback function for the
PDC implementation that could be set to actively define a chunk as
unsaved, allowing chunks that have not been mutated through block
changes to still require saving if the chunks pdc was mutated.

This implementation however would pass a callback that references the
chunk access internally, meaning the PDC now actively holds onto a
callback that holds a reference to the entire chunk.

Aditionally, this change also impacted the pdc for item metas and
entities for really no reason whatsoever.

This commit re-implements the fix by introducing a new child of the pdc
implementation that the chunk now uses as its pdc. This specific
implementation maintains a dirty flag that is set to `true` on any form
of mutation and set back to false by the chunk that owns the PDC
whenever the chunk itself is flag as no longer dirty.

nms-patches/net/minecraft/world/level/chunk/Chunk.patch

@@ -39,19 +39,18 @@
      public Chunk(WorldServer worldserver, ProtoChunk protochunk, @Nullable Chunk.c chunk_c) {
          this(worldserver, protochunk.getPos(), protochunk.getUpgradeData(), protochunk.unpackBlockTicks(), protochunk.unpackFluidTicks(), protochunk.getInhabitedTime(), protochunk.getSections(), chunk_c, protochunk.getBlendingData());
          Iterator iterator = protochunk.getBlockEntities().values().iterator();
-@@ -142,6 +154,11 @@
+@@ -142,6 +154,10 @@
  
          this.setLightCorrect(protochunk.isLightCorrect());
          this.unsaved = true;
 +        this.needsDecoration = true; // CraftBukkit
 +        // CraftBukkit start
 +        this.persistentDataContainer = protochunk.persistentDataContainer; // SPIGOT-6814: copy PDC to account for 1.17 to 1.18 chunk upgrading.
-+        this.persistentDataContainer.setCallback(() -> setUnsaved(true)); // SPIGOT-6814: Handle cases were only persistentData is saved
 +        // CraftBukkit end
      }
  
      @Override
-@@ -238,9 +255,16 @@
+@@ -238,9 +254,16 @@
          }
      }
  
@@ -68,7 +67,7 @@
          int i = blockposition.getY();
          ChunkSection chunksection = this.getSection(this.getSectionIndex(i));
          boolean flag1 = chunksection.hasOnlyAir();
-@@ -279,7 +303,8 @@
+@@ -279,7 +302,8 @@
                  if (!chunksection.getBlockState(j, k, l).is(block)) {
                      return null;
                  } else {
@@ -78,7 +77,7 @@
                          iblockdata.onPlace(this.level, blockposition, iblockdata1, flag);
                      }
  
-@@ -324,7 +349,12 @@
+@@ -324,7 +348,12 @@
  
      @Nullable
      public TileEntity getBlockEntity(BlockPosition blockposition, Chunk.EnumTileEntityState chunk_enumtileentitystate) {
@@ -92,7 +91,7 @@
  
          if (tileentity == null) {
              NBTTagCompound nbttagcompound = (NBTTagCompound) this.pendingBlockEntities.remove(blockposition);
-@@ -395,6 +425,13 @@
+@@ -395,6 +424,13 @@
                  tileentity1.setRemoved();
              }
  
@@ -106,7 +105,7 @@
          }
      }
  
-@@ -424,6 +461,12 @@
+@@ -424,6 +460,12 @@
          if (this.isInLevel()) {
              TileEntity tileentity = (TileEntity) this.blockEntities.remove(blockposition);
  
@@ -119,7 +118,7 @@
              if (tileentity != null) {
                  this.removeGameEventListener(tileentity);
                  tileentity.setRemoved();
-@@ -471,6 +514,55 @@
+@@ -471,6 +513,55 @@
  
      }
  
@@ -175,7 +174,7 @@
      public boolean isEmpty() {
          return false;
      }
-@@ -659,7 +751,7 @@
+@@ -659,7 +750,7 @@
  
      private <T extends TileEntity> void updateBlockEntityTicker(T t0) {
          IBlockData iblockdata = t0.getBlockState();
@@ -184,7 +183,7 @@
  
          if (blockentityticker == null) {
              this.removeBlockEntityTicker(t0.getBlockPos());
-@@ -752,7 +844,7 @@
+@@ -752,7 +843,7 @@
          private boolean loggedInvalidBlockState;
  
          a(TileEntity tileentity, BlockEntityTicker blockentityticker) {
@@ -193,7 +192,7 @@
              this.ticker = blockentityticker;
          }
  
-@@ -775,7 +867,7 @@
+@@ -775,7 +866,7 @@
                              this.loggedInvalidBlockState = true;
                              Chunk.LOGGER.warn("Block entity {} @ {} state {} invalid for ticking:", new org.apache.logging.log4j.util.Supplier[]{this::getType, this::getPos, () -> {
                                          return iblockdata;

nms-patches/net/minecraft/world/level/chunk/IChunkAccess.patch

@@ -6,25 +6,37 @@
  
 +    // CraftBukkit start - SPIGOT-6814: move to IChunkAccess to account for 1.17 to 1.18 chunk upgrading.
 +    private static final org.bukkit.craftbukkit.persistence.CraftPersistentDataTypeRegistry DATA_TYPE_REGISTRY = new org.bukkit.craftbukkit.persistence.CraftPersistentDataTypeRegistry();
-+    public org.bukkit.craftbukkit.persistence.CraftPersistentDataContainer persistentDataContainer = new org.bukkit.craftbukkit.persistence.CraftPersistentDataContainer(DATA_TYPE_REGISTRY);
++    public org.bukkit.craftbukkit.persistence.DirtyCraftPersistentDataContainer persistentDataContainer = new org.bukkit.craftbukkit.persistence.DirtyCraftPersistentDataContainer(DATA_TYPE_REGISTRY);
 +    // CraftBukkit end
 +
      public IChunkAccess(ChunkCoordIntPair chunkcoordintpair, ChunkConverter chunkconverter, LevelHeightAccessor levelheightaccessor, IRegistry<BiomeBase> iregistry, long i, @Nullable ChunkSection[] achunksection, @Nullable BlendingData blendingdata) {
          this.chunkPos = chunkcoordintpair;
          this.upgradeData = chunkconverter;
-@@ -94,7 +99,12 @@
+@@ -94,7 +99,11 @@
          }
  
          replaceMissingSections(levelheightaccessor, iregistry, this.sections);
 +        // CraftBukkit start
 +        this.biomeRegistry = iregistry;
-+        this.persistentDataContainer.setCallback(() -> setUnsaved(true)); // CraftBukkit - SPIGOT-6814: Handle cases were only persistentData is saved
      }
 +    public final IRegistry<BiomeBase> biomeRegistry;
 +    // CraftBukkit end
  
      private static void replaceMissingSections(LevelHeightAccessor levelheightaccessor, IRegistry<BiomeBase> iregistry, ChunkSection[] achunksection) {
          for (int i = 0; i < achunksection.length; ++i) {
+@@ -258,10 +267,11 @@
+ 
+     public void setUnsaved(boolean flag) {
+         this.unsaved = flag;
++        if (!flag) this.persistentDataContainer.dirty(false); // CraftBukkit - SPIGOT-6814: chunk was saved, pdc is no longer dirty
+     }
+ 
+     public boolean isUnsaved() {
+-        return this.unsaved;
++        return this.unsaved || this.persistentDataContainer.dirty(); // CraftBukkit - SPIGOT-6814: chunk is unsaved if pdc was mutated
+     }
+ 
+     public abstract ChunkStatus getStatus();
 @@ -394,6 +404,27 @@
          }
      }

src/main/java/org/bukkit/craftbukkit/persistence/CraftPersistentDataContainer.java

@@ -15,13 +15,11 @@ import org.bukkit.persistence.PersistentDataAdapterContext;
 import org.bukkit.persistence.PersistentDataContainer;
 import org.bukkit.persistence.PersistentDataType;
 
-public final class CraftPersistentDataContainer implements PersistentDataContainer {
+public class CraftPersistentDataContainer implements PersistentDataContainer {
 
-    private static final Callback EMPTY = () -> { };
     private final Map<String, NBTBase> customDataTags = new HashMap<>();
     private final CraftPersistentDataTypeRegistry registry;
     private final CraftPersistentDataAdapterContext adapterContext;
-    private Callback callback = EMPTY;
 
     public CraftPersistentDataContainer(Map<String, NBTBase> customTags, CraftPersistentDataTypeRegistry registry) {
         this(registry);
@@ -33,14 +31,6 @@ public final class CraftPersistentDataContainer implements PersistentDataContain
         this.adapterContext = new CraftPersistentDataAdapterContext(this.registry);
     }
 
-    public void setCallback(Callback callback) {
-        if (callback == null) {
-            this.callback = EMPTY;
-            return;
-        }
-
-        this.callback = callback;
-    }
 
     @Override
     public <T, Z> void set(NamespacedKey key, PersistentDataType<T, Z> type, Z value) {
@@ -49,7 +39,6 @@ public final class CraftPersistentDataContainer implements PersistentDataContain
         Validate.notNull(value, "The provided value for the custom value was null");
 
         this.customDataTags.put(key.toString(), registry.wrap(type.getPrimitiveType(), type.toPrimitive(value, adapterContext)));
-        callback.onValueChange();
     }
 
     @Override
@@ -103,7 +92,6 @@ public final class CraftPersistentDataContainer implements PersistentDataContain
         Validate.notNull(key, "The provided key for the custom value was null");
 
         this.customDataTags.remove(key.toString());
-        callback.onValueChange();
     }
 
     @Override
@@ -138,19 +126,16 @@ public final class CraftPersistentDataContainer implements PersistentDataContain
 
     public void put(String key, NBTBase base) {
         this.customDataTags.put(key, base);
-        callback.onValueChange();
     }
 
     public void putAll(Map<String, NBTBase> map) {
         this.customDataTags.putAll(map);
-        callback.onValueChange();
     }
 
     public void putAll(NBTTagCompound compound) {
         for (String key : compound.getAllKeys()) {
             this.customDataTags.put(key, compound.get(key));
         }
-        callback.onValueChange();
     }
 
     public Map<String, NBTBase> getRaw() {
@@ -171,9 +156,4 @@ public final class CraftPersistentDataContainer implements PersistentDataContain
     public Map<String, Object> serialize() {
         return (Map<String, Object>) CraftNBTTagConfigSerializer.serialize(toTagCompound());
     }
-
-    @FunctionalInterface
-    public interface Callback {
-        void onValueChange();
-    }
 }

src/main/java/org/bukkit/craftbukkit/persistence/DirtyCraftPersistentDataContainer.java

@@ -0,0 +1,62 @@
+package org.bukkit.craftbukkit.persistence;
+
+import java.util.Map;
+import net.minecraft.nbt.NBTBase;
+import net.minecraft.nbt.NBTTagCompound;
+import org.bukkit.NamespacedKey;
+import org.bukkit.persistence.PersistentDataType;
+
+/**
+ * A child class of the persistent data container that recalls if it has been
+ * mutated from an external caller.
+ */
+public final class DirtyCraftPersistentDataContainer extends CraftPersistentDataContainer {
+
+    private boolean dirty;
+
+    public DirtyCraftPersistentDataContainer(Map<String, NBTBase> customTags, CraftPersistentDataTypeRegistry registry) {
+        super(customTags, registry);
+    }
+
+    public DirtyCraftPersistentDataContainer(CraftPersistentDataTypeRegistry registry) {
+        super(registry);
+    }
+
+    public boolean dirty() {
+        return this.dirty;
+    }
+
+    public void dirty(final boolean dirty) {
+        this.dirty = dirty;
+    }
+
+    @Override
+    public <T, Z> void set(NamespacedKey key, PersistentDataType<T, Z> type, Z value) {
+        super.set(key, type, value);
+        this.dirty(true);
+    }
+
+    @Override
+    public void remove(NamespacedKey key) {
+        super.remove(key);
+        this.dirty(true);
+    }
+
+    @Override
+    public void put(String key, NBTBase base) {
+        super.put(key, base);
+        this.dirty(true);
+    }
+
+    @Override
+    public void putAll(NBTTagCompound compound) {
+        super.putAll(compound);
+        this.dirty(true);
+    }
+
+    @Override
+    public void putAll(Map<String, NBTBase> map) {
+        super.putAll(map);
+        this.dirty(true);
+    }
+}