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

This reverts commit 1458a5c225931dfba65ac90b99b8ad5ebe0e1b31.
The vanilla implementation allows a thread to acquire, then pass the
instance to another thread (using proper synchronization) and then have
that thread handle the release. The new implementation does not support
that, at all. A similar approach will probably still work (states
UNLOCKED, LOCKED, CRASHING; locked sections for updates as in vanilla;
one global "crash handler"), but will require more time than I can
currently spend on it.

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

@@ -1,19 +0,0 @@
-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,7 +18,6 @@ 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();
@@ -51,10 +50,6 @@ 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

@@ -1,10 +0,0 @@
-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

@@ -1,50 +0,0 @@
-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

@@ -1,39 +0,0 @@
-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

@@ -1,15 +0,0 @@
-{
-  "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

@@ -1,43 +0,0 @@
-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

@@ -1,94 +0,0 @@
-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,7 +31,6 @@
     "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,7 +22,6 @@ loom {
                 "dedupmultipart",
                 "blockstatecache",
                 "dedupbakedquad",
-                "threaddetec",
         ].stream()
                 .<String> map({ s -> "ferritecore." + s + ".mixin.json" })
                 .collect(Collectors.toList())

summary.md

@@ -140,18 +140,3 @@ 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`