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

[Vladimir Ivanov]

On Mon, 15 Mar 2021 01:40:33 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:
> 
>   refactor some codes

Overall, looks good.

Some refactoring suggestions.

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

> 671:   if (is_pure_call()) {
> 672:     if (is_boxing_call() && callprojs.resproj != nullptr) {
> 673:         replace_box_to_scalar(call, callprojs.resproj);

I suggest to split it into 2 parts: 
  * `bool has_non_debug_usages(Node* n)` check which ensures there are only debug usages present
  *  `scalarize_debug_usages(...)` which iterates over all usages assuming they are debug and scalarizes them.

After that you can just keep `result_not_used` as is. After all debug usages are gone, the call becomes clearly not used. And IMO `result_not_used` communicates the intention better than `call_can_eliminate`.

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

> 801:  public:
> 802:   LateInlineBoxingCallGenerator(ciMethod* method, CallGenerator* inline_cg) :
> 803:     LateInlineCallGenerator(method, inline_cg, /*is_pure=*/true, /*is_boxing*/true) {}

You can replace `_is_boxing_call` with `is_boxing_late_inline()`. I find it clearer.

src/hotspot/share/opto/callGenerator.cpp
@@ -813,6 +795,8 @@ class LateInlineBoxingCallGenerator : public LateInlineCallGenerator {
     return new_jvms;
   }
 
+  virtual bool is_boxing_late_inline() const { return true; }
+
   virtual CallGenerator* with_call_node(CallNode* call) {
     LateInlineBoxingCallGenerator* cg = new LateInlineBoxingCallGenerator(method(), _inline_cg);
     cg->set_call_node(call->as_CallStaticJava());

src/hotspot/share/opto/callGenerator.hpp
@@ -75,6 +75,7 @@ class CallGenerator : public ResourceObj {
   virtual bool      is_late_inline() const         { return false; }
   // same but for method handle calls
   virtual bool      is_mh_late_inline() const      { return false; }
+  virtual bool      is_boxing_late_inline() const  { return false; }
   virtual bool      is_string_late_inline() const  { return false; }
   virtual bool      is_virtual_late_inline() const { return false; }

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

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