RFR: 8333893: Optimization for StringBuilder append boolean & null [v13]

[Emanuel Peter]

On Sun, 23 Jun 2024 08:47:25 GMT, Shaojin Wen <duke at openjdk.org> wrote:

>> After PR https://github.com/openjdk/jdk/pull/16245, C2 optimizes stores into primitive arrays by combining values ​​into larger stores.
>> 
>> This PR rewrites the code of appendNull and append(boolean) methods so that these two methods can be optimized by C2.
>
> Shaojin Wen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   private static final field `UNSAFE`

I'm running your benchmark [PutCharTest.java](https://github.com/user-attachments/files/15974672/PutCharTest.txt) with:

/oracle-work/jdk-fork2/build/linux-x64-slowdebug/jdk/bin/java --add-modules java.base --add-exports java.base/jdk.internal.misc=ALL-UNNAMED --add-exports java.base/jdk.internal.util=ALL-UNNAMED -XX:+TraceMergeStores -Xbatch -XX:CompileCommand=printcompilation,PutCharTest::* -XX:CompileCommand=compileonly,PutCharTest::put* -XX:+PrintIdeal PutCharTest.java


Aha, I found the limitation in `MergeStores`:
https://github.com/openjdk/jdk/blob/f7862bd6b9994814c6dfd43d471122408601f288/src/hotspot/share/opto/memnode.cpp#L2982-L2986

Basically, I require the `store` to have the same data-size as the element-size of the array.
But the `putChar` ends up as a 2-byte `StoreC`, and the array is a `byte[]` with 1-byte elements.

I quickly removed this check:

diff --git a/src/hotspot/share/opto/memnode.cpp b/src/hotspot/share/opto/memnode.cpp
index d0b6c59637f..78efda2db13 100644
--- a/src/hotspot/share/opto/memnode.cpp
+++ b/src/hotspot/share/opto/memnode.cpp
@@ -2980,10 +2980,10 @@ StoreNode* MergePrimitiveArrayStores::run() {
     return nullptr;
   }
   BasicType bt = aryptr_t->elem()->array_element_basic_type();
-  if (!is_java_primitive(bt) ||
-      type2aelembytes(bt) != _store->memory_size()) {
-    return nullptr;
-  }
+  //if (!is_java_primitive(bt) ||
+  //    type2aelembytes(bt) != _store->memory_size()) {
+  //  return nullptr;
+  //}
 
   // The _store must be the "last" store in a chain. If we find a use we could merge with
   // then that use or a store further down is the "last" store.
@@ -3033,13 +3033,13 @@ bool MergePrimitiveArrayStores::is_compatible_store(const StoreNode* other_store
   if (!is_java_primitive(aryptr_bt1) || !is_java_primitive(aryptr_bt2)) {
     return false;
   }
-  int size1 = type2aelembytes(aryptr_bt1);
-  int size2 = type2aelembytes(aryptr_bt2);
-  if (size1 != size2 ||
-      size1 != _store->memory_size() ||
-      _store->memory_size() != other_store->memory_size()) {
-    return false;
-  }
+  //int size1 = type2aelembytes(aryptr_bt1);
+  //int size2 = type2aelembytes(aryptr_bt2);
+  //if (size1 != size2 ||
+  //    size1 != _store->memory_size() ||
+  //    _store->memory_size() != other_store->memory_size()) {
+  //  return false;
+  //}
   return true;
 }
 


And now it optimizes:


15523  103    b  4       PutCharTest::putNull (14 bytes)
[TraceMergeStores]: Replace
  74  StoreC  === 62 7 73 22  [[ 99 ]]  @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched unsafe  Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms: PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:4 (line 7) PutCharTest::putNull @ bci:10 (line 23)
  99  StoreC  === 62 74 98 23  [[ 124 ]]  @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched unsafe  Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms: PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:13 (line 8) PutCharTest::putNull @ bci:10 (line 23)
 124  StoreC  === 62 99 123 24  [[ 150 ]]  @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched unsafe  Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms: PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:23 (line 9) PutCharTest::putNull @ bci:10 (line 23)
 150  StoreC  === 62 124 149 24  [[ 17 ]]  @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; mismatched unsafe  Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; !jvms: PutCharTest::putChar @ bci:14 (line 14) PutCharTest::putCharsAt @ bci:33 (line 10) PutCharTest::putNull @ bci:10 (line 23)
[TraceMergeStores]: with
 155  ConL  === 0  [[ 156 ]]  #long:30399761348886638
 156  StoreL  === 62 7 73 155  [[ ]]  @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4; mismatched  Memory: @byte[int:>=0] (java/lang/Cloneable,java/io/Serializable):NotNull:exact+any *, idx=4;


I will file an RFE to enable this use-case as well. @wenshao thanks for the standalone test, that was really helpful!

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

PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2189509983