Uploaded image for project: 'Minecraft: Java Edition'
  1. Minecraft: Java Edition
  2. MC-134989

AbstractMap::hashCode accounts for substantial CPU overhead (from profiling)

    XMLWordPrintable

    Details

    • Type: Bug
    • Status: Resolved
    • Resolution: Fixed
    • Affects Version/s: Minecraft 1.13
    • Fix Version/s: Minecraft 1.13.1
    • Labels:
      None
    • Environment:

      Linux compute0.[redacted].com 4.1.4-gentoo #1 SMP Mon Sep 21 04:48:00 EDT 2015 x86_64 Intel(R) Core(TM) i5-4430 CPU @ 3.00GHz GenuineIntel GNU/Linux
      openjdk version "1.8.0_171"

    • Confirmation Status:
      Unconfirmed

      Description

      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
              }
       

        Attachments

          Activity

            People

            • Assignee:
              grum [Mojang] Grum (Erik Broes)
              Reporter:
              theosib2 Timothy Miller
            • Votes:
              3 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: