Feature: Replace ThreadingDetector by a single VarHandle-operated Thread field in PalettedContainer

common/src/main/java/malte0811/ferritecore/impl/ThreadingDetectorInstance.java

@@ -0,0 +1,19 @@
+package malte0811.ferritecore.impl;
+
+import malte0811.ferritecore.util.SmallThreadingDetector;
+import net.minecraft.world.level.chunk.PalettedContainer;
+
+// Class is not loaded unless threaddetec mixin is active
+public class ThreadingDetectorInstance {
+    public static final SmallThreadingDetector PALETTED_CONTAINER_DETECTOR;
+
+    static {
+        try {
+            //noinspection JavaReflectionMemberAccess (Field is added by Mixin)
+            var ownerField = PalettedContainer.class.getDeclaredField("ownerThread");
+            PALETTED_CONTAINER_DETECTOR = new SmallThreadingDetector(ownerField, "PalettedContainer");
+        } catch (IllegalAccessException | NoSuchFieldException e) {
+            throw new RuntimeException(e);
+        }
+    }
+}

common/src/main/java/malte0811/ferritecore/mixin/config/FerriteConfig.java

@@ -18,6 +18,7 @@ public class FerriteConfig {
     public static final Option DEDUP_QUADS;
     public static final Option COMPACT_FAST_MAP;
     public static final Option POPULATE_NEIGHBOR_TABLE;
+    public static final Option THREADING_DETECTOR;
 
     static {
         ConfigBuilder builder = new ConfigBuilder();
@@ -50,6 +51,10 @@ public class FerriteConfig {
                 "bakedQuadDeduplication",
                 "Deduplicate vertex data of baked quads in the basic model implementations"
         );
+        THREADING_DETECTOR = builder.createOption(
+                "smallThreadingDetector",
+                "Replace objects used to detect multi-threaded access to chunks by a much smaller field"
+        );
         COMPACT_FAST_MAP = builder.createOptInOption(
                 "compactFastMap",
                 "Use a slightly more compact, but also slightly slower representation for block states"

common/src/main/java/malte0811/ferritecore/mixin/threaddetec/Config.java

@@ -0,0 +1,10 @@
+package malte0811.ferritecore.mixin.threaddetec;
+
+import malte0811.ferritecore.mixin.config.FerriteConfig;
+import malte0811.ferritecore.mixin.config.FerriteMixinConfig;
+
+public class Config extends FerriteMixinConfig {
+    public Config() {
+        super(FerriteConfig.THREADING_DETECTOR);
+    }
+}

common/src/main/java/malte0811/ferritecore/mixin/threaddetec/PalettedContainerMixin.java

@@ -0,0 +1,50 @@
+package malte0811.ferritecore.mixin.threaddetec;
+
+import malte0811.ferritecore.impl.ThreadingDetectorInstance;
+import net.minecraft.util.ThreadingDetector;
+import net.minecraft.world.level.chunk.PalettedContainer;
+import org.spongepowered.asm.mixin.*;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Inject;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
+
+@Mixin(PalettedContainer.class)
+public class PalettedContainerMixin {
+    @Shadow
+    @Final
+    @Mutable
+    private ThreadingDetector threadingDetector;
+
+    // Public for easy VarHandle use
+    public Thread ownerThread;
+
+    @Inject(
+            method = {
+                    "<init>(Lnet/minecraft/core/IdMap;Ljava/lang/Object;Lnet/minecraft/world/level/chunk/PalettedContainer$Strategy;)V",
+                    "<init>(Lnet/minecraft/core/IdMap;Lnet/minecraft/world/level/chunk/PalettedContainer$Strategy;Lnet/minecraft/world/level/chunk/PalettedContainer$Data;)V",
+                    "<init>(Lnet/minecraft/core/IdMap;Lnet/minecraft/world/level/chunk/PalettedContainer$Strategy;Lnet/minecraft/world/level/chunk/PalettedContainer$Configuration;Lnet/minecraft/util/BitStorage;Ljava/util/List;)V",
+            },
+            at = @At("TAIL")
+    )
+    public void redirectBuildThreadingDetector(CallbackInfo ci) {
+        this.threadingDetector = null;
+    }
+
+    /**
+     * @reason The vanilla ThreadingDetector field is null now, and replaced by SmallThreadingDetector
+     * @author malte0811
+     */
+    @Overwrite
+    public void acquire() {
+        ThreadingDetectorInstance.PALETTED_CONTAINER_DETECTOR.acquire(this);
+    }
+
+    /**
+     * @reason The vanilla ThreadingDetector field is null now, and replaced by SmallThreadingDetector
+     * @author malte0811
+     */
+    @Overwrite
+    public void release() {
+        ThreadingDetectorInstance.PALETTED_CONTAINER_DETECTOR.release(this);
+    }
+}

common/src/main/java/malte0811/ferritecore/util/SmallThreadingDetector.java

@@ -0,0 +1,39 @@
+package malte0811.ferritecore.util;
+
+import com.google.common.base.Preconditions;
+import net.minecraft.util.ThreadingDetector;
+
+import java.lang.invoke.MethodHandles;
+import java.lang.invoke.VarHandle;
+import java.lang.reflect.Field;
+
+public class SmallThreadingDetector {
+    private final VarHandle ownerThread;
+    private final String name;
+
+    public SmallThreadingDetector(Field ownerThread, String name) throws IllegalAccessException {
+        Preconditions.checkArgument(ownerThread.getType() == Thread.class);
+        this.ownerThread = MethodHandles.lookup().unreflectVarHandle(ownerThread);
+        this.name = name;
+    }
+
+    public void acquire(Object obj) {
+        Thread currentThread = Thread.currentThread();
+        var prevOwner = (Thread) ownerThread.getAndSet(obj, currentThread);
+        if (prevOwner == null) {
+            return;
+        }
+        // Tried to acquire a detector that was already owned by a different thread
+        throw ThreadingDetector.makeThreadingException(name, prevOwner);
+    }
+
+    public void release(Object obj) {
+        Thread currentThread = Thread.currentThread();
+        var prevOwner = (Thread) ownerThread.getAndSet(obj, null);
+        if (prevOwner == currentThread) {
+            return;
+        }
+        // Some other thread tried to acquire while we owned the detector
+        throw ThreadingDetector.makeThreadingException(name, prevOwner);
+    }
+}

common/src/main/resources/ferritecore.threaddetec.mixin.json

@@ -0,0 +1,15 @@
+{
+  "required": true,
+  "package": "malte0811.ferritecore.mixin.threaddetec",
+  "compatibilityLevel": "JAVA_17",
+  "client": [
+  ],
+  "injectors": {
+    "defaultRequire": 1
+  },
+  "minVersion": "0.8",
+  "plugin": "malte0811.ferritecore.mixin.threaddetec.Config",
+  "mixins": [
+    "PalettedContainerMixin"
+  ]
+}

common/src/test/java/malte0811/ferritecore/util/FakeGameVersion.java

@@ -0,0 +1,43 @@
+package malte0811.ferritecore.util;
+
+import net.minecraft.WorldVersion;
+import net.minecraft.world.level.storage.DataVersion;
+
+import java.util.Date;
+
+public class FakeGameVersion implements WorldVersion {
+    @Override
+    public DataVersion getDataVersion() {
+        return new DataVersion(0);
+    }
+
+    @Override
+    public String getId() {
+        return "test";
+    }
+
+    @Override
+    public String getName() {
+        return "test";
+    }
+
+    @Override
+    public String getReleaseTarget() {
+        return "none";
+    }
+
+    @Override
+    public int getProtocolVersion() {
+        return 0;
+    }
+
+    @Override
+    public Date getBuildTime() {
+        return new Date(0);
+    }
+
+    @Override
+    public boolean isStable() {
+        return false;
+    }
+}

common/src/test/java/malte0811/ferritecore/util/SmallThreadingDetectorTest.java

@@ -0,0 +1,94 @@
+package malte0811.ferritecore.util;
+
+import net.minecraft.ReportedException;
+import net.minecraft.SharedConstants;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.function.Executable;
+
+import java.util.concurrent.CyclicBarrier;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.function.Consumer;
+
+class SmallThreadingDetectorTest {
+    private static final SmallThreadingDetector DETECTOR;
+
+    static {
+        try {
+            DETECTOR = new SmallThreadingDetector(
+                    OwnedObject.class.getDeclaredField("owner"), "Test threading detector"
+            );
+        } catch (IllegalAccessException | NoSuchFieldException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
+    @BeforeAll
+    static void setup() {
+        SharedConstants.setVersion(new FakeGameVersion());
+    }
+
+    @Test
+    public void testSynchronized() {
+        var obj = new OwnedObject();
+        var monitor = new Object();
+        final int numThreads = 10;
+        var barrier = new CyclicBarrier(numThreads);
+        Executable acquireAndRelease = () -> {
+            for (int i = 0; i < 10; ++i) {
+                synchronized (monitor) {
+                    DETECTOR.acquire(obj);
+                    DETECTOR.release(obj);
+                }
+                barrier.await();
+            }
+        };
+        for (int i = 0; i < numThreads; ++i)
+            runOnNewThread(acquireAndRelease);
+    }
+
+    @Test
+    public void testUnsynchronized() throws InterruptedException {
+        var obj = new OwnedObject();
+        runOnNewThread(() -> DETECTOR.acquire(obj)).join();
+        Assertions.assertThrows(ReportedException.class, () -> DETECTOR.acquire(obj));
+    }
+
+    @Test
+    // This isn't guaranteed to pass, but in practice it always will, and that's good enough
+    public void testRace() throws InterruptedException {
+        var obj = new OwnedObject();
+        AtomicBoolean anyTripped = new AtomicBoolean(false);
+        for (int i = 0; i < 10; ++i)
+            runOnNewThread(() -> {
+                final long start = System.currentTimeMillis();
+                while (!anyTripped.get() && System.currentTimeMillis() - start < 1000) {
+                    DETECTOR.acquire(obj);
+                    DETECTOR.release(obj);
+                }
+            }, $ -> anyTripped.set(true));
+        Thread.sleep(1000);
+        Assertions.assertTrue(anyTripped.get());
+    }
+
+    private static Thread runOnNewThread(Executable toRun) {
+        return runOnNewThread(toRun, Throwable::printStackTrace);
+    }
+
+    private static Thread runOnNewThread(Executable toRun, Consumer<Throwable> onXCP) {
+        var thread = new Thread(() -> {
+            try {
+                toRun.execute();
+            } catch (Throwable e) {
+                onXCP.accept(e);
+            }
+        });
+        thread.start();
+        return thread;
+    }
+
+    public static class OwnedObject {
+        public Thread owner;
+    }
+}

fabric/src/main/resources/fabric.mod.json

@@ -31,6 +31,7 @@
     "ferritecore.mrl.mixin.json",
     "ferritecore.predicates.mixin.json",
     "ferritecore.dedupbakedquad.mixin.json",
+    "ferritecore.threaddetec.mixin.json",
     "ferritecore.fabric.mixin.json"
   ],
   "custom": {

forge/build.gradle

@@ -22,6 +22,7 @@ loom {
                 "dedupmultipart",
                 "blockstatecache",
                 "dedupbakedquad",
+                "threaddetec",
         ].stream()
                 .<String> map({ s -> "ferritecore." + s + ".mixin.json" })
                 .collect(Collectors.toList())

summary.md

@@ -140,3 +140,18 @@ Saved memory: Close to 150 MB
 CPU impact: Some during model loading, none afterwards  
 Side: client  
 Mixin subpackage: `bakedquad`
+
+### 9. `ThreadingDetector`
+
+Since 1.18 each `PalettedContainer` contains a `ThreadingDetector` to trigger a crash when multiple threads try to use
+the container at the same time. Since one of these exists for every loaded chunk section (actually two due to the
+implementation of `ImposterProtoChunk`) this adds up to around 10-15 MB with only one player and scales with the amount
+of loaded chunks. In singleplayer these objects exist for both server and client, so the amount is doubled.  
+The same effect can be achieved by adding a single field of type `Thread` to `PalettedContainer` and accessing it
+atomically using `VarHandle`. The most expensive operation in both implementations is one atomic memory access, the new
+implementation even saves an additional lock acquisition.
+
+Saved memory: 10-15 MB with only one player, more if more chunks are loaded. Does not depend on modpack size.  
+CPU impact: None or slightly negative  
+Side: Both  
+Mixin subpackage: `threaddetec`