On Thu, 13 Jun 2024 07:36:34 GMT, Emanuel Peter <epe...@openjdk.org> wrote:
>> Thanks to @eme64 's patience and help, I found a way to use MergeStore
>> without doing boundary checking.
>>
>> It would be even better if Unsafe.putChar could be used for MergeStore in
>> the future.
>>
>> ## 1. JavaCode
>>
>> class StringUTF16 {
>> public static void putCharsAt(byte[] value, int i, char c1, char c2,
>> char c3, char c4) {
>> // Don't use the putChar method, Its instrinsic will cause C2 unable
>> to combining values into larger stores.
>> putChar1(value, i , c1);
>> putChar1(value, i + 1, c2);
>> putChar1(value, i + 2, c3);
>> putChar1(value, i + 3, c4);
>> }
>>
>> public static void putCharsAt(byte[] value, int i, char c1, char c2,
>> char c3, char c4, char c5) {
>> // Don't use the putChar method, Its instrinsic will cause C2 unable
>> to combining values into larger stores.
>> putChar1(value, i , c1);
>> putChar1(value, i + 1, c2);
>> putChar1(value, i + 2, c3);
>> putChar1(value, i + 3, c4);
>> putChar1(value, i + 4, c5);
>> }
>>
>> static void putChar1(byte[] value, int i, char c) {
>> int address = Unsafe.ARRAY_BYTE_BASE_OFFSET + (i << 1);
>> Unsafe.getUnsafe().putByte(value, address , (byte)(c >>
>> HI_BYTE_SHIFT));
>> Unsafe.getUnsafe().putByte(value, address + 1, (byte)(c >>
>> LO_BYTE_SHIFT));
>> }
>> }
>>
>>
>> ## 2. Benchmark Numbers
>> The performance numbers under MacBookPro M1 Max are as follows:
>>
>> -Benchmark Mode Cnt Score Error Units
>> -StringBuilders.toStringCharWithNull8Latin1 avgt 15 7.073 ? 0.010 ns/op
>> -StringBuilders.toStringCharWithNull8Utf16 avgt 15 9.298 ? 0.015 ns/op
>> -StringBuilders.toStringCharWithBool8Latin1 avgt 15 7.387 ? 0.021 ns/op
>> -StringBuilders.toStringCharWithBool8Utf16 avgt 15 9.615 ? 0.011 ns/op
>>
>> +Benchmark Mode Cnt Score Error Units
>> +StringBuilders.toStringCharWithNull8Latin1 avgt 15 5.628 ? 0.017 ns/op
>> +25.67%
>> +StringBuilders.toStringCharWithNull8Utf16 avgt 15 6.873 ? 0.026 ns/op
>> +35.28%
>> +StringBuilders.toStringCharWithBool8Latin1 avgt 15 6.480 ? 0.077 ns/op
>> +13.99%
>> +StringBuilders.toStringCharWithBool8Utf16 avgt 15 7.456 ? 0.059 ns/op
>> +28.95%
>>
>>
>> ## 3. TraceMergeStores
>>
>> [TraceMergeStores]: Replace
>> 65 StoreB === 54 7 64 12 [[ 87 ]] @byte[int:>=0]
>> (java/lang/Cloneable,java/io/Serializable):exact+any *, idx=4; unsafe
>> Memory: @byte[int:>=0] (java/la...
>
> @wenshao I'm glad we figured it out a bit, and I hope you learned a thing or
> two :)
>
>> It would be even better if Unsafe.putChar could be used for MergeStore in
>> the future.
>
> The `_putCharStringU` intrinsic uses
> `LibraryCallKit::inline_string_char_access`. It seems to generate IR nodes, I
> speculate it could be `StoreC`. We'd have to do more digging to see why that
> pattern is not accepted by `MergeStore`.
>
> @wenshao I'm not sure if everything is right with the bounds checking, but I
> leave this to the library folks to review. What you will definately need is
> benchmarking not just on `M1`, but also `x86-64` and other `aarch64`
> architectures.
I'm not opposed to accepting this patch as-is, but I think we should do so with
an eye towards reverting if we figure out a way to improve the `putChar`
intrinsic so that it doesn't block merge store optimization. What do you think
@eme64?
As the need for the changes in
[b5ad8e7](https://github.com/openjdk/jdk/pull/19626/commits/b5ad8e70928c547d134d0e4a532441cad9a7e4a2)
showed added code complexity (like here) can be detrimental to performance,
and if the `putChar` can be improved we might see benefits in more places.
-------------
PR Comment: https://git.openjdk.org/jdk/pull/19626#issuecomment-2180308686
