Minecraftforge: DimensionType - crash if many are registered - linked to EnumHelper.addEnum + JVM Optimiser!

[radfast]

In 1.10.2, run the code below as Forge's example mod.

EXPAND TO SEE THE CODE

package com.example.examplemod;

import net.minecraft.init.Blocks;
import net.minecraft.world.DimensionType;
import net.minecraft.world.WorldProviderEnd;
import net.minecraftforge.common.DimensionManager;
import net.minecraftforge.fml.common.Mod;
import net.minecraftforge.fml.common.Mod.EventHandler;
import net.minecraftforge.fml.common.event.FMLInitializationEvent;

@Mod(modid = ExampleMod.MODID, version = ExampleMod.VERSION)
public class ExampleMod
{
    public static final String MODID = "examplemod";
    public static final String VERSION = "1.0";

    @EventHandler
    public void init(FMLInitializationEvent event)
    {
        // some example code
        System.out.println("DIRT BLOCK >> "+Blocks.DIRT.getUnlocalizedName());
        testDimensions();
        System.out.println("Nothing broken!");
    }

    private void testDimensions()
    {
        for (int i = 2; i < 100; i++)
        {
            if (!DimensionManager.isDimensionRegistered(i))
            {
                //Register a new DimensionType - this extends the DimensionType enum
                DimensionType dt = DimensionType.register("TestDim", "Test", i, WorldProviderEnd.class, false);

                //This locally simulates DimensionType.getById(dt.getId() so that we can see the error
                getById(dt.getId(), DimensionType.values().length);
            }
        }
    }

    public static DimensionType getById(int id, int compareSize)
    {
        int valuesSize = DimensionType.values().length;
        if (valuesSize != compareSize) throw new IllegalArgumentException("Wow, this should be impossible!! Enum values size mismatch : " + compareSize + " vs. " + valuesSize);

        for (DimensionType dimensiontype : DimensionType.values())
        {
            if (dimensiontype.getId() == id)
            {
                return dimensiontype;
            }
        }

        return null;
    }
}

---

A "Wow, this should be impossible!" exception will be thrown after registering a certain number of DimensionType. For me, it's usually around 56 registered and then crash. YMMV. If you can't make it crash, try increasing the number of iterations!

This code sample simulates code used in an actual mod, OpenTerrainGenerator - look in their class com.khorn.terraincontrol.forge.TCPlugin.registerDimensions() if interested.

Why is this impossible? DimensionType.values().length should evaluate to the exact same value in the calling method testDimensions() and in the called method getById().

What I think is happening:

• EnumHelper.addEnum() is extending the DimensionTypes enum in its clever way, using reflection to modify fields inside the enum
• The calling code in the outer method testDimensions() keeps track correctly of the changing enum
• After it's been called already ~50 times, the inner method getById() is detected by the JVM Optimiser as inner loop code which needs to be optimised.
• The JVM Optimiser does not understand that the enum can change size so the Optimiser starts to use a cached value of the enum - by strange Optimiser logic, this cached value is only used inside the inner method getById().
• The cached value does not keep up with the changes made by EnumHelper.addEnum()

Wow! Not sure if that's the right explanation. If it is, then this could even be a bug in Java 8?

Why is this a problem? Modpacks with OpenTerrainGenerator combined with other mods are crashing, and other mods are not able to register their dimensions properly.

In the code sample here, you can see the method getById() is basically the same (apart from error tracking) as the method net.minecraft.world.DimensionType.getById(). And DimensionManager.registerDimension() calls this.

---

TL;DR: the issue I'm describing here, causes game crash with InvalidArgumentException from net.minecraft.world.DimensionType.getById() even when the calling code in the mod correctly registered the DimensionType. The problem can only be seen if a mod (for example OpenTerrainGenerator) registers many DimensionTypes.

Probably an issue in all Forge versions, though I've only tested in 1.10.2.

[williewillus]

this is irrelevant but how did you make the collapsible block like that? need to steal that for future tickets/PRs :P

[williewillus]

also, I can't reproduce this. Oracle's Java 8 64 bit on Windows 10

[radfast]

hey @williewillus :)

