From f77221d6d8e6da6f71ee01d0fec005df6becf97d Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Mon, 19 Jun 2023 20:58:51 -0400 Subject: [PATCH 1/5] Fix missing newline [skip ci] --- scripts/propagate.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/scripts/propagate.sh b/scripts/propagate.sh index 23b8a6a0..86bc3968 100755 --- a/scripts/propagate.sh +++ b/scripts/propagate.sh @@ -47,6 +47,7 @@ for version in "${all_versions[@]}"; do read -rs -n1 fi if (git add . && git commit -m "Merge $our_version into $version" &>/dev/null); then + echo git push -u origin propagations/$version:$version &>/dev/null else echo -e "\b\b\b\bnothing to merge" From 1bf64f9aa1994c9be4c9f93620b59ab2c223a7be Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Mon, 19 Jun 2023 21:07:13 -0400 Subject: [PATCH 2/5] Use annotated tags for releases [skip ci] --- scripts/autorelease.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/autorelease.sh b/scripts/autorelease.sh index bf338308..508143b5 100755 --- a/scripts/autorelease.sh +++ b/scripts/autorelease.sh @@ -27,6 +27,8 @@ do_release() { echo "we think the current tag is $(git describe --tags --abbrev=0)" echo "the current commit head is $(git rev-parse HEAD)" read -p "new tag name: " tag_name + git tag -a $tag_name -m "$tag_name" + git push --tags gh release create $tag_name --target $1 --title "$tag_name" --notes "" } From 41b71c5e59d4539e4421ff3e43150bb7efde06c6 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Mon, 19 Jun 2023 21:49:45 -0400 Subject: [PATCH 3/5] Attempt fix for Engineer's Decor and related crashes --- .../modernfix/forge/config/ConfigFixer.java | 53 +++++++++++++++++++ .../modernfix/forge/init/ModernFixForge.java | 2 + 2 files changed, 55 insertions(+) create mode 100644 forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java b/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java new file mode 100644 index 00000000..48150a34 --- /dev/null +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java @@ -0,0 +1,53 @@ +package org.embeddedt.modernfix.forge.config; + +import net.minecraftforge.fml.ModContainer; +import net.minecraftforge.fml.ModList; +import net.minecraftforge.fml.common.ObfuscationReflectionHelper; +import net.minecraftforge.fml.config.ModConfig; +import org.embeddedt.modernfix.ModernFix; + +import java.util.Optional; +import java.util.function.Consumer; + +public class ConfigFixer { + /** + * To prevent mods from crashing if their ModConfigEvent is invoked by Night Config's watch thread and Forge + * at the same time, wrap their config handler so that it only executes the event in serial for that mod. + * + * Should have no noticeable performance impact as config handlers are virtually instant. + */ + public static void replaceConfigHandlers() { + ModList.get().forEachModContainer((id, container) -> { + try { + Optional> configOpt = ObfuscationReflectionHelper.getPrivateValue(ModContainer.class, container, "configHandler"); + if(configOpt.isPresent()) { + ObfuscationReflectionHelper.setPrivateValue(ModContainer.class, container, Optional.of(new LockingConfigHandler(id, configOpt.get())), "configHandler"); + } + } catch(RuntimeException e) { + ModernFix.LOGGER.error("Error replacing config handler", e); + } + }); + } + + private static class LockingConfigHandler implements Consumer { + private final Consumer actualHandler; + private final String modId; + + LockingConfigHandler(String id, Consumer actualHandler) { + this.modId = id; + this.actualHandler = actualHandler; + } + + @Override + public void accept(ModConfig.ModConfigEvent modConfigEvent) { + synchronized (this) { + this.actualHandler.accept(modConfigEvent); + } + } + + @Override + public String toString() { + return "LockingConfigHandler{id=" + modId + "}"; + } + } +} diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/init/ModernFixForge.java b/forge/src/main/java/org/embeddedt/modernfix/forge/init/ModernFixForge.java index 570f5242..bb360b2c 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/init/ModernFixForge.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/init/ModernFixForge.java @@ -25,6 +25,7 @@ import org.embeddedt.modernfix.forge.classloading.ClassLoadHack; import org.embeddedt.modernfix.forge.classloading.ModFileScanDataDeduplicator; import org.embeddedt.modernfix.forge.ModernFixConfig; import org.embeddedt.modernfix.entity.EntityDataIDSyncHandler; +import org.embeddedt.modernfix.forge.config.ConfigFixer; import org.embeddedt.modernfix.forge.packet.PacketHandler; import org.embeddedt.modernfix.forge.registry.ObjectHolderClearer; import org.embeddedt.modernfix.forge.util.KubeUtil; @@ -48,6 +49,7 @@ public class ModernFixForge { PacketHandler.register(); ModFileScanDataDeduplicator.deduplicate(); ClassLoadHack.loadModClasses(); + ConfigFixer.replaceConfigHandlers(); } @SubscribeEvent From c1acdf1bb4dcd44412c9389b4f6e03c4979115c5 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 20 Jun 2023 09:42:44 -0400 Subject: [PATCH 4/5] Make config wrapping possible to disable --- .../embeddedt/modernfix/core/config/ModernFixEarlyConfig.java | 3 +++ .../java/org/embeddedt/modernfix/forge/config/ConfigFixer.java | 3 +++ 2 files changed, 6 insertions(+) diff --git a/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java b/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java index a9540db6..5c3b5cf0 100644 --- a/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java +++ b/common/src/main/java/org/embeddedt/modernfix/core/config/ModernFixEarlyConfig.java @@ -163,6 +163,9 @@ public class ModernFixEarlyConfig { this.scanForAndBuildMixinOptions(); mixinOptions.addAll(DEFAULT_SETTING_OVERRIDES.keySet()); + if(!isFabric) { + mixinOptions.add("mixin.bugfix.fix_config_crashes"); + } for(String optionName : mixinOptions) { boolean defaultEnabled = DEFAULT_SETTING_OVERRIDES.getOrDefault(optionName, true); this.options.putIfAbsent(optionName, new Option(optionName, defaultEnabled, false)); diff --git a/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java b/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java index 48150a34..fb7e0f71 100644 --- a/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java +++ b/forge/src/main/java/org/embeddedt/modernfix/forge/config/ConfigFixer.java @@ -5,6 +5,7 @@ import net.minecraftforge.fml.ModList; import net.minecraftforge.fml.common.ObfuscationReflectionHelper; import net.minecraftforge.fml.config.ModConfig; import org.embeddedt.modernfix.ModernFix; +import org.embeddedt.modernfix.core.ModernFixMixinPlugin; import java.util.Optional; import java.util.function.Consumer; @@ -17,6 +18,8 @@ public class ConfigFixer { * Should have no noticeable performance impact as config handlers are virtually instant. */ public static void replaceConfigHandlers() { + if(!ModernFixMixinPlugin.instance.isOptionEnabled("bugfix.fix_config_crashes.ConfigFixerMixin")) + return; ModList.get().forEachModContainer((id, container) -> { try { Optional> configOpt = ObfuscationReflectionHelper.getPrivateValue(ModContainer.class, container, "configHandler"); From 5d6566512c9b434070bbe231ac6c7d09c68569b0 Mon Sep 17 00:00:00 2001 From: embeddedt <42941056+embeddedt@users.noreply.github.com> Date: Tue, 20 Jun 2023 10:31:49 -0400 Subject: [PATCH 5/5] Rewrite CanonizingStringMap to simply use a normal HashMap and intern keys There are no memory savings from using the fastutil maps, and they may be harming performance based on the Project MMO issues Probably also the solution to #134 --- .../modernfix/util/CanonizingStringMap.java | 157 +++--------------- 1 file changed, 23 insertions(+), 134 deletions(-) diff --git a/common/src/main/java/org/embeddedt/modernfix/util/CanonizingStringMap.java b/common/src/main/java/org/embeddedt/modernfix/util/CanonizingStringMap.java index 1202500b..352492b1 100644 --- a/common/src/main/java/org/embeddedt/modernfix/util/CanonizingStringMap.java +++ b/common/src/main/java/org/embeddedt/modernfix/util/CanonizingStringMap.java @@ -1,157 +1,46 @@ package org.embeddedt.modernfix.util; +import com.google.common.base.Function; import com.google.common.collect.Interner; import com.google.common.collect.Interners; -import it.unimi.dsi.fastutil.objects.*; -import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; +import com.google.common.collect.Maps; -import java.util.*; -import java.util.function.Function; +import java.util.HashMap; +import java.util.Map; /** - * Replacement backing map for CompoundTags. Uses an array map for tags with 4 or less entries, - * and a hash map for larger tags. + * Replacement backing map for CompoundTags that interns keys. */ -public class CanonizingStringMap implements Map { - private Object2ObjectMap backingMap; - - private static final int GROWTH_THRESHOLD = 4; +public class CanonizingStringMap extends HashMap { private static final Interner KEY_INTERNER = Interners.newStrongInterner(); + private static String intern(String key) { + return key != null ? KEY_INTERNER.intern(key) : null; + } + public CanonizingStringMap() { - this(new Object2ObjectArrayMap<>()); - } - - protected CanonizingStringMap(Object2ObjectMap newMap) { - this.backingMap = newMap; + super(); } @Override - public int size() { - return backingMap.size(); + public T put(String key, T value) { + return super.put(intern(key), value); } @Override - public boolean isEmpty() { - return backingMap.isEmpty(); + public void putAll(Map m) { + HashMap tmp = new HashMap<>(); + m.forEach((k, v) -> tmp.put(intern(k), v)); + super.putAll(tmp); } - @Override - public boolean containsKey(Object o) { - return backingMap.containsKey(o); + private void putAllWithoutInterning(Map m) { + super.putAll(m); } - @Override - public boolean containsValue(Object o) { - return backingMap.containsValue(o); - } - - @Override - public T get(Object o) { - return backingMap.get(o); - } - - @Nullable - @Override - public T put(String s, T t) { - if(backingMap.size() >= GROWTH_THRESHOLD && !(backingMap instanceof Object2ObjectOpenHashMap) && !backingMap.containsKey(s)) { - // map will grow to GROWTH_THRESHOLD + 1 entries, change to hashmap - backingMap = new Object2ObjectOpenHashMap<>(backingMap); - } - s = KEY_INTERNER.intern(s); - return backingMap.put(s, t); - } - - @Override - public T remove(Object o) { - T value = backingMap.remove(o); - // need to shrink to be consistent with new maps - if(backingMap.size() <= GROWTH_THRESHOLD && backingMap instanceof Object2ObjectOpenHashMap) { - backingMap = new Object2ObjectArrayMap<>(backingMap); - } - return value; - } - - @Override - public void putAll(@NotNull Map map) { - if(map.size() == 0) - return; - // grow early if we know there are enough non-overlapping keys - if((map.size() - backingMap.size()) > GROWTH_THRESHOLD && !(backingMap instanceof Object2ObjectOpenHashMap)) { - backingMap = new Object2ObjectOpenHashMap<>(backingMap); - } - map.forEach((String key, T val) -> { - key = KEY_INTERNER.intern(key); - backingMap.put(key, val); - }); - // if it's still an array, and now too big, grow it - if(backingMap.size() > GROWTH_THRESHOLD && !(backingMap instanceof Object2ObjectOpenHashMap)) { - backingMap = new Object2ObjectOpenHashMap<>(backingMap); - } - } - - @Override - public void clear() { - if(!(this.backingMap instanceof Object2ObjectArrayMap)) - this.backingMap = new Object2ObjectArrayMap<>(); - else - this.backingMap.clear(); - } - - @NotNull - @Override - public Set keySet() { - // has to be modifiable because mods (cough, Tinkers) use it to clear the tag - return this.backingMap.keySet(); - } - - @NotNull - @Override - public Collection values() { - return Collections.unmodifiableCollection(this.backingMap.values()); - } - - @NotNull - @Override - public Set> entrySet() { - return Collections.unmodifiableSet(this.backingMap.entrySet()); - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - CanonizingStringMap that = (CanonizingStringMap)o; - if(that.backingMap.size() != backingMap.size()) - return false; - return backingMap.object2ObjectEntrySet().containsAll(that.backingMap.object2ObjectEntrySet()); - } - - /** - * We deliberately use a hashcode that will be consistent regardless of underlying map type. - */ - @Override - public int hashCode() { - final ObjectIterator> i = Object2ObjectMaps.fastIterator(backingMap); - int h = 0, n = backingMap.size(); - while (n-- != 0) - h += i.next().hashCode(); - return h; - } - - public static CanonizingStringMap deepCopy(CanonizingStringMap inputMap, Function deepCopier) { - Objects.requireNonNull(deepCopier); - Object2ObjectMap copiedBackingMap; - int size = inputMap.backingMap.size(); - if(size > GROWTH_THRESHOLD) { - copiedBackingMap = new Object2ObjectOpenHashMap<>(size); - } else - copiedBackingMap = new Object2ObjectArrayMap<>(size); - inputMap.backingMap.object2ObjectEntrySet().forEach(entry -> { - if(entry.getKey() != null && entry.getValue() != null) - copiedBackingMap.put(entry.getKey(), deepCopier.apply(entry.getValue())); - }); - return new CanonizingStringMap<>(copiedBackingMap); + public static CanonizingStringMap deepCopy(CanonizingStringMap incomingMap, Function deepCopier) { + CanonizingStringMap newMap = new CanonizingStringMap<>(); + newMap.putAllWithoutInterning(Maps.transformValues(incomingMap, deepCopier)); + return newMap; } }