Feature: Re-implement SmallThreadingDetector, this time in a way that doesn't crash when generating a world

common/src/main/java/malte0811/ferritecore/ducks/SmallThreadDetectable.java

@@ -0,0 +1,15 @@
+package malte0811.ferritecore.ducks;
+
+/**
+ * An object that can be acquired and released similar to the vanilla ThreadingDetector.
+ * Methods must only be called while synchronized on this object.
+ */
+public interface SmallThreadDetectable {
+    byte UNLOCKED = 0;
+    byte LOCKED = 1;
+    byte CRASHING = 2;
+
+    byte ferritecore$getState();
+
+    void ferritecore$setState(byte newState);
+}

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,60 @@
+package malte0811.ferritecore.mixin.threaddetec;
+
+import malte0811.ferritecore.ducks.SmallThreadDetectable;
+import malte0811.ferritecore.util.SmallThreadingDetector;
+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 implements SmallThreadDetectable {
+    @Shadow
+    @Final
+    @Mutable
+    private ThreadingDetector threadingDetector;
+
+    private byte ferritecore$threadingState = UNLOCKED;
+
+    @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() {
+        SmallThreadingDetector.acquire(this, "PalettedContainer");
+    }
+
+    /**
+     * @reason The vanilla ThreadingDetector field is null now, and replaced by SmallThreadingDetector
+     * @author malte0811
+     */
+    @Overwrite
+    public void release() {
+        SmallThreadingDetector.release(this);
+    }
+
+    @Override
+    public byte ferritecore$getState() {
+        return ferritecore$threadingState;
+    }
+
+    @Override
+    public void ferritecore$setState(byte newState) {
+        ferritecore$threadingState = newState;
+    }
+}

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