I'm a huge fan of yours! Thanks for your rendering primer, and those explanations of the Botania Platform smartmodel.

TO WILLIEWILLUS :)

I've recently implemented some elements of that in our Panel Lighting in Galacticraft - these are like camouflage blocks with lights added.

The collapsible block is made using HTML5 details tags like this:
<details> <summary>EXPAND ME</summary > codeblock </details>

What's nice is that somehow most of Github's markdown still works inside the tags - not quite everything, try it yourself you'll see.

---

Back on topic, my tests were run with Oracle jdk1.8.0_121 32-bit and forge-1.10.2-12.18.3.2239. I know that's not the latest Forge, but it's my modding workspace. I checked DimensionType + EnumHelper code history here and couldn't notice any relevant changes to it since it was first implemented this way in March 2016.

If you're not seeing it, please try increasing the number of iterations. If my theory that the Optimiser is causing this, then results are also going to vary from JVM to JVM, I guess.

[radfast]

The issue is not only on my JVM, I should say! These are the stats of the player who first reported the issue to us:

Minecraft Version: 1.10.2
Operating System: Windows 10 (amd64) version 10.0
Java Version: 1.8.0_25, Oracle Corporation
Java VM Version: Java HotSpot(TM) 64-Bit Server VM (mixed mode), Oracle Corporation
Memory: 1562194808 bytes (1489 MB) / 3981967360 bytes (3797 MB) up to 7639400448 bytes (7285 MB)
JVM Flags: 4 total; -XX:HeapDumpPath=MojangTricksIntelDriversForPerformance_javaw.exe_minecraft.exe.heapdump -Xmx8195m -Xms256m -XX:PermSize=256m
IntCache: cache: 0, tcache: 0, allocated: 0, tallocated: 0
FML: MCP 9.32 Powered by Forge 12.18.3.2281 Optifine OptiFine_1.10.2_HD_U_D8 138 mods loaded, 138 mods active

[williewillus]

