JDK-8074383 : Redundant "compare" in generated code for String.charAt bounds checks
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: compiler
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2015-03-04
  • Updated: 2015-05-15
  • Resolved: 2015-05-15
Related Reports
Relates :  
Relates :  
Description
If you run a simple benchmark like this:

    private String s;
    private int idx;

    @Setup
    public void setup() {
        idx = 13;
        s = "Improve the scope of range check optimizations";
    }

    @Benchmark
    public void test_charAt() {
        trampoline_charAt();
    }

    @CompilerControl(CompilerControl.Mode.DONT_INLINE)
    public char trampoline_charAt() {
        return s.charAt(idx);
    }

...then you will see this generated code with the latest jdk9-dev:

 mov    %eax,-0x14000(%rsp)
 push   %rbp
 sub    $0x20,%rsp 
 mov    0xc(%rsi),%r11d        ; get field $idx
 mov    0x10(%rsi),%r10d       ; get field $s
 mov    0xc(%r12,%r10,8),%r9d  ; get field $s.value
 test   %r11d,%r11d            
 jl     0x00007f6204b5cc19     ; range check 1, jump if ($idx < 0)
 mov    0xc(%r12,%r9,8),%ebp   ; get $s.value.arraylength
 cmp    %ebp,%r11d             
 jge    0x00007f6204b5cc31     ; range check 2, jump if ($idx >= arraylength)
 cmp    %ebp,%r11d             ; <--- REDUNDANT COMPARE
 jae    0x00007f6204b5cc03     ; jump if overflow?
 lea    (%r12,%r9,8),%r10      ; unpack $s.value array reference
 movzwl 0x10(%r10,%r11,2),%eax ; get $s.value[$idx]
 add    $0x20,%rsp             ; epilogue and return
 pop    %rbp
 test   %eax,0x15f1a3fe(%rip) 
 retq   

As you can see, we do double compare on the same arguments, even though jumps are not affecting the flags, and there seem to be no incoming branches to the second cmp.

Comments
Confirmed: this generated code quirk is gone with recent JDK 9 build that has JDK-8073480 change in it. Closing as fixed.
15-05-2015

Excellent! I will verify once JDK-8073480 is pushed.
10-03-2015

My current patch for JDK-8073480 indeed fixes this: 0x00007f35511c2380: mov %eax,-0x16000(%rsp) 0x00007f35511c2387: push %rbp 0x00007f35511c2388: sub $0x20,%rsp ;*synchronization entry ; - org.openjdk.DoubleCmp::trampoline_charAt@-1 (line 92) 0x00007f35511c238c: mov 0xc(%rsi),%r11d ;*getfield idx ; - org.openjdk.DoubleCmp::trampoline_charAt@5 (line 92) 0x00007f35511c2390: mov 0x10(%rsi),%r10d ;*getfield s ; - org.openjdk.DoubleCmp::trampoline_charAt@1 (line 92) 0x00007f35511c2394: mov 0xc(%r12,%r10,8),%r9d ;*getfield value ; - java.lang.String::charAt@6 (line 662) ; - org.openjdk.DoubleCmp::trampoline_charAt@8 (line 92) ; implicit exception: dispatches to 0x00007f35511c23d1 ;; B2: # B7 B3 <- B1 Freq: 0.999999 0x00007f35511c2399: mov 0xc(%r12,%r9,8),%r8d ; implicit exception: dispatches to 0x00007f35511c23e5 ;; B3: # B5 B4 <- B2 Freq: 0.999998 0x00007f35511c239e: cmp %r8d,%r11d 0x00007f35511c23a1: jae 0x00007f35511c23b9 ;*if_icmplt ; - java.lang.String::charAt@10 (line 662) ; - org.openjdk.DoubleCmp::trampoline_charAt@8 (line 92) ;; B4: # N68 <- B3 Freq: 0.999997 0x00007f35511c23a3: lea (%r12,%r9,8),%r10 0x00007f35511c23a7: movzwl 0x10(%r10,%r11,2),%eax ;*caload ; - java.lang.String::charAt@27 (line 665) ; - org.openjdk.DoubleCmp::trampoline_charAt@8 (line 92) 0x00007f35511c23ad: add $0x20,%rsp 0x00007f35511c23b1: pop %rbp 0x00007f35511c23b2: test %eax,0xbc34c48(%rip) # 0x00007f355cdf7000 ; {poll_return} 0x00007f35511c23b8: retq Let's keep that bug open until JDK-8073480 is pushed and you can verify it, Aleksey?
10-03-2015

Benchmark: http://cr.openjdk.java.net/~shade/8074383/DoubleCmp.java Executable JAR: http://cr.openjdk.java.net/~shade/8074383/benchmarks.jar Perfasm output (with sanely formatted assembly): http://cr.openjdk.java.net/~shade/8074383/output.perfasm
04-03-2015