New: Do not create multiple multipart model instances for the same input

build.gradle

@@ -23,7 +23,7 @@ version = "${mod_version}"
 group = "malte0811.${modid}" // http://maven.apache.org/guides/mini/guide-naming-conventions.html
 archivesBaseName = "${modid}"
 
-def mixinConfigs = ["predicates", "fastmap", "nopropertymap", "mrl"].stream()
+def mixinConfigs = ["predicates", "fastmap", "nopropertymap", "mrl", "dedupmultipart"].stream()
         .map({s -> archivesBaseName+"."+s+".mixin.json"})
         .collect(Collectors.toList())
 

src/main/java/malte0811/ferritecore/impl/ModelResourceLocationImpl.java → src/main/java/malte0811/ferritecore/impl/Deduplicator.java

@@ -2,7 +2,10 @@ package malte0811.ferritecore.impl;
 
 import com.mojang.datafixers.util.Unit;
 import malte0811.ferritecore.ModMain;
+import net.minecraft.block.BlockState;
 import net.minecraft.client.Minecraft;
+import net.minecraft.client.renderer.model.IBakedModel;
+import net.minecraft.client.renderer.model.MultipartBakedModel;
 import net.minecraft.client.resources.ReloadListener;
 import net.minecraft.profiler.IProfiler;
 import net.minecraft.resources.IReloadableResourceManager;
@@ -11,29 +14,36 @@ import net.minecraftforge.api.distmarker.Dist;
 import net.minecraftforge.client.event.ParticleFactoryRegisterEvent;
 import net.minecraftforge.eventbus.api.SubscribeEvent;
 import net.minecraftforge.fml.common.Mod;
+import org.apache.commons.lang3.tuple.Pair;
 
 import javax.annotation.Nonnull;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.function.Function;
+import java.util.function.Predicate;
 
 @Mod.EventBusSubscriber(value = Dist.CLIENT, modid = ModMain.MODID, bus = Mod.EventBusSubscriber.Bus.MOD)
-public class ModelResourceLocationImpl {
+public class Deduplicator {
     private static final Map<String, String> VARIANT_IDENTITIES = new ConcurrentHashMap<>();
+    // Typedefs would be a nice thing to have
+    private static final Map<List<Pair<Predicate<BlockState>, IBakedModel>>, MultipartBakedModel> KNOWN_MULTIPART_MODELS = new ConcurrentHashMap<>();
 
     public static String deduplicateVariant(String variant) {
         return VARIANT_IDENTITIES.computeIfAbsent(variant, Function.identity());
     }
 
+    public static MultipartBakedModel makeMultipartModel(List<Pair<Predicate<BlockState>, IBakedModel>> selectors) {
+        return KNOWN_MULTIPART_MODELS.computeIfAbsent(selectors, MultipartBakedModel::new);
+    }
+
     @SubscribeEvent
     public static void registerReloadListener(ParticleFactoryRegisterEvent ev) {
         // Register the reload listener s.t. its "sync" part runs after the model loader reload
         ((IReloadableResourceManager) Minecraft.getInstance().getResourceManager()).addReloadListener(new ReloadListener<Unit>() {
             @Nonnull
             @Override
-            protected Unit prepare(
-                    @Nonnull IResourceManager resourceManagerIn, @Nonnull IProfiler profilerIn
-            ) {
+            protected Unit prepare(@Nonnull IResourceManager resourceManagerIn, @Nonnull IProfiler profilerIn) {
                 return Unit.INSTANCE;
             }
 
@@ -42,6 +52,7 @@ public class ModelResourceLocationImpl {
                     @Nonnull Unit objectIn, @Nonnull IResourceManager resourceManagerIn, @Nonnull IProfiler profilerIn
             ) {
                 VARIANT_IDENTITIES.clear();
+                KNOWN_MULTIPART_MODELS.clear();
             }
         });
     }

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

@@ -8,14 +8,17 @@ import javax.annotation.Nullable;
 import java.nio.file.Path;
 import java.nio.file.Paths;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.List;
 import java.util.Objects;
+import java.util.function.Predicate;
 
 public class FerriteConfig {
     public static final Option NEIGHBOR_LOOKUP;
     public static final Option PROPERTY_MAP;
     public static final Option PREDICATES;
     public static final Option MRL_CACHE;
+    public static final Option DEDUP_MULTIPART;
 
     static {
         ConfigBuilder builder = new ConfigBuilder();
@@ -23,7 +26,8 @@ public class FerriteConfig {
         PROPERTY_MAP = builder.createOption(
                 "replacePropertyMap",
                 "Do not store the properties of a state explicitly and read them" +
-                        "from the replace neighbor table instead. Requires " + NEIGHBOR_LOOKUP.getName() + " to be enabled"
+                        "from the replace neighbor table instead. Requires " + NEIGHBOR_LOOKUP.getName() + " to be enabled",
+                NEIGHBOR_LOOKUP
         );
         PREDICATES = builder.createOption(
                 "cacheMultipartPredicates",
@@ -33,13 +37,13 @@ public class FerriteConfig {
                 "modelResourceLocations",
                 "Avoid creation of new strings when creating ModelResourceLocations"
         );
+        DEDUP_MULTIPART = builder.createOption(
+                "multipartDeduplication",
+                "Do not create a new MultipartBakedModel instance for each block state using the same multipart" +
+                        "model. Requires " + PREDICATES.getName() + " to be enabled",
+                PREDICATES
+        );
         builder.finish();
-        if (PROPERTY_MAP.isEnabled() && !NEIGHBOR_LOOKUP.isEnabled()) {
-            throw new IllegalStateException(
-                    PROPERTY_MAP.getName() + " is enabled in the FerriteCore config, but " +
-                            NEIGHBOR_LOOKUP.getName() + " is not. This is not supported!"
-            );
-        }
     }
 
     private static CommentedFileConfig read(Path configPath) {
@@ -58,8 +62,8 @@ public class FerriteConfig {
     public static class ConfigBuilder {
         private final List<Option> options = new ArrayList<>();
 
-        public Option createOption(String name, String comment) {
-            Option result = new Option(name, comment);
+        public Option createOption(String name, String comment, Option... dependencies) {
+            Option result = new Option(name, comment, dependencies);
             options.add(result);
             return result;
         }
@@ -76,7 +80,7 @@ public class FerriteConfig {
             spec.correct(configData);
             configData.save();
             for (Option o : options) {
-                o.value = configData.get(o.getName());
+                o.set(configData::get);
             }
         }
     }
@@ -84,12 +88,29 @@ public class FerriteConfig {
     public static class Option {
         private final String name;
         private final String comment;
+        private final List<Option> dependencies;
         @Nullable
         private Boolean value;
 
-        public Option(String name, String comment) {
+        public Option(String name, String comment, Option... dependencies) {
             this.name = name;
             this.comment = comment;
+            this.dependencies = Arrays.asList(dependencies);
+        }
+
+        void set(Predicate<String> isEnabled) {
+            final boolean enabled = isEnabled.test(getName());
+            if (enabled) {
+                for (Option dep : dependencies) {
+                    if (!isEnabled.test(dep.getName())) {
+                        throw new IllegalStateException(
+                                getName() + " is enabled in the FerriteCore config, but " + dep.getName()
+                                        + " is not. This is not supported!"
+                        );
+                    }
+                }
+            }
+            this.value = enabled;
         }
 
         public String getName() {

src/main/java/malte0811/ferritecore/mixin/dedupmultipart/Config.java

@@ -0,0 +1,19 @@
+package malte0811.ferritecore.mixin.dedupmultipart;
+
+import com.google.common.collect.ImmutableList;
+import malte0811.ferritecore.mixin.config.FerriteConfig;
+import malte0811.ferritecore.mixin.config.FerriteMixinConfig;
+
+import java.util.List;
+
+public class Config extends FerriteMixinConfig {
+    @Override
+    protected List<String> getAllMixins() {
+        return ImmutableList.of("MixinMultipartBuilder");
+    }
+
+    @Override
+    protected boolean isEnabled(String mixin) {
+        return FerriteConfig.DEDUP_MULTIPART.isEnabled();
+    }
+}

src/main/java/malte0811/ferritecore/mixin/dedupmultipart/MixinMultipartBuilder.java

@@ -0,0 +1,30 @@
+package malte0811.ferritecore.mixin.dedupmultipart;
+
+import malte0811.ferritecore.impl.Deduplicator;
+import net.minecraft.block.BlockState;
+import net.minecraft.client.renderer.model.IBakedModel;
+import net.minecraft.client.renderer.model.MultipartBakedModel;
+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.function.Predicate;
+
+@Mixin(MultipartBakedModel.Builder.class)
+public class MixinMultipartBuilder {
+    @Shadow
+    @Final
+    private List<Pair<Predicate<BlockState>, IBakedModel>> selectors;
+
+    /**
+     * @reason Cache/deduplicate returned model
+     * @author malte0811
+     */
+    @Overwrite
+    public IBakedModel build() {
+        return Deduplicator.makeMultipartModel(selectors);
+    }
+}

src/main/java/malte0811/ferritecore/mixin/mrl/ModelResourceLocationMixin.java

@@ -1,6 +1,6 @@
 package malte0811.ferritecore.mixin.mrl;
 
-import malte0811.ferritecore.impl.ModelResourceLocationImpl;
+import malte0811.ferritecore.impl.Deduplicator;
 import net.minecraft.client.renderer.model.ModelResourceLocation;
 import net.minecraft.util.ResourceLocation;
 import org.spongepowered.asm.mixin.Final;
@@ -26,6 +26,6 @@ public class ModelResourceLocationMixin {
         // Do not use new strings for path and namespace, and deduplicate the variant string
         ((ResourceLocationAccess) this).setPath(location.getPath());
         ((ResourceLocationAccess) this).setNamespace(location.getNamespace());
-        this.variant = ModelResourceLocationImpl.deduplicateVariant(variantIn);
+        this.variant = Deduplicator.deduplicateVariant(variantIn);
     }
 }

src/main/resources/ferritecore.dedupmultipart.mixin.json

@@ -0,0 +1,16 @@
+{
+  "required": true,
+  "package": "malte0811.ferritecore.mixin.dedupmultipart",
+  "compatibilityLevel": "JAVA_8",
+  "refmap": "ferritecore.refmap.json",
+  "injectors": {
+    "defaultRequire": 1
+  },
+  "minVersion": "0.8",
+  "plugin": "malte0811.ferritecore.mixin.dedupmultipart.Config",
+  "client": [
+    "MixinMultipartBuilder"
+  ],
+  "mixins": [
+  ]
+}

summary.md

@@ -11,11 +11,11 @@ The vanilla implementation contains code along these lines:
 
 ```java
 Optional<T> opt=newlyCreatedOptional();
-        if(!opt.isPresent()){
-        // Something
-        }else{
-        return()->doThing(opt.get());
-        }
+if(!opt.isPresent()){
+    // Something
+}else{
+    return()->doThing(opt.get());
+}
 ```
 
 The created lambda is kept around for a long time, and there are a few million of them. In
@@ -99,3 +99,15 @@ Side: client
 Mixin subpackage: `mrl`  
 Note: The CPU impact of the current Mixin implementation is positive for both parts, because a negative impact for the
 first part would require changing what constructor the constructor in question redirects to.
+
+### 6. Multipart model instances
+By default every blockstate using a multipart model gets its own instance of that multipart model. Since multipart
+models are generally used for blocks with a lot of states this means a lot of instances, and a lot of wasted memory.
+The only input data for a multipart model is a `List<Pair<Predicate<BlockState>, IBakedModel>>`. The predicate is
+already deduplicated by point 4, so it is very easy to use the same instance for equivalent lists. This reduces the
+number of instance from about 200k to 1.5k (DW20 1.2.0).
+
+Saved memory: Close to 200 MB  
+CPU impact: Slight during loading, zero at runtime  
+Side: client  
+Mixin subpackage: `dedupmultipart`