RFR: 8261137: Optimization of Box nodes in uncommon_trap

classic Classic list List threaded Threaded
32 messages Options
12
Reply | Threaded
Open this post in threaded view
|

RFR: 8261137: Optimization of Box nodes in uncommon_trap

Wang Huang
JDK-8075052 have removed useless autobox. However, in some cases, the box is still saved. For instance:
@Benchmark
public void testMethod(Blackhole bh) {
  int sum = 0;
  for (int i = 0; i < data.length; i++) {
      Integer ii = Integer.valueOf(data[i]);
      if (i < data.length) {
          sum += ii.intValue();
      }
  }
  bh.consume(sum);
}
Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
The uncommon_trap is generated by the optimized "if", because its condition is always true.

We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
and deleting the use of box:

There is no additional fail/error(s) of jtreg after this patch.

-------------

Commit messages:
 - 8261137: Optimization of Box nodes in uncommon_trap

Changes: https://git.openjdk.java.net/jdk/pull/2401/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2401&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261137
  Stats: 46 lines in 1 file changed: 45 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2401.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2401/head:pull/2401

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap

Eric Liu-2
On Thu, 4 Feb 2021 08:43:35 GMT, Wang Huang <[hidden email]> wrote:

> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
> @Benchmark
> public void testMethod(Blackhole bh) {
>   int sum = 0;
>   for (int i = 0; i < data.length; i++) {
>       Integer ii = Integer.valueOf(data[i]);
>       if (i < data.length) {
>           sum += ii.intValue();
>       }
>   }
>   bh.consume(sum);
> }
> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>
> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
> and deleting the use of box:
>
> There is no additional fail/error(s) of jtreg after this patch.

I was wandering if we can remove the useless `if` as it's always true in this case. Do you know why this kind of `if` haven't been eliminated by GVN phase?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap

Nils Eliasson-2
In reply to this post by Wang Huang
On Thu, 4 Feb 2021 08:43:35 GMT, Wang Huang <[hidden email]> wrote:

> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
> @Benchmark
> public void testMethod(Blackhole bh) {
>   int sum = 0;
>   for (int i = 0; i < data.length; i++) {
>       Integer ii = Integer.valueOf(data[i]);
>       if (i < data.length) {
>           sum += ii.intValue();
>       }
>   }
>   bh.consume(sum);
> }
> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>
> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
> and deleting the use of box:
>
> There is no additional fail/error(s) of jtreg after this patch.

src/hotspot/share/opto/callGenerator.cpp line 606:

> 604:   }
> 605:
> 606:   if (callprojs.resproj != NULL && call->is_CallStaticJava() &&

Please extract the new code into a method with a descriptive name.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap

Wang Huang
On Thu, 4 Feb 2021 12:41:50 GMT, Nils Eliasson <[hidden email]> wrote:

>> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
>> @Benchmark
>> public void testMethod(Blackhole bh) {
>>   int sum = 0;
>>   for (int i = 0; i < data.length; i++) {
>>       Integer ii = Integer.valueOf(data[i]);
>>       if (i < data.length) {
>>           sum += ii.intValue();
>>       }
>>   }
>>   bh.consume(sum);
>> }
>> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
>> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>>
>> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
>> and deleting the use of box:
>>
>> There is no additional fail/error(s) of jtreg after this patch.
>
> src/hotspot/share/opto/callGenerator.cpp line 606:
>
>> 604:   }
>> 605:
>> 606:   if (callprojs.resproj != NULL && call->is_CallStaticJava() &&
>
> Please extract the new code into a method with a descriptive name.

OK. I will refactor these codes ASAP.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap

Wang Huang
In reply to this post by Eric Liu-2
On Thu, 4 Feb 2021 10:01:50 GMT, Eric Liu <[hidden email]> wrote:

> I was wandering if we can remove the useless `if` as it's always true in this case. Do you know why this kind of `if` haven't been eliminated by GVN phase?
It is a good idea . However, C2 might not optimize some `if` for many reasons:
*  This situation can also be triggered in calling other method. For example :
  ```
public class MyBenchmark {

    static int[] data = new int[10000];

    static {
        for(int i = 0; i < data.length; ++i) {
            data[i] = 299;
        }
    }

    @Benchmark
    public void testMethod(Blackhole bh) {
      int sum = 0;
      for (int i = 0; i < data.length; i++) {
        Integer ii = Integer.valueOf(data[i]);
        sum += abs(ii);
      }
      bh.consume(sum);
    }

    public int abs(Integer ii) {
      if (ii.intValue() > 0) {
        return ii.intValue();
      } else {
        return 0 - ii.intValue();
      }
    }
}
 The method `abs` will be inlined when it is hot enough. However, this `If` can not be eliminated.
*  The case in the JDK-8261137 is just like this case:
public class MyBenchmark {

    static int[] data = new int[10000];

    static {
        for(int i = 0; i < data.length; ++i) {
            data[i] = i * 1337 % 7331;
        }
    }

    @Benchmark
    public void testMethod(Blackhole bh) {
      int sum = 0;
      for (int i = 0; i < data.length; i++) {
          Integer ii = Integer.valueOf(data[i]);
          if (i < 100000) {
              sum += ii.intValue();
          }
      }
      bh.consume(sum);
    }
}

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Wang Huang
In reply to this post by Wang Huang
> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
> @Benchmark
> public void testMethod(Blackhole bh) {
>   int sum = 0;
>   for (int i = 0; i < data.length; i++) {
>       Integer ii = Integer.valueOf(data[i]);
>       if (i < data.length) {
>           sum += ii.intValue();
>       }
>   }
>   bh.consume(sum);
> }
> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>
> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
> and deleting the use of box:
>
> There is no additional fail/error(s) of jtreg after this patch.

Wang Huang has updated the pull request incrementally with one additional commit since the last revision:

  refactor codes

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2401/files
  - new: https://git.openjdk.java.net/jdk/pull/2401/files/73be94ce..4dfee52a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2401&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2401&range=00-01

  Stats: 91 lines in 1 file changed: 47 ins; 43 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2401.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2401/head:pull/2401

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap

Eric Liu-2
In reply to this post by Wang Huang
On Fri, 5 Feb 2021 07:15:55 GMT, Wang Huang <[hidden email]> wrote:

> > I was wandering if we can remove the useless `if` as it's always true in this case. Do you know why this kind of `if` haven't been eliminated by GVN phase?

> It is a good idea . However, C2 might not optimize some `if` for many reasons:
>
> * This situation can also be triggered in calling other method. For example :
>
> ```
> public class MyBenchmark {
>
>   static int[] data = new int[10000];
>
>   static {
>       for(int i = 0; i < data.length; ++i) {
>           data[i] = 299;
>       }
>   }
>
>   @Benchmark
>   public void testMethod(Blackhole bh) {
>     int sum = 0;
>     for (int i = 0; i < data.length; i++) {
>       Integer ii = Integer.valueOf(data[i]);
>       sum += abs(ii);
>     }
>     bh.consume(sum);
>   }
>
>   public int abs(Integer ii) {
>     if (ii.intValue() > 0) {
>       return ii.intValue();
>     } else {
>       return 0 - ii.intValue();
>     }
>   }
> }
> ```
>
> The method `abs` will be inlined when it is hot enough. However, this `If` can not be eliminated.
>
> * The case in the JDK-8261137 is just like this case:
>
> ```
> public class MyBenchmark {
>
>     static int[] data = new int[10000];
>
>     static {
>         for(int i = 0; i < data.length; ++i) {
>             data[i] = i * 1337 % 7331;
>         }
>     }
>
>     @Benchmark
>     public void testMethod(Blackhole bh) {
>       int sum = 0;
>       for (int i = 0; i < data.length; i++) {
>           Integer ii = Integer.valueOf(data[i]);
>           if (i < 100000) {
>               sum += ii.intValue();
>           }
>       }
>       bh.consume(sum);
>     }
> }
> ```

Thanks for your explanation. This makes more sense to me now.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Vladimir Kozlov-2
In reply to this post by Wang Huang
On Fri, 5 Feb 2021 07:29:00 GMT, Wang Huang <[hidden email]> wrote:

>> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
>> @Benchmark
>> public void testMethod(Blackhole bh) {
>>   int sum = 0;
>>   for (int i = 0; i < data.length; i++) {
>>       Integer ii = Integer.valueOf(data[i]);
>>       if (i < data.length) {
>>           sum += ii.intValue();
>>       }
>>   }
>>   bh.consume(sum);
>> }
>> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
>> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>>
>> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
>> and deleting the use of box:
>>
>> There is no additional fail/error(s) of jtreg after this patch.
>
> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
>
>   refactor codes

I am a little concern about stretching uses of input value outside scope where it is created (for example, loop's variable or a value depending on it).
This optimization may work only because boxed values are immutable.
I will run our tests with this changes.

src/hotspot/share/opto/callGenerator.cpp line 582:

> 580:         Node* uncommon_trap_node = delay_boxes.pop();
> 581:         int in_edge = uncommon_trap_node->find_edge(res);
> 582:         assert(in_edge > 0, "sanity");

If there are several references you need to replace all of them.

src/hotspot/share/opto/callGenerator.cpp line 591:

> 589:         Node* sobj = new SafePointScalarObjectNode(gvn.type(res)->isa_oopptr(),
> 590: #ifdef ASSERT
> 591:                                                   NULL,

I would suggest to record `call` node here treating it as allocation.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Vladimir Kozlov-2
On Mon, 8 Feb 2021 18:45:37 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   refactor codes
>
> I am a little concern about stretching uses of input value outside scope where it is created (for example, loop's variable or a value depending on it).
> This optimization may work only because boxed values are immutable.
> I will run our tests with this changes.

Our tier1-4 testing passed

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Xin Liu
On Mon, 8 Feb 2021 22:47:43 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> I am a little concern about stretching uses of input value outside scope where it is created (for example, loop's variable or a value depending on it).
>> This optimization may work only because boxed values are immutable.
>> I will run our tests with this changes.
>
> Our tier1-4 testing passed

> I was wandering if we can remove the useless `if` as it's always true in this case. Do you know why this kind of `if` haven't been eliminated by GVN phase?

Eventually, c2 should know that `i < data.length' is true. it should be done by GCP later.

The example is a special case.  you can remove this " if (i < data.length)"  here.  Generally, target code can look like this. c2 speculatively generates an uncommon_trap of "unstable_if".

  for (int i = 0; i < data.length; i++) {
      Integer ii = Integer.valueOf(data[i]);
      if (cond) { //likely
          sum += ii.intValue();
      }
  }

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Xin Liu
In reply to this post by Wang Huang
On Fri, 5 Feb 2021 07:29:00 GMT, Wang Huang <[hidden email]> wrote:

>> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
>> @Benchmark
>> public void testMethod(Blackhole bh) {
>>   int sum = 0;
>>   for (int i = 0; i < data.length; i++) {
>>       Integer ii = Integer.valueOf(data[i]);
>>       if (i < data.length) {
>>           sum += ii.intValue();
>>       }
>>   }
>>   bh.consume(sum);
>> }
>> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
>> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>>
>> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
>> and deleting the use of box:
>>
>> There is no additional fail/error(s) of jtreg after this patch.
>
> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
>
>   refactor codes

src/hotspot/share/opto/callGenerator.cpp line 558:

> 556:
> 557: static void delay_box_in_uncommon_trap(CallNode* call, Node* resproj) {
> 558:   if (resproj != NULL && call->is_CallStaticJava() &&

IMHO, we should use nullptr here because hotspot now is using c++14.

src/hotspot/share/opto/callGenerator.cpp line 560:

> 558:   if (resproj != NULL && call->is_CallStaticJava() &&
> 559:       call->as_CallStaticJava()->is_boxing_method()) {
> 560:     GraphKit kit(call->jvms());

you can postpone to construct this object in if(no_use).

src/hotspot/share/opto/callGenerator.cpp line 586:

> 584:         ciInstanceKlass* klass = call->as_CallStaticJava()->method()->holder();
> 585:         int n_fields = klass->nof_nonstatic_fields();
> 586:         assert(n_fields == 1, "sanity");

I think you  also need to check the only non-static field of klass must be a scalar.
"sanity" is too concise. I think we should leave a message to say it's an auto-boxing class.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Xin Liu
In reply to this post by Vladimir Kozlov-2
On Mon, 8 Feb 2021 18:15:54 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   refactor codes
>
> src/hotspot/share/opto/callGenerator.cpp line 582:
>
>> 580:         Node* uncommon_trap_node = delay_boxes.pop();
>> 581:         int in_edge = uncommon_trap_node->find_edge(res);
>> 582:         assert(in_edge > 0, "sanity");
>
> If there are several references you need to replace all of them.

+1
scalar replacement uses range substitution
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/macro.cpp#L892

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Wang Huang
In reply to this post by Xin Liu
On Tue, 9 Feb 2021 00:35:08 GMT, Xin Liu <[hidden email]> wrote:

>> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   refactor codes
>
> src/hotspot/share/opto/callGenerator.cpp line 558:
>
>> 556:
>> 557: static void delay_box_in_uncommon_trap(CallNode* call, Node* resproj) {
>> 558:   if (resproj != NULL && call->is_CallStaticJava() &&
>
> IMHO, we should use nullptr here because hotspot now is using c++14.

Thank you for your review. I will change that.

> src/hotspot/share/opto/callGenerator.cpp line 560:
>
>> 558:   if (resproj != NULL && call->is_CallStaticJava() &&
>> 559:       call->as_CallStaticJava()->is_boxing_method()) {
>> 560:     GraphKit kit(call->jvms());
>
> you can postpone to construct this object in if(no_use).

Sure. I will change that.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v2]

Wang Huang
In reply to this post by Xin Liu
On Tue, 9 Feb 2021 00:52:31 GMT, Xin Liu <[hidden email]> wrote:

>> src/hotspot/share/opto/callGenerator.cpp line 582:
>>
>>> 580:         Node* uncommon_trap_node = delay_boxes.pop();
>>> 581:         int in_edge = uncommon_trap_node->find_edge(res);
>>> 582:         assert(in_edge > 0, "sanity");
>>
>> If there are several references you need to replace all of them.
>
> +1
> scalar replacement uses range substitution
> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/macro.cpp#L892

Thank you for your review. It's my fault. I will revise this in next commit.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v3]

Wang Huang
In reply to this post by Wang Huang
> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
> @Benchmark
> public void testMethod(Blackhole bh) {
>   int sum = 0;
>   for (int i = 0; i < data.length; i++) {
>       Integer ii = Integer.valueOf(data[i]);
>       if (i < data.length) {
>           sum += ii.intValue();
>       }
>   }
>   bh.consume(sum);
> }
> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>
> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
> and deleting the use of box:
>
> There is no additional fail/error(s) of jtreg after this patch.

Wang Huang has updated the pull request incrementally with one additional commit since the last revision:

  fix some bugs

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2401/files
  - new: https://git.openjdk.java.net/jdk/pull/2401/files/4dfee52a..4c62ec8d

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2401&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2401&range=01-02

  Stats: 20 lines in 1 file changed: 6 ins; 5 del; 9 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2401.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2401/head:pull/2401

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v4]

Wang Huang
In reply to this post by Wang Huang
> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
> @Benchmark
> public void testMethod(Blackhole bh) {
>   int sum = 0;
>   for (int i = 0; i < data.length; i++) {
>       Integer ii = Integer.valueOf(data[i]);
>       if (i < data.length) {
>           sum += ii.intValue();
>       }
>   }
>   bh.consume(sum);
> }
> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>
> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
> and deleting the use of box:
>
> There is no additional fail/error(s) of jtreg after this patch.

Wang Huang has updated the pull request incrementally with one additional commit since the last revision:

  delete useless line

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2401/files
  - new: https://git.openjdk.java.net/jdk/pull/2401/files/4c62ec8d..e80e4959

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2401&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2401&range=02-03

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2401.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2401/head:pull/2401

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v4]

Vladimir Ivanov-2
On Wed, 10 Feb 2021 06:58:57 GMT, Wang Huang <[hidden email]> wrote:

>> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
>> @Benchmark
>> public void testMethod(Blackhole bh) {
>>   int sum = 0;
>>   for (int i = 0; i < data.length; i++) {
>>       Integer ii = Integer.valueOf(data[i]);
>>       if (i < data.length) {
>>           sum += ii.intValue();
>>       }
>>   }
>>   bh.consume(sum);
>> }
>> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
>> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>>
>> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
>> and deleting the use of box:
>>
>> There is no additional fail/error(s) of jtreg after this patch.
>
> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
>
>   delete useless line

The improvement you are proposing is not specific to uncommon traps, but can be generalized to any debug usage at safepoints.

The downside is that, in general, rematerialization logic has to use the corresponding pure function in order to materialize the eliminated instance. In this particular case (primitive boxing), it has to take into account the caching effects of primitive box factories. Otherwise, user code can encounter identity paradoxes with rematerialized primitive box instances.

I don't see how the scalarization logic you propose preserves identity constraints imposed by `valueOf` factories.

-------------

Changes requested by vlivanov (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v4]

Xin Liu
In reply to this post by Wang Huang
On Wed, 10 Feb 2021 06:58:57 GMT, Wang Huang <[hidden email]> wrote:

>> JDK-8075052 has  removed useless autobox. However, in some cases, the box is still saved. For instance:
>> @Benchmark
>> public void testMethod(Blackhole bh) {
>>   int sum = 0;
>>   for (int i = 0; i < data.length; i++) {
>>       Integer ii = Integer.valueOf(data[i]);
>>       if (i < data.length) {
>>           sum += ii.intValue();
>>       }
>>   }
>>   bh.consume(sum);
>> }
>> Although the variable ii is only used at ii.intValue(), it cannot be eliminated as a result of being used by a hidden uncommon_trap.
>> The uncommon_trap is generated by the optimized "if", because its condition is always true.
>>
>> We can postpone box in uncommon_trap in this situation. We treat box as a scalarized object by adding a SafePointScalarObjectNode in the uncommon_trap node,
>> and deleting the use of box:
>>
>> There is no additional fail/error(s) of jtreg after this patch.
>
> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
>
>   delete useless line

src/hotspot/share/opto/callGenerator.cpp line 588:

> 586:         Node* sobj = new SafePointScalarObjectNode(gvn.type(res)->isa_oopptr(),
> 587: #ifdef ASSERT
> 588:                                                   (AllocateNode*)call,

You can use call->isa_Allocate();  It utilizes node's ad-hoc RTTI  to do type casting.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v4]

Vladimir Kozlov-2
In reply to this post by Vladimir Ivanov-2
On Wed, 10 Feb 2021 21:26:17 GMT, Vladimir Ivanov <[hidden email]> wrote:

> The improvement you are proposing is not specific to uncommon traps, but can be generalized to any debug usage at safepoints.
>
> The downside is that, in general, rematerialization logic has to use the corresponding pure function in order to materialize the eliminated instance. In this particular case (primitive boxing), it has to take into account the caching effects of primitive box factories. Otherwise, user code can encounter identity paradoxes with rematerialized primitive box instances.
>
> I don't see how the scalarization logic you propose preserves identity constraints imposed by `valueOf` factories.

Yes, it seems this optimization introduces the issue we had with Graal (8223320):
"C2 doesn't model Integer.valueOf() as anything special. It just inlines it. So the check that determines whether to allocate a new Integer or take one from the cache always happens at runtime. Graal models it as a BoxNode. It is correctly lowered, however, if it needs to be present in a JVM state, it is described as an allocation. So the decision whether to allocate or take the cached value has to happen during the deopt."
There is code in deoptimizer for JVMCI which looks for cached Boxed values. We may need to adopt it for C2 EA for this optimization to work.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8261137: Optimization of Box nodes in uncommon_trap [v4]

Igor Veresov-3
On Wed, 10 Feb 2021 23:18:26 GMT, Vladimir Kozlov <[hidden email]> wrote:

> > The improvement you are proposing is not specific to uncommon traps, but can be generalized to any debug usage at safepoints.
> > The downside is that, in general, rematerialization logic has to use the corresponding pure function in order to materialize the eliminated instance. In this particular case (primitive boxing), it has to take into account the caching effects of primitive box factories. Otherwise, user code can encounter identity paradoxes with rematerialized primitive box instances.
> > I don't see how the scalarization logic you propose preserves identity constraints imposed by `valueOf` factories.
>
> Yes, it seems this optimization introduces the issue we had with Graal (8223320):
> "C2 doesn't model Integer.valueOf() as anything special. It just inlines it. So the check that determines whether to allocate a new Integer or take one from the cache always happens at runtime. Graal models it as a BoxNode. It is correctly lowered, however, if it needs to be present in a JVM state, it is described as an allocation. So the decision whether to allocate or take the cached value has to happen during the deopt."
> There is code in deoptimizer for JVMCI which looks for cached Boxed values. We may need to adopt it for C2 EA for this optimization to work.

In addition to using the logic that we already have there for Graal (see ```Deoptimization::get_cached_box()```), you need to track where the box came from. If it comes as a result of ```valueOf()``` then it has to come from the caches, if it's something that the user allocates with ```new Integer(x)```, then it should be just a normal scalarized object.

-------------

PR: https://git.openjdk.java.net/jdk/pull/2401
12