Skip to content

SpeedupOreDictionaryTransformer#522

Merged
mitchej123 merged 18 commits intomasterfrom
speedup-oredictionary
May 24, 2025
Merged

SpeedupOreDictionaryTransformer#522
mitchej123 merged 18 commits intomasterfrom
speedup-oredictionary

Conversation

@mitchej123
Copy link
Copy Markdown
Contributor

@mitchej123 mitchej123 commented May 1, 2025

Gets rid of all Integer boxing/unboxing, and swaps based collections with the fastutils primative equivalent.

Local profile showing Integer.valueOf() top offenders w/ 32 chunk render distance + DH

image

@mitchej123 mitchej123 requested review from a team, ah-OOG-ah and eigenraven May 1, 2025 23:20
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 1, 2025

#523

@mitchej123
Copy link
Copy Markdown
Contributor Author

Probably needs some compat with Thermos - https://github.com/GTNewHorizons/Thermos/blob/1.7.10/patches/net/minecraftforge/oredict/OreDictionary.java.patch

Crucible oddly enough looks fine

@mitchej123
Copy link
Copy Markdown
Contributor Author

runServer seems to not be very happy.

Other than that, the only other transformers that run on this are NEI and some minor thermos patches. I'm 100% fine with it not being compatible with mixins 😎 and/or only enabling for NH.

@mitchej123
Copy link
Copy Markdown
Contributor Author

Considering using an overwrite equivalent for getOreIDs so it would work on thermos as well.

@mitchej123 mitchej123 force-pushed the speedup-oredictionary branch from 11ef238 to 94b4334 Compare May 2, 2025 22:07
@mitchej123 mitchej123 requested a review from Caedis May 2, 2025 22:41
@mitchej123
Copy link
Copy Markdown
Contributor Author

Assuming this works on zeta/nightlies, should be good to merge.

@Taskeren
Copy link
Copy Markdown

Taskeren commented May 5, 2025

I'm not an ASM expert.
Will it break the assertion that each type of OreDictionary will be a singleton of list?
This is widely used for my works to get the oredict of items in a recipe, where an oredict is unfold into a list of itemstacks.

@mitchej123
Copy link
Copy Markdown
Contributor Author

mitchej123 commented May 5, 2025

I'm not an ASM expert. Will it break the assertion that each type of OreDictionary will be a singleton of list? This is widely used for my works to get the oredict of items in a recipe, where an oredict is unfold into a list of itemstacks.

The goal of this PR was to replace all Integer boxing/unboxing with primitives. I don't believe the list was ever a singleton before...

from getOreIDs() as an example:

        Integer[] tmp = set.toArray(new Integer[set.size()]);
        int[] ret = new int[tmp.length];
        for (int x = 0; x < tmp.length; x++)
            ret[x] = tmp[x];
        return ret;

And if you're directly reflecting to get it... it's still a private static Map<Integer, List<Integer>> stackToId. I've just replaced the impl with fastutil primative based collections. = new Int2ObjectOpenHashMap(96); and an IntList

@Taskeren
Copy link
Copy Markdown

Taskeren commented May 5, 2025

I'm not an ASM expert. Will it break the assertion that each type of OreDictionary will be a singleton of list? This is widely used for my works to get the oredict of items in a recipe, where an oredict is unfold into a list of itemstacks.

The goal of this PR was to replace all Integer boxing/unboxing with primitives. I don't believe the list was ever a singleton before...

from getOreIDs() as an example:

        Integer[] tmp = set.toArray(new Integer[set.size()]);
        int[] ret = new int[tmp.length];
        for (int x = 0; x < tmp.length; x++)
            ret[x] = tmp[x];
        return ret;

And if you're directly reflecting to get it... it's still a private static Map<Integer, List<Integer>> stackToId. I've just replaced the impl with fastutil primative based collections. = new Int2ObjectOpenHashMap(96); and an IntList

It is a map of singleton lists, I can confirm it. But if you just replace the implement of the list to IntList, it shouldn't be a big deal.

Thanks for explaination. But if we can have codes in comments, it's better, so we don't have to "compile" the code by brain lol.

@mitchej123
Copy link
Copy Markdown
Contributor Author

OreDictionaryPatched.java.txt

Decompiled output on a server. ASM was generated by modifying OreDictionary.java, compiling, and roughly comparing the bytecode between the patched/unpatched version.

@mitchej123 mitchej123 requested a review from a team May 6, 2025 23:26
@brandyyn
Copy link
Copy Markdown
Contributor

