Move more code out of Mixin bodies for cleaner stack traces

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

@@ -88,6 +88,11 @@ public class FastMap<Value> {
         return result.build();
     }
 
+    public <T extends Comparable<T>>
+    Value withUnsafe(int globalTableIndex, Property<T> rowKey, Object columnKey) {
+        return with(globalTableIndex, rowKey, (T) columnKey);
+    }
+
     private static class Key<T extends Comparable<T>> {
         private final Property<T> property;
         private final List<T> values;

src/main/java/malte0811/ferritecore/HackyGlobalState.java

@@ -1,10 +0,0 @@
-package malte0811.ferritecore;
-
-import net.minecraft.state.Property;
-
-import java.util.Map;
-
-public class HackyGlobalState {
-    public static final ThreadLocal<Map<Map<Property<?>, Comparable<?>>, ?>> LAST_STATE_MAP = new ThreadLocal<>();
-    public static final ThreadLocal<FastMap<?>> LAST_FAST_STATE_MAP = new ThreadLocal<>();
-}

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

@@ -1,9 +1,19 @@
 package malte0811.ferritecore.ducks;
 
+import com.google.common.collect.ImmutableMap;
 import malte0811.ferritecore.FastMap;
+import net.minecraft.state.Property;
 
 public interface FastMapStateHolder<S> {
     FastMap<S> getStateMap();
 
+    void setStateMap(FastMap<S> newValue);
+
     int getStateIndex();
+
+    void setStateIndex(int newValue);
+
+    ImmutableMap<Property<?>, Comparable<?>> getVanillaPropertyMap();
+
+    void deleteVanillaPropertyMap();
 }

src/main/java/malte0811/ferritecore/impl/AndConditionImpl.java

@@ -0,0 +1,25 @@
+package malte0811.ferritecore.impl;
+
+import malte0811.ferritecore.util.PredicateHelper;
+import net.minecraft.block.Block;
+import net.minecraft.block.BlockState;
+import net.minecraft.client.renderer.model.multipart.ICondition;
+import net.minecraft.state.StateContainer;
+
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+
+public class AndConditionImpl {
+    private static final Map<List<Predicate<BlockState>>, Predicate<BlockState>> COMBINED_PREDICATE_CACHE = new ConcurrentHashMap<>();
+
+    public static Predicate<BlockState> getPredicate(
+            StateContainer<Block, BlockState> stateContainer, Iterable<? extends ICondition> conditions
+    ) {
+        return COMBINED_PREDICATE_CACHE.computeIfAbsent(
+                PredicateHelper.toCanonicalList(conditions, stateContainer),
+                listInt -> state -> listInt.stream().allMatch((predicate) -> predicate.test(state))
+        );
+    }
+}

src/main/java/malte0811/ferritecore/CachedOrPredicates.java → src/main/java/malte0811/ferritecore/impl/CachedOrPredicates.java

@@ -1,4 +1,4 @@
-package malte0811.ferritecore;
+package malte0811.ferritecore.impl;
 
 import net.minecraft.block.BlockState;
 

src/main/java/malte0811/ferritecore/impl/PropertyValueConditionImpl.java

@@ -0,0 +1,82 @@
+package malte0811.ferritecore.impl;
+
+import com.google.common.base.Splitter;
+import malte0811.ferritecore.util.PredicateHelper;
+import net.minecraft.block.Block;
+import net.minecraft.block.BlockState;
+import net.minecraft.state.Property;
+import net.minecraft.state.StateContainer;
+import org.apache.commons.lang3.tuple.Pair;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+
+public class PropertyValueConditionImpl {
+    private static final Map<Pair<Property<?>, Comparable<?>>, Predicate<BlockState>> STATE_HAS_PROPERTY_CACHE = new ConcurrentHashMap<>();
+
+    public static Predicate<BlockState> getPredicate(
+            StateContainer<Block, BlockState> stateContainer, String key, String value, Splitter splitter
+    ) {
+        Property<?> property = stateContainer.getProperty(key);
+        if (property == null) {
+            throw new RuntimeException(String.format(
+                    "Unknown property '%s' on '%s'", key, stateContainer.getOwner().toString()
+            ));
+        } else {
+            String valueNoInvert = 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'",
+                        value, key, stateContainer.getOwner().toString()
+                ));
+            } else {
+                Predicate<BlockState> isMatchedState;
+                if (matchedStates.size() == 1) {
+                    isMatchedState = makePropertyPredicate(stateContainer, property, valueNoInvert, key, value);
+                } else {
+                    List<Predicate<BlockState>> subPredicates = matchedStates.stream()
+                            .map(subValue -> makePropertyPredicate(stateContainer, property, subValue, key, value))
+                            .collect(Collectors.toList());
+                    // This line is the only functional change, but targeting it with anything but Overwrite appears to
+                    // be impossible
+                    isMatchedState = CachedOrPredicates.or(PredicateHelper.canonize(subPredicates));
+                }
+
+                return invert ? isMatchedState.negate() : isMatchedState;
+            }
+        }
+    }
+
+    private static <T extends Comparable<T>>
+    Predicate<BlockState> makePropertyPredicate(
+            StateContainer<Block, BlockState> container, Property<T> property, String subValue, String key, String value
+    ) {
+        Optional<T> optional = property.parseValue(subValue);
+        if (!optional.isPresent()) {
+            throw new RuntimeException(String.format(
+                    "Unknown value '%s' for property '%s' on '%s' in '%s'",
+                    subValue, key, container.getOwner().toString(), value
+            ));
+        } else {
+            T unwrapped = optional.get();
+            return STATE_HAS_PROPERTY_CACHE.computeIfAbsent(
+                    Pair.of(property, unwrapped),
+                    pair -> {
+                        Comparable<?> valueInt = pair.getRight();
+                        Property<?> propInt = pair.getLeft();
+                        return state -> state.get(propInt).equals(valueInt);
+                    }
+            );
+        }
+    }
+}