that java version is quite old though (it's 8 update 25, I have 8 update 131 installed), does it happen on new javas?

[radfast]

that player is on update 25, I'm on update 121 when I was able to reproduce it and isolate it to my example code

I'll grab 131 and see if it's still happening

[LexManos]

Ya, its probably an issue, however, there isn't much we can do about it but say that you shouldn't make registerDimension a hotspot on your JVM.
We have to hack enums, thats just how it works, and there isn't anything we can do that I know of to tell the JVM that things shouldn't be optimized/arnt final.

[radfast]

Thanks guys. Let's hope the OpenTerrainGenerator people see this then :)

[radfast]

Hmm looks like you were already aware of the potential issue, otherwise why have this validity check and comment in DimensionManager.registerDimension():
>
public static void registerDimension(int id, DimensionType type)
{
DimensionType.getById(type.getId()); //Check if type is invalid {will throw an error} No clue how it would be invalid tho...
if (dimensions.containsKey(id))
{
throw new IllegalArgumentException(String.format("Failed to register dimension for id %d, One is already registered", id));
}
dimensions.put(id, type);
if (id >= 0)
{
dimensionMap.set(id);
}
}

[Alpvax]

@Beanius Did you check that you were actually using the correct JVM? Sounds like you have multiple ones installed.

[Barteks2x]

Based on what I found any reasonably recent JVM allows to exclude compiling select methods from commandline arguments (I found it here).

But even if it works this way, there is no way to firce this option to be used (I didn't test it).

[AEnterprise]

@Beanius minecraft launcher downloads it's own copy of java to run minecraft with so unless you point it to your local installation it will keep using the (outdated) java version that ships with MC

[shadowfacts]

Because a lot of people don't know how to install Java or have a modern version.

Besides, this isn't the place to discuss this, use the forum.

[radfast]

• If the issue is seen with the JVM shipped by Minecraft (currently 1.8.0_25, thank you @AEnterprise) then it's an issue - you can't expect all Minecraft players to know to start with a different local installation of Java

• If the issue is seen with 32-bit JVM it's also an issue - not every Minecraft player has a 64-bit OS, some people are using old laptops etc

@LexManos my test code shows that the issue is / can be (depending on JVM) produced after registering only 55 DimensionTypes. 55 is not an unreasonable number of DimensionTypes. I am not here defending the strange code seen in OpenTerrainGenerator (which the devs have now updated).

I'm concerned about DimensionTypes created by mods that actually need a lot of DimensionTypes.

Our mod, Galacticraft, creates 6 new DimensionTypes as currently shipped (Moon, Mars, Asteroids, Venus, and two types of space station). We plan to add more in future. There are other developers making sub-mods for Galacticraft which add 20-30 additional planets. Each planet has a different DimensionType because it has a different WorldProvider class.

Put all that together with other Forge mods which also register DimensionTypes (there are many; and lots of mods have storage dimensions or auto-building dimensions) and you could very easily get to over 55 DimensionTypes in a large modpack.

I suggest don't use DimensionType.values() in Forge code, instead have a static ImmutableList caching the Enum values. The EnumHelper.addEnum() methods can then replace that ImmutableList at the same time as poking around with the Enum internals.

It's good practice to cache Enum.values() anyhow, otherwise there's a performance and memory/GC hit every time you call the method because it creates a new array every time! (Which is probably why the JVM optimiser looks at it as a hotspot) Further reading.

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time, and will be closed if no further activity occurs. If this issue was overlooked, forgotten, or should remain open for any other reason, please reply here to call attention to it and remove the stale status. Thank you for your contributions.

[williewillus]

still valid

[liach]

I think we may rather add enums to a pending list, then adding the list to the original enums and injecting values when we reach a loading state to bypass this issue temporarily.

[liach]

Actually, there is a really hacky way to let a method bypass inlines: we can inject equivalent of this code block:

if (false) new Throwable();

Since Java Virtual Machine NEVER inline throwable constructors.

[liach]

To clarify, @radfast's assumption is correct.

When I change the DynamicEnumTest's original test section to

    private enum TestEnum {
        v_0
    }

    public static void main(String[] args) throws IOException {

        // Dynamically add 3 new enum instances d, e, f to TestEnum

        for (int i = 1; i <= 10000; i++) {
            addEnum(TestEnum.class, "v_" + i);
            TestEnum.values();
        }

        // Run a few tests just to show it works OK.
        System.out.println(Arrays.deepToString(TestEnum.values()));
        // Shows : [a, b, c, d, e, f]
    }

With this test, the last element outputted is almost never v_10000, proving him correct. Maybe this issue is not JVM dependent. It also cannot be prevented by changing the visibility of the $VALUES field (or making it non-final, a thing the original enum test already did)

[williewillus]

Since Java Virtual Machine NEVER inline throwable constructors.

does it really? is there a spec/link somewhere that states that that is not allowed? If it's not in a spec then all bets are off since different VM's can do it differently.

[liach]

@williewillus

@ 240   java.lang.RuntimeException::<init> (7 bytes)   don't inline Throwable constructors

https://techblug.wordpress.com/2013/08/19/java-jit-compiler-inlining/
This is what I got when I run my test program above with -XX:+PrintCompilation -XX:+UnlockDiagnosticVMOptions -XX:+PrintInlining jvm flags. The website link is the source that I refered to.

[williewillus]

but if it's not in the JVM spec, another JVM implementation (J9, Excelsior JET, OpenJDK, Oracle's proprietary) could behave differently

so I think bothering mojang to un-enumify these things (as Lex mentioned on twitter) is a better way to go

[liach]

I changed it to a throw so that the vm must not inline (otherwise the stacktrace will be broken)

[liach]

Noop, I find that I just need to blast the final modifier on the $VALUES field with asm. Pull request ready soon.

[liach]

To clarify, the actual explanation is this:
The inliner did not inline any method call: it inlines private static final DimensionType[] $VALUES field reference instead, so that the actual pointer of the field changed, the inlined pointer is still there.

[radfast]

Thank you to all who have thought about this and contributed, the people who make Forge work and keep it working are the real heroes of the Minecraft community. (I am sorry to see it turned a bit ugly on thiakil's PR. Of course all contributors are trying to find a practical and robust solution, but ultimately Lex' views have to be respected.)

When first posted I thought this issue would be interesting even if an edge case, because it is illustrative of a more fundamental problem as discussed, and who doesn't enjoy the nitty gritty stuff about the JVM optimiser and how Enum really works. (Maybe it could also be the root cause of one or two other unexplained issues though none sping to mind right now?)

I've now realised that it's possible this is the cause of a tricky issue which the relevant mod authors and Sponge devs have not been able to solve for ~ 1 year now. The issue is that after a certain number _N_ of new DimensionType are registered on a Sponge server, some of those dimensions start to be remapped to the wrong DimensionType. So when the dimension loads, it gets the wrong WorldProvider class. In testing by one of the mod authors _N_ can be quite small like around 15. It may be a problem on Sponge's side (or in our mod code :o ) but seems like a chance it could be connected with the issues discussed here. Maybe Sponge makes some things more of a hotspot.

---

LexManos: Ya, its probably an issue, however, there isn't much we can do about it but say that you shouldn't make registerDimension a hotspot on your JVM.

When registering the different planet dimensions on the server, didn't seem like we really have any choice about registering them in a loop during server start. The dimensions have to all be registered before any players login, because a player logging in could be located in any one of these dimensions.

But I will look now at whether we can try to de-hotspot this, for example by registering one dimension per tick after the server starts and preventing player login for the first _N_ ticks. (I don't know how to prevent player login temporarily, if anyone knows a way I would sure appreciate a pointer.)

---

@liach: To clarify, @radfast's assumption is correct.

BFF <3 <3 ❤️ 👍 could you maybe say this to my family from time to time?

[radfast]

Meanwhile we are busy still creating new dimensions :) Here's the next one coming, currently WIP:

[Barteks2x]

I was able to successfully work around the issue in tests (didn't test with actual dimensions) by using JVM option in this form: -XX:CompileCommand="exclude name.of.the.EnumClass::values" (or -XX:CompileCommandFile=SomeFile.txt with list of the options). But it's likely to have negative impact on performance as it's going to use interpreter for the values() method. If it actually affects some users and there is no other workaround, this may be worth trying.

[YoungCP]

As mentioned by @SquidDev in #4931 ...

I couldn't get CompileCommand to work, but there's a way to do this without adding anything to the command line. A .hotspot_compiler file (should it exist) in the minecraft instance directory (e.g. directory containing options.txt, knownkeys.txt, etc.) is always loaded and parsed by the JVM. That could be distributed with forge, or even created on first-run (then restart)...

edit; Mods might need to pre-identify enums that need to be appended/manipulated and then an exclude directive will need to be added for that enum? A default list could be included?

edit: I'm wrong;
Java HotSpot(TM) 64-Bit Server VM warning: .hotspot_compiler file is present but has been ignored. Run with -XX:CompileCommandFile=.hotspot_compiler to load the file.

[thiakil]

Couple of solutions I found to this:

• JVM params -XX:FreqInlineSize=9 -XX:MaxInlineSize=9
  
  
  
  • Probably bad performance, according to this the defaults are much higher.
  
  
• Storing registrations to a List/Map in DimensionType and modifying byName/getById to use it
  
  
  
  • Also (ideally) involves adding utility methods to use the list/map to get an array of values that's guaranteed to not be cached
  
  
  • Avoids making values() a hotspot so easily
  
  

[stale[bot]]

This issue has been automatically marked as stale because it has not had activity in a long time. If this issue is still relevant and should remain open, please reply with a short explanation (e.g. "I have checked the code and this issue is still relevant because ___." or "Here's a screenshot of this issue on the latest version"). Thank you for your contributions!

[stale[bot]]

This issue has been automatically closed because it has not had activity in a long time. Please feel free to reopen it or create a new issue.

[PG85]

Hey guys, a bit late to catch this, just got a bug report for it so I found this old issue. Just want to say thanks for the effort looking into this. Even though OTG no longer creates hotspots, I do still get this issue reported occasionally with other mods for 1.12.2. I suppose all we can do for now is try to avoid creating hotspots and hope for a proper fix with Mojang's help. Thanks and cheers.