Why copy a field reference to a local before using it in a loop?

Question

What's the advantage of this OpenJDK line number 1455.

Code snippet:

private final char value[];
// ...
public int hashCode() {
    int h = hash;
    if (h == 0 && value.length > 0) {



        char val[] = value;      // <--- this line



        for (int i = 0; i < value.length; i++) {
            h = 31 * h + val[i];
        }
        hash = h;
    }
    return h;
}

Notice that, although a reference to private final char value[] is copied to the local val for access inside the loop, its .length field is still accessed through value, not val.

I suspect "performance" to be the answer (e.g. it is faster to read from the local than from the field) but I would appreciate a precise and easy-to-read answer, perhaps even some data about the advantage.

Answer 1

I took a look at the bytecode, as @user commented, it's probably an optimization to avoid the getfield call within the loop. But they messed up and still reference the the value variable in the loop condition... so this actually makes the bytecode longer and slower.

public int h1() {
    int h = hash;
    if (h == 0 && value.length > 0) {
        char val[] = value;
        for (int i = 0; i < value.length; i++) {
            h = 31 * h + val[i];
        }
        hash = h;
    }
    return h;
}

public int h2() {
    int h = hash;
    if (h == 0 && value.length > 0) {
        for (int i = 0; i < value.length; i++) {
            h = 31 * h + value[i];
        }
        hash = h;
    }
    return h;
}

We can see that both methods produce almost identical bytecode, however our "optimized" implementation actually ends up using 2 more calls.

Notice how the for loop test (lines 39-45 in h1, and lines 37-43 in h2) call getfield to do the array length call.

Bytecode:

public int h1();
Code:
   0: aload_0
   1: getfield      #17                 // Field hash:I
   4: istore_1
   5: iload_1
   6: ifne          53
   9: aload_0
  10: getfield      #15                 // Field value:[C
  13: arraylength
  14: ifle          53
  17: aload_0
  18: getfield      #15                 // Field value:[C
  21: astore_2
  22: iconst_0
  23: istore_3
  24: goto          39
  27: bipush        31
  29: iload_1
  30: imul
  31: aload_2
  32: iload_3
  33: caload
  34: iadd
  35: istore_1
  36: iinc          3, 1
  39: iload_3
  40: aload_0
  41: getfield      #15                 // Field value:[C
  44: arraylength
  45: if_icmplt     27
  48: aload_0
  49: iload_1
  50: putfield      #17                 // Field hash:I
  53: iload_1
  54: ireturn

public int h2();
Code:
   0: aload_0
   1: getfield      #17                 // Field hash:I
   4: istore_1
   5: iload_1
   6: ifne          51
   9: aload_0
  10: getfield      #15                 // Field value:[C
  13: arraylength
  14: ifle          51
  17: iconst_0
  18: istore_2
  19: goto          37
  22: bipush        31
  24: iload_1
  25: imul
  26: aload_0
  27: getfield      #15                 // Field value:[C
  30: iload_2
  31: caload
  32: iadd
  33: istore_1
  34: iinc          2, 1
  37: iload_2
  38: aload_0
  39: getfield      #15                 // Field value:[C
  42: arraylength
  43: if_icmplt     22
  46: aload_0
  47: iload_1
  48: putfield      #17                 // Field hash:I
  51: iload_1
  52: ireturn

If they had changed the loop condition to also use the local field,

...
for (int i = 0; i < val.length; i++) {
...

Then the bytecode actually becomes shorter and loses the arguably slower getfield call in the loop,

 public int h1();
Code:
   0: aload_0       
   1: getfield      #17                 // Field hash:I
   4: istore_1      
   5: iload_1       
   6: ifne          50
   9: aload_0       
  10: getfield      #15                 // Field value:[C
  13: arraylength   
  14: ifle          50
  17: aload_0       
  18: getfield      #15                 // Field value:[C
  21: astore_2      
  22: iconst_0      
  23: istore_3      
  24: goto          39
  27: bipush        31
  29: iload_1       
  30: imul          
  31: aload_2       
  32: iload_3       
  33: caload        
  34: iadd          
  35: istore_1      
  36: iinc          3, 1
  39: iload_3       
  40: aload_2       
  41: arraylength   
  42: if_icmplt     27
  45: aload_0       
  46: iload_1       
  47: putfield      #17                 // Field hash:I
  50: iload_1       
  51: ireturn       

Doing a dumb test of just looping over the method a few million times on my jdk 1.7.0_79 consistently shows the original method hilariously takes about 5 times longer to run then either the un-"optimized" or the correctly "optimized" methods. The latter 2 having no difference in performance.

So I guess in conclusion, storing the field locally was an attempt at optimizing the methods bytecode, probably before the jit was able to optimize this itself, but they messed it up and actually made the method a lot worse...

This code is actually fixed in Java 9 per, https://bugs.openjdk.java.net/browse/JDK-8058643