Uploaded image for project: 'JDK'
  1. JDK
  2. JDK-8043677

Performance regression in StringConcat opto: side-effect detection bails in trivial cases

    Details

    • Type: Bug
    • Status: Closed
    • Priority: P4
    • Resolution: Won't Fix
    • Affects Version/s: 8
    • Fix Version/s: None
    • Component/s: hotspot
    • Labels:

      Description

      Hi,

      This issue was originally diagnosed at
      http://stackoverflow.com/questions/23756966/why-is-stringbuilderappendint-faster-in-java-7-than-in-java-8

      Minimizing that test case into:

      @Fork(5)
      @Warmup(iterations = 5)
      @Measurement(iterations = 5)
      @BenchmarkMode(Mode.AverageTime)
      @OutputTimeUnit(TimeUnit.NANOSECONDS)
      @State(Scope.Benchmark)
      public class IntStr {
          private int counter;

          @GenerateMicroBenchmark
          public String inlineSideEffect() {
              return new StringBuilder().append(counter++).toString();
          }

          @GenerateMicroBenchmark
          public String spliceSideEffect() {
              int cnt = counter++;
              return new StringBuilder().append(cnt).toString();
          }
      }

      ...will yield, on JDK 7u55:
      Benchmark Mode Samples Mean Mean error Units
      o.s.IntStr.inlineSideEffect avgt 25 65.460 1.747 ns/op
      o.s.IntStr.spliceSideEffect avgt 25 64.414 1.323 ns/op

      ...and on JDK 8u5:
      Benchmark Mode Samples Mean Mean error Units
      o.s.IntStr.inlineSideEffect avgt 25 84.953 2.274 ns/op
      o.s.IntStr.spliceSideEffect avgt 25 65.386 1.194 ns/op

      I am pretty sure this is because JDK-8009303 added the memory flow verification, which detected the inline increment (actually, the store part) within the StringBuilder chain and bailed out. You can see if we splice out the increment by hand, the performance is back to where it was.

      Since we know the effects of StringBuilder, we can probably figure out the arguments are not dependent on SB instance state, and splice out append arguments automatically?

        Issue Links

          Activity

          Hide
          iveresov Igor Veresov added a comment -
          It's not an equivalent transform. For example "new StringBuilder" can through an OOM and we shouldn't see the counter incremented in that case.
          Show
          iveresov Igor Veresov added a comment - It's not an equivalent transform. For example "new StringBuilder" can through an OOM and we shouldn't see the counter incremented in that case.
          Hide
          shade Aleksey Shipilev added a comment -
          That's true, for plain Java case. But, can we OOM in the StringConcat-optimized code? If not, we can guarantee the side-effects will happen, and evaluate the append() arguments early, if those are trivial enough?
          Show
          shade Aleksey Shipilev added a comment - That's true, for plain Java case. But, can we OOM in the StringConcat-optimized code? If not, we can guarantee the side-effects will happen, and evaluate the append() arguments early, if those are trivial enough?
          Hide
          iveresov Igor Veresov added a comment -
          We certainly can OOM. We allocate the resulting string in which we stash the results of the individual appends. There are also many reasons uncommon traps can happen along the way. The result is, if we have to deopt there for whatever reason we need to be able return to the interpreter to the point where the first StringBuilder was being allocated and reexecute the whole thing (which will redo all side effects while reevaluating the arguments of appends). We preserve the local state to do that, but we can't roll back globally visible side-effects (which would require some kind of a global STM mechanism).
          Show
          iveresov Igor Veresov added a comment - We certainly can OOM. We allocate the resulting string in which we stash the results of the individual appends. There are also many reasons uncommon traps can happen along the way. The result is, if we have to deopt there for whatever reason we need to be able return to the interpreter to the point where the first StringBuilder was being allocated and reexecute the whole thing (which will redo all side effects while reevaluating the arguments of appends). We preserve the local state to do that, but we can't roll back globally visible side-effects (which would require some kind of a global STM mechanism).
          Hide
          shade Aleksey Shipilev added a comment -
          Yes, I understand the rationale for the original JDK-8009303. I missed the part that we need to return to *interpreter* which prevents us from rolling back to the modified code where trivial append() arguments are evaluated before diving into SB chain. Let's wait for others to comment on this, and lacking further comments, this ought to be closed as WNF.
          Show
          shade Aleksey Shipilev added a comment - Yes, I understand the rationale for the original JDK-8009303 . I missed the part that we need to return to *interpreter* which prevents us from rolling back to the modified code where trivial append() arguments are evaluated before diving into SB chain. Let's wait for others to comment on this, and lacking further comments, this ought to be closed as WNF.

            People

            • Assignee:
              iveresov Igor Veresov
              Reporter:
              shade Aleksey Shipilev
            • Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: