New: Do not create new String instances when constructing MRLs

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"].stream()
+def mixinConfigs = ["predicates", "fastmap", "nopropertymap", "mrl"].stream()
         .map({s -> archivesBaseName+"."+s+".mixin.json"})
         .collect(Collectors.toList())
 

gradle.properties

@@ -2,7 +2,7 @@
 # This is required to provide enough memory for the Minecraft decompilation process.
 org.gradle.jvmargs=-Xmx3G
 org.gradle.daemon=false
-mod_version=1.0
+mod_version=1.1
 modid=ferritecore
 mc_version=1.16.4
 forge_version=35.1.13

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

@@ -0,0 +1,48 @@
+package malte0811.ferritecore.impl;
+
+import com.mojang.datafixers.util.Unit;
+import malte0811.ferritecore.ModMain;
+import net.minecraft.client.Minecraft;
+import net.minecraft.client.resources.ReloadListener;
+import net.minecraft.profiler.IProfiler;
+import net.minecraft.resources.IReloadableResourceManager;
+import net.minecraft.resources.IResourceManager;
+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 javax.annotation.Nonnull;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Function;
+
+@Mod.EventBusSubscriber(value = Dist.CLIENT, modid = ModMain.MODID, bus = Mod.EventBusSubscriber.Bus.MOD)
+public class ModelResourceLocationImpl {
+    private static final Map<String, String> VARIANT_IDENTITIES = new ConcurrentHashMap<>();
+
+    public static String deduplicateVariant(String variant) {
+        return VARIANT_IDENTITIES.computeIfAbsent(variant, Function.identity());
+    }
+
+    @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
+            ) {
+                return Unit.INSTANCE;
+            }
+
+            @Override
+            protected void apply(
+                    @Nonnull Unit objectIn, @Nonnull IResourceManager resourceManagerIn, @Nonnull IProfiler profilerIn
+            ) {
+                VARIANT_IDENTITIES.clear();
+            }
+        });
+    }
+}

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