@@ -0,0 +1,152 @@
+package malte0811.ferritecore.util;
+
+import malte0811.ferritecore.ducks.SmallThreadDetectable;
+import net.minecraft.util.ThreadingDetector;
+
+import java.util.IdentityHashMap;
+import java.util.Map;
+import java.util.Objects;
+
+@SuppressWarnings("SynchronizationOnLocalVariableOrMethodParameter")
+public class SmallThreadingDetector {
+    public static void acquire(SmallThreadDetectable obj, String name) {
+        byte oldState;
+        synchronized (obj) {
+            oldState = obj.ferritecore$getState();
+            if (oldState == SmallThreadDetectable.UNLOCKED) {
+                // Fast path: previously unlocked, everything is fine
+                // Performance: Acquire lock, "non-atomic CAS", release lock
+                // Vanilla fast path: Acquire lock, atomic CAS (tryAcquire), release lock
+                // So this should be at least as fast as the vanilla version
+                obj.ferritecore$setState(SmallThreadDetectable.LOCKED);
+                return;
+                // Anything after this line will only run when we are going to crash, so performance is not a concern
+            } else if (oldState == SmallThreadDetectable.LOCKED) {
+                // Locking twice => start crash in synchronized block, release lock and wait
+                // for release from other thread
+                GlobalCrashHandler.startCrash(obj, name);
+                obj.ferritecore$setState(SmallThreadDetectable.CRASHING);
+            }
+        }
+        if (oldState == SmallThreadDetectable.LOCKED) {
+            // Locking twice
+            GlobalCrashHandler.crashAcquire(obj);
+        } else {
+            // already crashing, probably something like 3 acquires in a row
+            // The vanilla detector doesn't explicitly handle this case and will probably produce confusing output,
+            // this implementation throws an exception 1 second after the "main" threads have crashed instead.
+            GlobalCrashHandler.crashBystander(obj);
+        }
+    }
+
+    public static void release(SmallThreadDetectable obj) {
+        byte oldState;
+        synchronized (obj) {
+            oldState = obj.ferritecore$getState();
+            if (oldState == SmallThreadDetectable.LOCKED) {
+                // Fast path, same performance (both here and vanilla) as in acquire
+                obj.ferritecore$setState(SmallThreadDetectable.UNLOCKED);
+                return;
+            }
+        }
+        if (oldState == SmallThreadDetectable.CRASHING) {
+            // Acquire started a crash and is waiting for this thread to also be ready
+            GlobalCrashHandler.crashRelease(obj);
+        }
+        // Release without having acquired before: weird, but vanilla in principle allows it
+    }
+
+    /**
+     * This code only runs when preparing a threading crash, so none of it needs to be remotely fast
+     */
+    private static class GlobalCrashHandler {
+        private static final Object MONITOR = new Object();
+        // SmallThreadDetectable's currently involved in crashes
+        // Access to the map needs to be synchronized on MONITOR
+        private static final Map<SmallThreadDetectable, CrashingState> ACTIVE_CRASHES = new IdentityHashMap<>();
+
+        private static void startCrash(SmallThreadDetectable owner, String name) {
+            synchronized (MONITOR) {
+                ACTIVE_CRASHES.put(owner, new CrashingState(name));
+            }
+        }
+
+        private static void crashAcquire(SmallThreadDetectable owner) {
+            var state = getAndWait(owner, ThreadRole.ACQUIRE);
+            throw ThreadingDetector.makeThreadingException(state.name, state.acquireThread);
+        }
+
+        private static void crashRelease(SmallThreadDetectable owner) {
+            var state = getAndWait(owner, ThreadRole.RELEASE);
+            throw ThreadingDetector.makeThreadingException(state.name, state.releaseThread);
+        }
+
+        private static void crashBystander(SmallThreadDetectable owner) {
+            var state = getAndWait(owner, ThreadRole.BYSTANDER);
+            try {
+                Thread.sleep(1000);
+            } catch (InterruptedException x) {
+                Thread.currentThread().interrupt();
+            }
+            throw new RuntimeException(
+                    "Bystander to crash of type" + state.name + "on threads " + state.releaseThread + ", " + state.acquireThread
+            );
+        }
+
+        private static CrashingState getAndWait(SmallThreadDetectable owner, ThreadRole role) {
+            CrashingState result;
+            synchronized (MONITOR) {
+                result = Objects.requireNonNull(ACTIVE_CRASHES.get(owner));
+            }
+            result.waitUntilReady(role);
+            return result;
+        }
+    }
+
+    /**
+     * Data needed to produce the proper crash for race on a single SmallThreadingDetectable
+     */
+    private static class CrashingState {
+        final String name;
+        Thread acquireThread;
+        Thread releaseThread;
+
+        private CrashingState(String name) {
+            this.name = name;
+        }
+
+        public synchronized void waitUntilReady(ThreadRole role) {
+            // Update thread fields with the newly known one (we're synchronized on `this`, so we can just access them
+            // as we want)
+            if (role == ThreadRole.ACQUIRE) {
+                acquireThread = Thread.currentThread();
+            } else if (role == ThreadRole.RELEASE) {
+                releaseThread = Thread.currentThread();
+            }
+            // Notify other threads waiting for this crash to be ready
+            notifyAll();
+            try {
+                final long maxTotalTime = 10000;
+                final var start = System.currentTimeMillis();
+                while (acquireThread == null || releaseThread == null) {
+                    if (System.currentTimeMillis() - start > maxTotalTime) {
+                        // Crash without both threads present if we don't manage to "find" them within 10 seconds
+                        // Happens e.g. when a release call is just missing, vanilla would hang indefinitely instead
+                        // in this case
+                        throw new RuntimeException(
+                                "Threading detector crash did not find other thread, missing release call?"
+                        );
+                    }
+                    // Release lock on this for up to 10 seconds, or until the other threads are ready
+                    this.wait(maxTotalTime);
+                }
+            } catch (InterruptedException x) {
+                Thread.currentThread().interrupt();
+            }
+        }
+    }
+
+    private enum ThreadRole {
+        ACQUIRE, RELEASE, BYSTANDER
+    }
+}

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,102 @@
+package malte0811.ferritecore.util;
+
+import malte0811.ferritecore.ducks.SmallThreadDetectable;
+import net.minecraft.SharedConstants;
+import net.minecraft.util.ThreadingDetector;
+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 {
+    @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) {
+                    SmallThreadingDetector.acquire(obj, "test");
+                    SmallThreadingDetector.release(obj);
+                }
+                barrier.await();
+            }
+        };
+        for (int i = 0; i < numThreads; ++i)
+            runOnNewThread(acquireAndRelease);
+    }
+
+    @Test
+    public void testHandoff() throws InterruptedException {
+        var obj = new OwnedObject();
+        runOnNewThread(() -> SmallThreadingDetector.acquire(obj, "test")).join();
+        SmallThreadingDetector.release(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) {
+                    SmallThreadingDetector.acquire(obj, "test");
+                    SmallThreadingDetector.release(obj);
+                }
+            }, $ -> anyTripped.set(true));
+        Thread.sleep(1000);
+        Assertions.assertTrue(anyTripped.get());
+    }
+
+    @Test
+    public void testReleaseNoAcquire() {
+        var obj = new OwnedObject();
+        SmallThreadingDetector.release(obj);
+
+        ThreadingDetector detec = new ThreadingDetector("test");
+        detec.checkAndUnlock();
+    }
+
+    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 implements SmallThreadDetectable {
+        private byte state = 0;
+
+        @Override
+        public byte ferritecore$getState() {
+            return state;
+        }
+
+        @Override
+        public void ferritecore$setState(byte newState) {
+            state = newState;
+        }
+    }
+}

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,22 @@ 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 particular with render distance). 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 `byte` to `PalettedContainer` and using
+the `PalettedContainer` itself as the monitor for synchronization (nothing else synchronizes on them), with a slow
+global data structure for handling the actual crashing since this path should virtually never run. The performance
+should be nearly identical to vanilla, possibly even a little better: Vanilla's fast path consist mainly of a lock
+acquisition, an atomic CAS (behind some abstraction), and a lock release. This implementation replaces the atomic CAS
+with a non-atomic one, and does not need any additional atomic operations.
+
+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`