src/main/java/malte0811/ferritecore/impl/StateHolderImpl.java

@@ -0,0 +1,33 @@
+package malte0811.ferritecore.impl;
+
+import malte0811.ferritecore.FastMap;
+import malte0811.ferritecore.ducks.FastMapStateHolder;
+import malte0811.ferritecore.ducks.NoPropertyStateHolder;
+import net.minecraft.state.Property;
+
+import java.util.Map;
+
+public class StateHolderImpl {
+    public static final ThreadLocal<Map<Map<Property<?>, Comparable<?>>, ?>> LAST_STATE_MAP = new ThreadLocal<>();
+    public static final ThreadLocal<FastMap<?>> LAST_FAST_STATE_MAP = new ThreadLocal<>();
+
+    public static <S>
+    void populateNeighbors(Map<Map<Property<?>, Comparable<?>>, S> states, FastMapStateHolder<S> holder) {
+        if (holder.getStateMap() != null) {
+            throw new IllegalStateException();
+        } else if (states == LAST_STATE_MAP.get()) {
+            // Use threadlocal state to use the same fast map for all states of one block
+            holder.setStateMap((FastMap<S>) LAST_FAST_STATE_MAP.get());
+        } else {
+            LAST_STATE_MAP.set(states);
+            FastMap<S> globalTable = new FastMap<>(holder.getVanillaPropertyMap().keySet(), states);
+            holder.setStateMap(globalTable);
+            LAST_FAST_STATE_MAP.set(globalTable);
+        }
+        int index = holder.getStateMap().getIndexOf(holder.getVanillaPropertyMap());
+        holder.setStateIndex(index);
+        if (holder instanceof NoPropertyStateHolder) {
+            holder.deleteVanillaPropertyMap();
+        }
+    }
+}

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

