Bug report on high CPU usage in AbstractMap::hashCode

AbstractMap::hashCode is a substantial portion of Minecraft CPU overhead, because child classes of PropertyEnum (all of which are interned) are implementing equals and hashCode incorrectly.  I discovered this in 1.12.2, but it is still implemented this way in 1.13, and this could help a lot with some of the performance problems.


I was doing some profiling on Minecraft 1.12.2 using Honest Profiler (https://github.com/jvm-profiling-tools/honest-profiler), and something that kept showing up in the top ranks of CPU users was {{java.util.AbstractMap::hashCode}}.  I credit Pokechu22 for recognizing what that means:  *A hashmap or hashset was being used as the KEY for another container.*

The offender turned out to be {{net.minecraft.block.properties.PropertyEnum}}.   This contains both a HashSet and a HashMap:

    private final ImmutableSet<T> allowedValues;
    private final Map<String, T> nameToValue = Maps.<String, T>newHashMap();

And then it uses them both to compute the hash:

    public int hashCode()
    {
        int i = super.hashCode();
        i = 31 * i + this.allowedValues.hashCode();
        i = 31 * i + this.nameToValue.hashCode();
        return i;
    }

This gets called from all over the place, and the common code path for them all is:

     (t  0.1,s  0.0) net.minecraft.block.state.BlockStateContainer$StateImplementation::getValue
      (t  0.1,s  0.0) com.google.common.collect.RegularImmutableMap::get
       (t  0.1,s  0.0) com.google.common.collect.RegularImmutableMap::get
        (t  0.1,s  0.0) net.minecraft.block.properties.PropertyEnum::hashCode
         (t  0.1,s  0.0) java.util.AbstractMap::hashCode
          (t  0.1,s  0.1) java.util.HashMap$Node::hashCode

Most or all blocks have a {{PropertyEnum}} as a member variable.  But there are a few critical cases where the PropertyEnum is used as a KEY, and one turns out to be in redstone components, which is acknowledged to need significant optimization.

For one example, in {{BlockRedstoneRepeater}}, we have this:

    protected IBlockState getPoweredState(IBlockState unpoweredState)
    {
        Integer integer = (Integer)unpoweredState.getValue(DELAY);
        Boolean obool = (Boolean)unpoweredState.getValue(LOCKED);
        EnumFacing enumfacing = (EnumFacing)unpoweredState.getValue(FACING);
        return Blocks.POWERED_REPEATER.getDefaultState().withProperty(FACING, enumfacing).withProperty(DELAY, integer).withProperty(LOCKED, obool);
    }

{{BlockRedstoneDiode.isSameDiode}} is also an extremely frequent caller to {{BlockRedstoneRepeater.getPoweredState}}:

    public boolean isSameDiode(IBlockState state)
    {
        Block block = state.getBlock();
        return block == this.getPoweredState(this.getDefaultState()).getBlock() || block == this.getUnpoweredState(this.getDefaultState()).getBlock();
    }

Here, {{this.getDefaultState()}} leads to this in Block.java:

    protected final BlockStateContainer blockState;

It all leads to one place:  net.minecraft.block.state.BlockStateContainer.StateImplementation.getValue

That class contains this member variable:

        private final ImmutableMap < IProperty<?>, Comparable<? >> properties;

Accessed this way:

        public <T extends Comparable<T>> T getValue(IProperty<T> property)
        {
            Comparable<?> comparable = (Comparable)this.properties.get(property);
        ...

The only thing that "implements IProperty" was block/properties/PropertyHelper.java, and block/properties/PropertyEnum.java extends {{PropertyHelper}}.

To summarize, in most or all cases when you want to get a block property, you end up in code that uses a block property type object as a hashmap key, and computing the hash of a map is *expensive*.

Since these objects are supposed to be immutable, one way to fix this is to _cache_ the hash code.  That is, compute the hash in the object's constructor and store it in an object variable.

However, in a direct discussion with Grum, he pointed out these for these classes, there is only a single instance for any given value, which means that the code should be using {{==}} for comparison and {{System.identityHashCode(this)}} for the hash code.  In other words, there are four classes under net/minecraft/block/properties (now net/minecraft/state with different names) that need to be modified:

- PropertyHelper.java
- PropertyBool.java
- PropertyEnum.java
- PropertyInteger.java

And they should implement equals and hashCode as follows, quoting Grum:

        @Override
        public boolean equals(final Object o) {
            // We're singletons.
            return this == o;
        }

        @Override
        public int hashCode() {
            return System.identityHashCode(this); // like object
        }