Add a config and split StateHolder optimizations in a way that allows disabling property map optimization without disabling neighbor state optimization

build.gradle

@@ -40,6 +40,7 @@ minecraft {
             workingDirectory project.file('run')
             arg "-mixin.config="+archivesBaseName+".mixins.json"
             property 'mixin.env.disableRefMap', 'true'
+            property 'mixin.debug.export', 'true'
 
 
             // Recommended logging level for the console

src/main/java/malte0811/ferritecore/FastMap.java

@@ -1,13 +1,11 @@
 package malte0811.ferritecore;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import net.minecraft.state.Property;
 
 import javax.annotation.Nullable;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 
 public class FastMap<Value> {
     private final List<Key<?>> keys;
@@ -82,6 +80,14 @@ public class FastMap<Value> {
         return rawKeys;
     }
 
+    public ImmutableMap<Property<?>, Comparable<?>> makeValuesFor(int index) {
+        ImmutableMap.Builder<Property<?>, Comparable<?>> result = ImmutableMap.builder();
+        for (Property<?> p : getProperties()) {
+            result.put(p, Objects.requireNonNull(getValue(index, p)));
+        }
+        return result.build();
+    }
+
     private static class Key<T extends Comparable<T>> {
         private final Property<T> property;
         private final List<T> values;

src/main/java/malte0811/ferritecore/ducks/FastMapStateHolder.java

@@ -0,0 +1,9 @@
+package malte0811.ferritecore.ducks;
+
+import malte0811.ferritecore.FastMap;
+
+public interface FastMapStateHolder<S> {
+    FastMap<S> getStateMap();
+
+    int getStateIndex();
+}

src/main/java/malte0811/ferritecore/ducks/NoPropertyStateHolder.java

@@ -0,0 +1,4 @@
+package malte0811.ferritecore.ducks;
+
+public interface NoPropertyStateHolder {
+}

src/main/java/malte0811/ferritecore/mixin/FastMapStateHolderMixin.java

@@ -0,0 +1,85 @@
+package malte0811.ferritecore.mixin;
+
+import com.google.common.collect.ImmutableMap;
+import malte0811.ferritecore.FastMap;
+import malte0811.ferritecore.HackyGlobalState;
+import malte0811.ferritecore.ducks.FastMapStateHolder;
+import malte0811.ferritecore.ducks.NoPropertyStateHolder;
+import net.minecraft.state.Property;
+import net.minecraft.state.StateHolder;
+import org.spongepowered.asm.mixin.*;
+
+import java.util.Collection;
+import java.util.Map;
+
+@Mixin(StateHolder.class)
+public abstract class FastMapStateHolderMixin<O, S> implements FastMapStateHolder<S> {
+    @Mutable
+    @Shadow
+    @Final
+    private ImmutableMap<Property<?>, Comparable<?>> properties;
+    @Shadow
+    @Final
+    protected O instance;
+
+    @Shadow
+    public abstract Collection<Property<?>> getProperties();
+
+    @Shadow
+    public abstract <T extends Comparable<T>> T get(Property<T> property);
+
+    private int globalTableIndex;
+    private FastMap<S> globalTable;
+
+    /**
+     * @reason This Mixin completely replaces the data structures initialized by this method, as the original ones waste
+     * a lot of memory
+     * @author malte0811
+     */
+    @Overwrite
+    public void func_235899_a_(Map<Map<Property<?>, Comparable<?>>, S> states) {
+        if (globalTable != null) {
+            throw new IllegalStateException();
+        } else if (states == HackyGlobalState.LAST_STATE_MAP.get()) {
+            // Use "hacky global state" to use the same fast map for all states of one block
+            this.globalTable = (FastMap<S>) HackyGlobalState.LAST_FAST_STATE_MAP.get();
+        } else {
+            HackyGlobalState.LAST_STATE_MAP.set(states);
+            this.globalTable = new FastMap<>(getProperties(), states);
+            HackyGlobalState.LAST_FAST_STATE_MAP.set(this.globalTable);
+        }
+        this.globalTableIndex = this.globalTable.getIndexOf(properties);
+        if (this instanceof NoPropertyStateHolder) {
+            properties = null;
+        }
+    }
+
+    /**
+     * @reason The original implementation relies on the vanilla neighbor data, which is never present with these Mixins
+     * @author malte0811
+     */
+    @Overwrite
+    public <T extends Comparable<T>, V extends T> S with(Property<T> property, V value) {
+        Comparable<?> comparable = get(property);
+        if (comparable == value) {
+            return (S) this;
+        } else {
+            S s = this.globalTable.with(this.globalTableIndex, property, value);
+            if (s == null) {
+                throw new IllegalArgumentException("Cannot set property " + property + " to " + value + " on " + this.instance + ", it is not an allowed value");
+            } else {
+                return s;
+            }
+        }
+    }
+
+    @Override
+    public FastMap<S> getStateMap() {
+        return globalTable;
+    }
+
+    @Override
+    public int getStateIndex() {
+        return globalTableIndex;
+    }
+}

src/main/java/malte0811/ferritecore/mixin/NoPropertyStateHolderMixin.java

@@ -0,0 +1,75 @@
+package malte0811.ferritecore.mixin;
+
+import com.google.common.collect.ImmutableMap;
+import malte0811.ferritecore.FastMap;
+import malte0811.ferritecore.ducks.FastMapStateHolder;
+import malte0811.ferritecore.ducks.NoPropertyStateHolder;
+import net.minecraft.state.Property;
+import net.minecraft.state.StateHolder;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Shadow;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Coerce;
+import org.spongepowered.asm.mixin.injection.Inject;
+import org.spongepowered.asm.mixin.injection.Redirect;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
+
+import java.util.Collection;
+
+@Mixin(StateHolder.class)
+public abstract class NoPropertyStateHolderMixin implements NoPropertyStateHolder {
+    @Shadow
+    public abstract ImmutableMap<Property<?>, Comparable<?>> getValues();
+
+    // All other Mixins: If the new data structures are initialized, use those. Otherwise (if populateNeighbors didn't
+    // run yet) use the vanilla code using `properties`
+    @Redirect(
+            method = {"get", "func_235903_d_"}, at = @At(
+            value = "INVOKE",
+            target = "Lcom/google/common/collect/ImmutableMap;get(Ljava/lang/Object;)Ljava/lang/Object;",
+            remap = false
+    )
+    )
+    @Coerce
+    public Object getValueOfProperty(ImmutableMap<?, ?> vanillaMap, Object key) {
+        final FastMap<?> globalTable = ((FastMapStateHolder<?>) this).getStateMap();
+        final int globalTableIndex = ((FastMapStateHolder<?>) this).getStateIndex();
+        if (globalTable != null) {
+            return globalTable.getValue(globalTableIndex, (Property<?>) key);
+        } else {
+            return vanillaMap.get(key);
+        }
+    }
+
+    // TODO speed up in some way?
+    // The cleanest (lowest-impact) approach would be to use a custom implementation of ImmutableMap (based on a FastMap
+    // and an index), but that whole class hierarchy is a very "closed" (many essential methods/classes are
+    // package-private)
+    @Inject(method = "getValues", at = @At("HEAD"), cancellable = true)
+    public void getValuesHead(CallbackInfoReturnable<ImmutableMap<Property<?>, Comparable<?>>> cir) {
+        final FastMap<?> globalTable = ((FastMapStateHolder<?>) this).getStateMap();
+        if (globalTable != null) {
+            cir.setReturnValue(globalTable.makeValuesFor(((FastMapStateHolder<?>) this).getStateIndex()));
+            cir.cancel();
+        }
+    }
+
+    @Inject(method = "hasProperty", at = @At("HEAD"), cancellable = true)
+    public <T extends Comparable<T>>
+    void hasPropertyHead(Property<T> property, CallbackInfoReturnable<Boolean> cir) {
+        final FastMap<?> globalTable = ((FastMapStateHolder<?>) this).getStateMap();
+        if (globalTable != null) {
+            cir.setReturnValue(globalTable.getProperties().contains(property));
+            cir.cancel();
+        }
+    }
+
+    @Inject(method = "getProperties", at = @At("HEAD"), cancellable = true)
+    public void getPropertiesHead(CallbackInfoReturnable<Collection<Property<?>>> cir) {
+        final FastMap<?> globalTable = ((FastMapStateHolder<?>) this).getStateMap();
+        if (globalTable != null) {
+            cir.setReturnValue(globalTable.getProperties());
+            cir.cancel();
+        }
+    }
+}

src/main/java/malte0811/ferritecore/mixin/PropertyValueConditionMixin.java

@@ -34,43 +34,50 @@ public class PropertyValueConditionMixin {
     @Final
     private static Splitter SPLITTER;
 
-   /**
-    * @reason Use cached predicates in the case of multiple specified values
-    * A less invasive Mixin would be preferable (especially since only one line really changes), but that would involve
-    * redirecting a lambda creation (not currently possible as far as I can tell) and capturing locals (possible, but
-    * annoying)
-    * @author malte0811
-    */
-   @Overwrite
-   public Predicate<BlockState> getPredicate(StateContainer<Block, BlockState> stateContainer) {
-      Property<?> property = stateContainer.getProperty(this.key);
-      if (property == null) {
-         throw new RuntimeException(String.format("Unknown property '%s' on '%s'", this.key, stateContainer.getOwner().toString()));
-      } else {
-         String valueNoInvert = this.value;
-         boolean invert = !valueNoInvert.isEmpty() && valueNoInvert.charAt(0) == '!';
-         if (invert) {
-            valueNoInvert = valueNoInvert.substring(1);
-         }
+    /**
+     * @reason Use cached predicates in the case of multiple specified values
+     * A less invasive Mixin would be preferable (especially since only one line really changes), but that would involve
+     * redirecting a lambda creation (not currently possible as far as I can tell) and capturing locals (possible, but
+     * annoying)
+     * @author malte0811
+     */
+    @Overwrite
+    public Predicate<BlockState> getPredicate(StateContainer<Block, BlockState> stateContainer) {
+        Property<?> property = stateContainer.getProperty(this.key);
+        if (property == null) {
+            throw new RuntimeException(String.format(
+                    "Unknown property '%s' on '%s'", this.key, stateContainer.getOwner().toString()
+            ));
+        } else {
+            String valueNoInvert = this.value;
+            boolean invert = !valueNoInvert.isEmpty() && valueNoInvert.charAt(0) == '!';
+            if (invert) {
+                valueNoInvert = valueNoInvert.substring(1);
+            }
 
-         List<String> matchedStates = SPLITTER.splitToList(valueNoInvert);
-         if (matchedStates.isEmpty()) {
-            throw new RuntimeException(String.format("Empty value '%s' for property '%s' on '%s'", this.value, this.key, stateContainer.getOwner().toString()));
-         } else {
-            Predicate<BlockState> isMatchedState;
-            if (matchedStates.size() == 1) {
-               isMatchedState = this.makePropertyPredicate(stateContainer, property, valueNoInvert);
+            List<String> matchedStates = SPLITTER.splitToList(valueNoInvert);
+            if (matchedStates.isEmpty()) {
+                throw new RuntimeException(String.format(
+                        "Empty value '%s' for property '%s' on '%s'",
+                        this.value, this.key, stateContainer.getOwner().toString()
+                ));
             } else {
-               List<Predicate<BlockState>> subPredicates = matchedStates.stream()
-                       .map(subValue -> this.makePropertyPredicate(stateContainer, property, subValue))
-                       .collect(Collectors.toList());
-               isMatchedState = CachedOrPredicates.or(subPredicates);
-            }
+                Predicate<BlockState> isMatchedState;
+                if (matchedStates.size() == 1) {
+                    isMatchedState = this.makePropertyPredicate(stateContainer, property, valueNoInvert);
+                } else {
+                    List<Predicate<BlockState>> subPredicates = matchedStates.stream()
+                            .map(subValue -> this.makePropertyPredicate(stateContainer, property, subValue))
+                            .collect(Collectors.toList());
+                    // This line is the only functional change, but targeting it with anything but Overwrite appears to
+                    // be impossible
+                    isMatchedState = CachedOrPredicates.or(subPredicates);
+                }
 
-            return invert ? isMatchedState.negate() : isMatchedState;
-         }
-      }
-   }
+                return invert ? isMatchedState.negate() : isMatchedState;
+            }
+        }
+    }
 
     /**
      * @reason The vanilla implementation captures an Optional in the resulting lambda, which eats a lot of memory. A
@@ -86,7 +93,10 @@ public class PropertyValueConditionMixin {
     ) {
         Optional<T> optional = property.parseValue(value);
         if (!optional.isPresent()) {
-            throw new RuntimeException(String.format("Unknown value '%s' for property '%s' on '%s' in '%s'", value, this.key, container.getOwner().toString(), this.value));
+            throw new RuntimeException(String.format(
+                    "Unknown value '%s' for property '%s' on '%s' in '%s'",
+                    value, this.key, container.getOwner().toString(), this.value
+            ));
         } else {
             T unwrapped = optional.get();
             Pair<Property<?>, Comparable<?>> key = Pair.of(property, unwrapped);

src/main/java/malte0811/ferritecore/mixin/StateholderMixin.java

@@ -1,128 +0,0 @@
-package malte0811.ferritecore.mixin;
-
-import com.google.common.collect.ImmutableMap;
-import malte0811.ferritecore.FastMap;
-import malte0811.ferritecore.HackyGlobalState;
-import net.minecraft.state.Property;
-import net.minecraft.state.StateHolder;
-import org.spongepowered.asm.mixin.*;
-import org.spongepowered.asm.mixin.injection.At;
-import org.spongepowered.asm.mixin.injection.Coerce;
-import org.spongepowered.asm.mixin.injection.Inject;
-import org.spongepowered.asm.mixin.injection.Redirect;
-import org.spongepowered.asm.mixin.injection.callback.CallbackInfoReturnable;
-
-import java.util.Collection;
-import java.util.Map;
-import java.util.Optional;
-
-@Mixin(StateHolder.class)
-public abstract class StateholderMixin<O, S> {
-    @Mutable
-    @Shadow
-    @Final
-    private ImmutableMap<Property<?>, Comparable<?>> properties;
-    @Shadow
-    @Final
-    protected O instance;
-
-    @Shadow
-    public abstract Collection<Property<?>> getProperties();
-
-    @Shadow
-    public abstract <T extends Comparable<T>> T get(Property<T> property);
-
-    private int globalTableIndex;
-    private FastMap<S> globalTable;
-
-    /**
-     * @reason This Mixin completely replaces the data structures initialized by this method, as the original ones waste
-     * a lot of memory
-     * @author malte0811
-     */
-    @Overwrite
-    public void func_235899_a_(Map<Map<Property<?>, Comparable<?>>, S> states) {
-        if (globalTable != null) {
-            throw new IllegalStateException();
-        } else if (states == HackyGlobalState.LAST_STATE_MAP.get()) {
-            // Use "hacky global state" to use the same fast map for all states of one block
-            this.globalTable = (FastMap<S>) HackyGlobalState.LAST_FAST_STATE_MAP.get();
-        } else {
-            HackyGlobalState.LAST_STATE_MAP.set(states);
-            this.globalTable = new FastMap<>(getProperties(), states);
-            HackyGlobalState.LAST_FAST_STATE_MAP.set(this.globalTable);
-        }
-        this.globalTableIndex = this.globalTable.getIndexOf(properties);
-        properties = null;
-    }
-
-    /**
-     * @reason The original implementation relies on the vanilla neighbor data, which is never present with these Mixins
-     * @author malte0811
-     */
-    @Overwrite
-    public <T extends Comparable<T>, V extends T> S with(Property<T> property, V value) {
-        Comparable<?> comparable = get(property);
-        if (comparable == value) {
-            return (S) this;
-        } else {
-            S s = this.globalTable.with(this.globalTableIndex, property, value);
-            if (s == null) {
-                throw new IllegalArgumentException("Cannot set property " + property + " to " + value + " on " + this.instance + ", it is not an allowed value");
-            } else {
-                return s;
-            }
-        }
-    }
-
-    // All other Mixins: If the new data structures are initialized, use those. Otherwise (if populateNeighbors didn't
-    // run yet) use the vanilla code using `properties`
-    @Redirect(
-            method = {"get", "func_235903_d_"},
-            at = @At(
-                    value = "INVOKE",
-                    target = "Lcom/google/common/collect/ImmutableMap;get(Ljava/lang/Object;)Ljava/lang/Object;"
-            )
-    )
-    @Coerce
-    public Object getValueOfProperty(ImmutableMap<?, ?> vanillaMap, Object key) {
-        if (globalTable != null) {
-            return globalTable.getValue(globalTableIndex, (Property<?>) key);
-        } else {
-            return vanillaMap.get(key);
-        }
-    }
-
-    // TODO speed up in some way?
-    // The cleanest (lowest-impact) approach would be to use a custom implementation of ImmutableMap (based on a FastMap
-    // and an index), but that whole class hierarchy is a very "closed" (many essential methods/classes are
-    // package-private)
-    @Inject(method = "getValues", at = @At("HEAD"), cancellable = true)
-    public void getValuesHead(CallbackInfoReturnable<ImmutableMap<Property<?>, Comparable<?>>> cir) {
-        if (globalTable != null) {
-            ImmutableMap.Builder<Property<?>, Comparable<?>> result = ImmutableMap.builder();
-            for (Property<?> p : globalTable.getProperties()) {
-                result.put(p, get(p));
-            }
-            cir.setReturnValue(result.build());
-            cir.cancel();
-        }
-    }
-
-    @Inject(method = "hasProperty", at = @At("HEAD"), cancellable = true)
-    public <T extends Comparable<T>>
-    void hasPropertyHead(Property<T> property, CallbackInfoReturnable<Boolean> cir) {
-        if (globalTable != null) {
-            cir.setReturnValue(globalTable.getProperties().contains(property));
-            cir.cancel();
-        }
-    }
-
-    @Inject(method = "getProperties", at = @At("HEAD"), cancellable = true)
-    public void getPropertiesHead(CallbackInfoReturnable<Collection<Property<?>>> cir) {
-        if (globalTable != null) {
-            cir.setReturnValue(globalTable.getProperties());
-            cir.cancel();
-        }
-    }
-}

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

@@ -0,0 +1,57 @@
+package malte0811.ferritecore.mixin.config;
+
+import com.electronwill.nightconfig.core.Config;
+import com.electronwill.nightconfig.core.ConfigSpec;
+import com.electronwill.nightconfig.core.file.CommentedFileConfig;
+import com.electronwill.nightconfig.core.io.ParsingException;
+
+import java.nio.file.Path;
+import java.nio.file.Paths;
+
+public class FerriteConfig {
+    private static final String NEIGHBOR_LOOKUP = "replaceNeighborLookup";
+    //TODO actually split
+    private static final String PROPERTY_MAP = "replacePropertyMap";
+    private static final String PREDICATES = "cacheMultipartPredicates";
+    private static final Config CONFIG;
+
+    static {
+        ConfigSpec spec = new ConfigSpec();
+        spec.define(NEIGHBOR_LOOKUP, true);
+        spec.define(PROPERTY_MAP, true);
+        spec.define(PREDICATES, true);
+        CommentedFileConfig configData = read(Paths.get("config", "ferritecore-mixin.toml"));
+        configData.setComment(NEIGHBOR_LOOKUP, "Replace the blockstate neighbor table");
+        configData.setComment(PROPERTY_MAP, "Do not store the properties of a state explicitly and read them" +
+                "from the replace neighbor table instead. Requires " + NEIGHBOR_LOOKUP + " to be enabled");
+        configData.setComment(PREDICATES, "Cache the predicate instances used in multipart models");
+        spec.correct(configData);
+        configData.save();
+        CONFIG = configData;
+    }
+
+    private static CommentedFileConfig read(Path configPath) {
+        final CommentedFileConfig configData = CommentedFileConfig.builder(configPath)
+                .sync()
+                .preserveInsertionOrder()
+                .build();
+        try {
+            configData.load();
+        } catch (ParsingException ex) {
+            throw new RuntimeException("Failed to load config " + configPath, ex);
+        }
+        return configData;
+    }
+
+    public static boolean replaceNeighborTable() {
+        return CONFIG.get(NEIGHBOR_LOOKUP);
+    }
+
+    public static boolean cachePredicates() {
+        return CONFIG.get(PREDICATES);
+    }
+
+    public static boolean noPropertyState() {
+        return CONFIG.<Boolean>get(PROPERTY_MAP) && CONFIG.<Boolean>get(NEIGHBOR_LOOKUP);
+    }
+}

src/main/java/malte0811/ferritecore/mixin/config/FerriteMixinConfig.java

@@ -0,0 +1,62 @@
+package malte0811.ferritecore.mixin.config;
+
+import com.google.common.base.Preconditions;
+import org.apache.logging.log4j.LogManager;
+import org.apache.logging.log4j.Logger;
+import org.objectweb.asm.tree.ClassNode;
+import org.spongepowered.asm.mixin.extensibility.IMixinConfigPlugin;
+import org.spongepowered.asm.mixin.extensibility.IMixinInfo;
+
+import java.util.List;
+import java.util.Set;
+
+public class FerriteMixinConfig implements IMixinConfigPlugin {
+    private static final String MIXIN_PREFIX = "malte0811.ferritecore.mixin.";
+    private static final Logger LOGGER = LogManager.getLogger("ferritecore-mixin");
+
+    @Override
+    public boolean shouldApplyMixin(String targetClassName, String mixinClassName) {
+        Preconditions.checkState(mixinClassName.startsWith(MIXIN_PREFIX), "Unexpected prefix on " + mixinClassName);
+        final String name = mixinClassName.substring(MIXIN_PREFIX.length());
+        boolean result;
+        switch (name) {
+            case "OrConditionMixin":
+            case "AndConditionMixin":
+            case "PropertyValueConditionMixin":
+                result = FerriteConfig.cachePredicates();
+                break;
+            case "FastMapStateHolderMixin":
+                result = FerriteConfig.replaceNeighborTable();
+                break;
+            case "NoPropertyStateHolderMixin":
+                result = FerriteConfig.noPropertyState();
+                break;
+            default:
+                throw new RuntimeException("Unknown mixin: " + name);
+        }
+        if (!result) {
+            LOGGER.info("Mixin {} is disabled due to config settings", mixinClassName);
+        }
+        return result;
+    }
+
+    @Override
+    public void onLoad(String mixinPackage) {
+        Preconditions.checkState(MIXIN_PREFIX.equals(mixinPackage + "."));
+    }
+
+    @Override
+    public String getRefMapperConfig() { return null; }
+
+    @Override
+    public void acceptTargets(Set<String> myTargets, Set<String> otherTargets) {}
+
+    @Override
+    public List<String> getMixins() { return null; }
+
+    @Override
+    public void preApply(String targetClassName, ClassNode targetClass, String mixinClassName, IMixinInfo mixinInfo) {}
+
+    @Override
+    public void postApply(String targetClassName, ClassNode targetClass, String mixinClassName, IMixinInfo mixinInfo) {}
+}

src/main/resources/ferritecore.mixins.json

@@ -4,15 +4,17 @@
   "compatibilityLevel": "JAVA_8",
   "refmap": "ferritecore.refmap.json",
   "mixins": [
-    "StateholderMixin"
+    "FastMapStateHolderMixin",
+    "NoPropertyStateHolderMixin"
   ],
   "client": [
-    "OrConditionMixin",
     "AndConditionMixin",
+    "OrConditionMixin",
     "PropertyValueConditionMixin"
   ],
   "injectors": {
     "defaultRequire": 1
   },
-  "minVersion": "0.8"
+  "minVersion": "0.8",
+  "plugin": "malte0811.ferritecore.mixin.config.FerriteMixinConfig"
 }