@@ -1,15 +1,16 @@
 package malte0811.ferritecore.mixin.fastmap;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Table;
 import malte0811.ferritecore.FastMap;
-import malte0811.ferritecore.HackyGlobalState;
 import malte0811.ferritecore.ducks.FastMapStateHolder;
-import malte0811.ferritecore.ducks.NoPropertyStateHolder;
+import malte0811.ferritecore.impl.StateHolderImpl;
 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.Redirect;
 
-import java.util.Collection;
 import java.util.Map;
 
 @Mixin(StateHolder.class)
@@ -18,19 +19,21 @@ public abstract class FastMapStateHolderMixin<O, S> implements FastMapStateHolde
     @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;
 
+    @Redirect(
+            method = "with",
+            at = @At(
+                    value = "INVOKE",
+                    target = "Lcom/google/common/collect/Table;get(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object;"
+            )
+    )
+    public Object getNeighborFromFastMap(Table<?, ?, ?> ignore, Object rowKey, Object columnKey) {
+        return this.globalTable.withUnsafe(this.globalTableIndex, (Property<?>) rowKey, columnKey);
+    }
+
     /**
      * @reason This Mixin completely replaces the data structures initialized by this method, as the original ones waste
      * a lot of memory
@@ -38,39 +41,7 @@ public abstract class FastMapStateHolderMixin<O, S> implements FastMapStateHolde
      */
     @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;
-            }
-        }
+        StateHolderImpl.populateNeighbors(states, this);
     }
 
     @Override
@@ -82,4 +53,24 @@ public abstract class FastMapStateHolderMixin<O, S> implements FastMapStateHolde
     public int getStateIndex() {
         return globalTableIndex;
     }
+
+    @Override
+    public ImmutableMap<Property<?>, Comparable<?>> getVanillaPropertyMap() {
+        return properties;
+    }
+
+    @Override
+    public void deleteVanillaPropertyMap() {
+        properties = null;
+    }
+
+    @Override
+    public void setStateMap(FastMap<S> newValue) {
+        globalTable = newValue;
+    }
+
+    @Override
+    public void setStateIndex(int newValue) {
+        globalTableIndex = newValue;
+    }
 }

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

@@ -26,7 +26,7 @@ public abstract class NoPropertyStateHolderMixin implements NoPropertyStateHolde
     // 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_"},