brandyyn commented May 7, 2025

Working fine in my 560 mod modpack 👍

@mitchej123
Copy link
Copy Markdown
Contributor Author

@Dream-Master good for you to merge once this has been confirmed working on Zeta/thermos.

@serenibyss serenibyss added the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label May 9, 2025
@Dream-Master
Copy link
Copy Markdown
Member

ill do a zeta update today and we will see

@Taskeren
Copy link
Copy Markdown

Looks this change breaks BW as I get the crash like this:

GT: 5.09.51.290-pre
Hodgepodge: 2.6.58-pre

java.lang.ClassCastException: class java.util.ArrayList cannot be cast to class it.unimi.dsi.fastutil.ints.IntList (java.util.ArrayList is in module java.base of loader 'bootstrap'; it.unimi.dsi.fastutil.ints.IntList is in unnamed module of loader 'Launch' @39ce27f2)
	at Launch//net.minecraftforge.oredict.OreDictionary.getOreIDs(Unknown Source)
	at Launch//bartworks.util.BWUtil.getOreNames(BWUtil.java:261)
	at Launch//bartworks.util.BWUtil.getCircuitTierFromItemStack(BWUtil.java:246)
	at Launch//bartworks.util.BWUtil.isTieredCircuit(BWUtil.java:256)
	at Launch//bartworks.system.material.CircuitGeneration.CircuitImprintLoader.isCircuitOreDict(CircuitImprintLoader.java:118)
	at Launch//bartworks.system.material.CircuitGeneration.CircuitImprintLoader.handleCircuitRecipeRebuilding(CircuitImprintLoader.java:92)
	at Launch//bartworks.system.material.CircuitGeneration.CircuitImprintLoader.lambda$rebuildCircuitAssemblerMap$0(CircuitImprintLoader.java:86)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)

This happens with my Twist 2.8.x nightly fork. LMK if you need any further details.

@mitchej123
Copy link
Copy Markdown
Contributor Author

Please provide a full log. I've run it in the full pack and haven't seen this issue - what did you do to trigger it?

@Taskeren
Copy link
Copy Markdown

Please provide a full log. I've run it in the full pack and haven't seen this issue - what did you do to trigger it?

https://github.com/Taskeren/Twist-Space-Technology-Mod/tree/release/2.8.x

I did nothing but start the game in debug mode (Java 17, Hotswap), and when I enter a world, this exception is thrown, and my game crashed.

Here is my full log: https://mclo.gs/AuSOcL6

@mitchej123
Copy link
Copy Markdown
Contributor Author

Try pulling the latest commit into 2.6.58-pre and see if that solves it.

@mitchej123
Copy link
Copy Markdown
Contributor Author

mitchej123 commented May 17, 2025

New version:

    public static void rebakeMap() {
        ((Int2ObjectOpenHashMap)stackToId).clear();

        for(int var0 = 0; var0 < idToStack.size(); ++var0) {
            List var1 = (List)idToStack.get(var0);
            if (var1 != null) {
                for(ItemStack var3 : var1) {
                    String var4 = var3.getItem().delegate.name();
                    int var5;
                    if (var4 == null) {
                        FMLLog.log(Level.DEBUG, "Defaulting unregistered ore dictionary entry for ore dictionary %s: type %s to -1", new Object[]{getOreName(var0), var3.getItem().getClass()});
                        var5 = -1;
                    } else {
                        var5 = GameData.getItemRegistry().getId(var4);
                    }

                    if (var3.getItemDamage() != 32767) {
                        var5 |= var3.getItemDamage() + 1 << 16;
                    }

                    Object var6 = (IntList)((Int2ObjectOpenHashMap)stackToId).get(var5);
                    if (var6 == null) {
                        var6 = new IntArrayList();
                        ((Int2ObjectOpenHashMap)stackToId).put(var5, var6);
                    }

                    ((IntList)var6).add(var0);
                }
            }
        }

@Taskeren
Copy link
Copy Markdown

What should I do to get the new commit in my project?

@mitchej123
Copy link
Copy Markdown
Contributor Author

checkout the -pre tag, and cherry pick it.

@mitchej123 mitchej123 merged commit c2f8a69 into master May 24, 2025
1 check passed
@mitchej123 mitchej123 deleted the speedup-oredictionary branch May 24, 2025 22:10
@serenibyss serenibyss removed the 🚧 Testing on Zeta Do not merge yet, testing this PR on Zeta label May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants