RFR: 8261137: Optimization of Box nodes in uncommon_trap [v5]

[Tobias Hartmann]

On Tue, 23 Feb 2021 07:44:58 GMT, Wang Huang <whuang at openjdk.org> 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 adjust my codes and add a new benchmark
>> 
>> 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]);
>>           black();
>>           if (i < 100000) {
>>               sum += ii.intValue();
>>           }
>>       }
>>       bh.consume(sum);
>>     }
>> 
>>     public void black(){}
>> }
>> 
>> 
>> aarch64:  
>> base line：
>> Benchmark                     Mode  Samples   Score  Score error  Units
>> o.s.MyBenchmark.testMethod    avgt       30  88.513        1.111  us/op
>> 
>> opt：
>> Benchmark                     Mode  Samples   Score  Score error  Units
>> o.s.MyBenchmark.testMethod    avgt       30  52.776        0.096  us/op
>> 
>> x86:  
>> base line：
>> Benchmark                     Mode  Samples   Score  Score error  Units
>> o.s.MyBenchmark.testMethod    avgt       30  81.066        3.156  us/op
>> 
>> opt:
>> Benchmark                     Mode  Samples   Score  Score error  Units
>> o.s.MyBenchmark.testMethod    avgt       30  55.596        0.775  us/op
>
> Wang Huang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   add debuginfo optimization

I agree with Vladimir that tests are needed. I've added some comments to the code.

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

> 559:   if (resproj != nullptr && call->is_CallStaticJava() &&
> 560:       call->as_CallStaticJava()->is_boxing_method()) {
> 561:     Unique_Node_List debuginfo_node_list;

Maybe rename this to `safepoints`.

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

> 567:         for (uint i = 0; i < dbg_start; i++) {
> 568:           if (sfpt->in(i) == resproj) {
> 569:             return;

I think this code can be replaced by:
  if (!sfpt->is_Call() || !sfpt->as_Call()->has_non_debug_use(n)) {
    safepoints.push(sfpt);
  } else {
    ...
  }

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

> 585:       ciInstanceKlass* klass = call->as_CallStaticJava()->method()->holder();
> 586:       int n_fields = klass->nof_nonstatic_fields();
> 587:       assert(n_fields == 1, "the klass must be an auto-boxing klass");

This code can be put in `ifdef ASSERT` and `n_fields` below can be replaced by 1.

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

> 654:   }
> 655: 
> 656:   replace_box_to_scalar(call, callprojs.resproj);

Should this be guarded by `C->eliminate_boxing()`?

src/hotspot/share/opto/callnode.hpp line 503:

> 501:                      // It is relative to the last (youngest) jvms->_scloff.
> 502:   uint _n_fields;    // Number of non-static fields of the scalarized object.
> 503:   bool _is_auto_box; // is the scalarized object is auto box.

Typo in comment. Should be something like `// True if the scalarized object is an auto box`

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

> 581:     while (debuginfo_node_list.size() > 0) {
> 582:       ProjNode* res = resproj->as_Proj();
> 583:       Node* debuginfo_node = debuginfo_node_list.pop();

`debuginfo_node` -> `safepoint`

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

> 594:                                                   first_ind, n_fields, true);
> 595:       sobj->init_req(0, kit.root());
> 596:       debuginfo_node->add_req(call->in(res->_con));

I don't understand why you are selecting the input based on the result projection field `res->_con`?

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

Changes requested by thartmann (Reviewer).

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