+            method = {"get", "func_235903_d_", "with"},
             at = @At(
                     value = "INVOKE",
                     target = "Lcom/google/common/collect/ImmutableMap;get(Ljava/lang/Object;)Ljava/lang/Object;",

src/main/java/malte0811/ferritecore/mixin/predicates/AndConditionMixin.java

@@ -1,6 +1,6 @@
 package malte0811.ferritecore.mixin.predicates;
 
-import malte0811.ferritecore.util.PredicateHelper;
+import malte0811.ferritecore.impl.AndConditionImpl;
 import net.minecraft.block.Block;
 import net.minecraft.block.BlockState;
 import net.minecraft.client.renderer.model.multipart.AndCondition;
@@ -11,15 +11,10 @@ import org.spongepowered.asm.mixin.Mixin;
 import org.spongepowered.asm.mixin.Overwrite;
 import org.spongepowered.asm.mixin.Shadow;
 
-import java.util.List;
-import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Predicate;
 
 @Mixin(AndCondition.class)
 public class AndConditionMixin {
-    private static final Map<List<Predicate<BlockState>>, Predicate<BlockState>> COMBINED_PREDICATE_CACHE = new ConcurrentHashMap<>();
-
     @Shadow @Final private Iterable<? extends ICondition> conditions;
 
     /**
@@ -28,9 +23,6 @@ public class AndConditionMixin {
      */
     @Overwrite
     public Predicate<BlockState> getPredicate(StateContainer<Block, BlockState> stateContainer) {
-        return COMBINED_PREDICATE_CACHE.computeIfAbsent(
-                PredicateHelper.toCanonicalList(conditions, stateContainer),
-                listInt -> state -> listInt.stream().allMatch((predicate) -> predicate.test(state))
-        );
+        return AndConditionImpl.getPredicate(stateContainer, conditions);
     }
 }

src/main/java/malte0811/ferritecore/mixin/predicates/OrConditionMixin.java

@@ -1,6 +1,6 @@
 package malte0811.ferritecore.mixin.predicates;
 
-import malte0811.ferritecore.CachedOrPredicates;
+import malte0811.ferritecore.impl.CachedOrPredicates;
 import malte0811.ferritecore.util.PredicateHelper;
 import net.minecraft.block.Block;
 import net.minecraft.block.BlockState;

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

@@ -1,29 +1,20 @@
 package malte0811.ferritecore.mixin.predicates;
 
 import com.google.common.base.Splitter;
-import malte0811.ferritecore.CachedOrPredicates;
+import malte0811.ferritecore.impl.PropertyValueConditionImpl;
 import net.minecraft.block.Block;
 import net.minecraft.block.BlockState;
 import net.minecraft.client.renderer.model.multipart.PropertyValueCondition;
-import net.minecraft.state.Property;
 import net.minecraft.state.StateContainer;
-import org.apache.commons.lang3.tuple.Pair;
 import org.spongepowered.asm.mixin.Final;
 import org.spongepowered.asm.mixin.Mixin;
 import org.spongepowered.asm.mixin.Overwrite;
 import org.spongepowered.asm.mixin.Shadow;
 
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Predicate;
-import java.util.stream.Collectors;
 
 @Mixin(PropertyValueCondition.class)
 public class PropertyValueConditionMixin {
-    private static final Map<Pair<Property<?>, Comparable<?>>, Predicate<BlockState>> STATE_HAS_PROPERTY_CACHE = new ConcurrentHashMap<>();
-
     @Shadow
     @Final
     private String key;
@@ -43,71 +34,6 @@ public class PropertyValueConditionMixin {
      */
     @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);
-                } 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;
-            }
-        }
-    }
-
-    /**
-     * @reason The vanilla implementation captures an Optional in the resulting lambda, which eats a lot of memory. A
-     * less aggressive Mixin would require fiddling with local variables, which is more effort than it would be worth
-     * for such an obscure method.
-     * Also adds a cache to avoid creating two Predicate instances for the same lookup
-     * @author malte0811
-     */
-    @Overwrite
-    private <T extends Comparable<T>>
-    Predicate<BlockState> makePropertyPredicate(
-            StateContainer<Block, BlockState> container, Property<T> property, String value
-    ) {
-        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
-            ));
-        } else {
-            T unwrapped = optional.get();
-            Pair<Property<?>, Comparable<?>> key = Pair.of(property, unwrapped);
-            return STATE_HAS_PROPERTY_CACHE.computeIfAbsent(
-                    key,
-                    pair -> {
-                        Comparable<?> valueInt = pair.getRight();
-                        Property<?> propInt = pair.getLeft();
-                        return state -> state.get(propInt).equals(valueInt);
-                    }
-            );
-        }
+        return PropertyValueConditionImpl.getPredicate(stateContainer, key, value, SPLITTER);
     }
 }

src/main/java/malte0811/ferritecore/util/PredicateHelper.java

@@ -18,8 +18,14 @@ public class PredicateHelper {
         for (ICondition cond : conditions) {
             list.add(cond.getPredicate(stateContainer));
         }
-        list.sort(Comparator.comparingInt(Predicate::hashCode));
-        list.trimToSize();
-        return list;
+        return canonize(list);
+    }
+
+    public static <T> List<Predicate<T>> canonize(List<Predicate<T>> input) {
+        input.sort(Comparator.comparingInt(Predicate::hashCode));
+        if (input instanceof ArrayList) {
+            ((ArrayList<Predicate<T>>) input).trimToSize();
+        }
+        return input;
     }
 }