@@ -10,9 +10,9 @@ 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 String MRL_CACHE = "modelResourceLocations";
     private static final Config CONFIG;
 
     static {
@@ -20,11 +20,13 @@ public class FerriteConfig {
         spec.define(NEIGHBOR_LOOKUP, true);
         spec.define(PROPERTY_MAP, true);
         spec.define(PREDICATES, true);
+        spec.define(MRL_CACHE, 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");
+        configData.setComment(MRL_CACHE, "Avoid creation of new strings when creating ModelResourceLocations");
         spec.correct(configData);
         configData.save();
         CONFIG = configData;
@@ -54,4 +56,8 @@ public class FerriteConfig {
     public static boolean noPropertyState() {
         return CONFIG.<Boolean>get(PROPERTY_MAP) && CONFIG.<Boolean>get(NEIGHBOR_LOOKUP);
     }
+
+    public static boolean optimizeMRL() {
+        return CONFIG.get(MRL_CACHE);
+    }
 }

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

@@ -0,0 +1,19 @@
+package malte0811.ferritecore.mixin.mrl;
+
+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("ResourceLocationAccess", "ModelResourceLocationMixin");
+    }
+
+    @Override
+    protected boolean isEnabled(String mixin) {
+        return FerriteConfig.optimizeMRL();
+    }
+}

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

@@ -0,0 +1,31 @@
+package malte0811.ferritecore.mixin.mrl;
+
+import malte0811.ferritecore.impl.ModelResourceLocationImpl;
+import net.minecraft.client.renderer.model.ModelResourceLocation;
+import net.minecraft.util.ResourceLocation;
+import org.spongepowered.asm.mixin.Final;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Mutable;
+import org.spongepowered.asm.mixin.Shadow;
+import org.spongepowered.asm.mixin.injection.At;
+import org.spongepowered.asm.mixin.injection.Inject;
+import org.spongepowered.asm.mixin.injection.callback.CallbackInfo;
+
+@Mixin(ModelResourceLocation.class)
+public class ModelResourceLocationMixin {
+    @Shadow
+    @Final
+    @Mutable
+    private String variant;
+
+    @Inject(
+            method = "<init>(Lnet/minecraft/util/ResourceLocation;Ljava/lang/String;)V",
+            at = @At("TAIL")
+    )
+    private void constructTail(ResourceLocation location, String variantIn, CallbackInfo ci) {
+        // 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);
+    }
+}

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

@@ -0,0 +1,17 @@
+package malte0811.ferritecore.mixin.mrl;
+
+import net.minecraft.util.ResourceLocation;
+import org.spongepowered.asm.mixin.Mixin;
+import org.spongepowered.asm.mixin.Mutable;
+import org.spongepowered.asm.mixin.gen.Accessor;
+
+@Mixin(ResourceLocation.class)
+public interface ResourceLocationAccess {
+    @Accessor
+    @Mutable
+    void setNamespace(String newNamespace);
+
+    @Accessor
+    @Mutable
+    void setPath(String newPath);
+}

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

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

summary.md

@@ -1,18 +1,21 @@
-This file tries to explain the changes done in FerriteCore, and how much memory they save.
-The saved memory refers to a ForgeCraft 1 instance around 19th December 2020.
+This file tries to explain the changes done in FerriteCore, and how much memory they save. The saved memory for the
+first 4 points refers to a ForgeCraft 1 instance around 19th December 2020, after that to version 1.2.0 of the (smaller)
+1.16.4 Direwolf20 pack. This is mostly because FC 1 uses [ServerPackLocator](https://github.com/cpw/serverpacklocator/),
+which makes reproducible tests near-impossible (and means that I can't test when the server is down :smile:)
 
-### Optionals in `PropertyValueCondition`
-This change is irrelevant with the last point, it is only included in this list for
-completeness.
+### 1. Optionals in `PropertyValueCondition`
+
+This change is made obsolete by the 4th point, it is only included in this list for completeness.
 
 The vanilla implementation contains code along these lines:
+
 ```java
-Optional<T> opt = newlyCreatedOptional();
-if (!opt.isPresent()) {
-    // Something
-} else {
-    return () -> doThing(opt.get());
-}
+Optional<T> opt=newlyCreatedOptional();
+        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
@@ -28,7 +31,7 @@ Saved memory: 100 MB
 CPU impact: zero or negative (one less pointer to follow)  
 Side: client  
 
-### BlockState neighbors
+### 2. BlockState neighbors
 To implement `StateHolder#with` (mostly seen as `BlockState#with`) a state needs to be
 able to quickly find its "neighbor states". In vanilla this is implemented using a
 `Table<Property<?>, Comparable<?>, S>` for each state. In total these tables use about 600
@@ -44,7 +47,7 @@ CPU impact: hard to prove, but most likely near zero
 Side: both  
 Mixin subpackage: `fastmap`  
 
-### BlockState property storage
+### 3. BlockState property storage
 Each blockstate stores its properties as an `ImmutableMap<Property<?>, Comparable<?>>`,
 which takes around 170 MB in total. Most operations do not actually require this map, they
 can be implemented with similar speed using the `FastMap` from the previous point.  There
@@ -61,7 +64,7 @@ CPU impact: unclear (see second paragraph)
 Side: both  
 Mixin subpackage: `nopropertymap`  
 
-### Multipart model predicate caching
+### 4. Multipart model predicate caching
 Each multipart model stores a number of predicates to determine which parts to show under
 what conditions. These predicates take up 300-400 MB. However in many cases these
 predicates are checking the same thing, they are just newly created every time. For
@@ -71,14 +74,28 @@ list of input predicates sorted by hash value.
 One detail that makes this even more effective is that a block can never have two
 properties that are equal according to `equals`, while the common property implementations
 include `equals`. Additionally `StateHolder#get` also considers `equals` (as opposed to
-reference equality), so using the same lambda for equivalent (but non
-reference-equivalent) properties and values is actually possible. This is particularly
-useful as one of the most common usages of multipart models is pipes, where the states are
-nearly always boolean properties named `north` etc. As a result the number of predicates
-is reduced from between 10s of thousands and millions to a few ten or hundred instances.
+reference equality), so using the same lambda for equivalent (but non reference-equivalent) properties and values is
+actually possible. This is particularly useful as one of the most common usages of multipart models is pipes, where the
+states are nearly always boolean properties named `north` etc. As a result the number of predicates is reduced from
+between 10s of thousands and millions to a few ten or hundred instances.
 
-Saved memory: 300-400 MB (relative to the state after the first change, so 100 MB more
-compared to a "clean" instance)  
+Saved memory: 300-400 MB (relative to the state after the first change, so 100 MB more compared to a "clean" instance)  
 CPU impact: Some impact in model loading (but less allocations), zero while playing  
 Side: client  
-Mixin subpackage: `predicates`  
+Mixin subpackage: `predicates`
+
+### 5. String instance reduction in `ModelResourceLocation`
+
+The `ModelResourceLocation` constructor accepting a `ResourceLocation` and a `BlockState`
+(the main one used in practice) is implemented by first converting the RL to a string and then splitting it again. This
+is not only a waste of CPU time, it also means that new
+`String` instances are created for the path and namespace of the MRL.  
+Another optimization is to deduplicate the `variant` string of the MRL, i.e. to use the same `String` instance for all
+MRLs with a given `variant`.
+
+Saved memory: about 300 MB (DW20 pack version 1.2.0)  
+CPU impact: Zero or negative for the first part, slight (<1s) during loading for the second part  
+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.