RFR: 8329331: Intrinsify Unsafe::setMemory [v12]

[Scott Gibbons]

On Thu, 11 Apr 2024 23:30:07 GMT, Sandhya Viswanathan <sviswanathan at openjdk.org> wrote:

>> Scott Gibbons has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Addressing more review comments
>
> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2751:
> 
>> 2749:         UnsafeSetMemoryMark usmm(this, true, true);
>> 2750: 
>> 2751:         __ generate_fill(T_BYTE, false, c_rarg0, c_rarg1, r11, rax, xmm0);
> 
> We will be duplicating the code gen for generate_fill here? Could we not do a tail call to _jbyte_fill here and add UnsafeSetMemoryMark inside _jbyte_fill?

It would not be appropriate to add set memory marks to the existing _jbyte_fill as it is being used by other routines, and the effect of the mark will be very hard to track down (if any).

Are you *sure* we want to do that?

> src/hotspot/share/opto/library_call.cpp line 4952:
> 
>> 4950: }
>> 4951: 
>> 4952: bool LibraryCallKit::inline_unsafe_setMemory() {
> 
> It will be good to add the signature of Unsafe.setMemory0 as a comment above line 4952.

Done

> src/hotspot/share/opto/runtime.cpp line 783:
> 
>> 781:   fields[argp++] = TypeLong::LONG;      // size
>> 782:   fields[argp++] = Type::HALF;          // size
>> 783:   fields[argp++] = TypeInt::INT;        // bytevalue
> 
> Should this be TypeInt::BYTE?

Should be TypeInt::UBYTE.  Changed.

> src/hotspot/share/runtime/sharedRuntime.cpp line 181:
> 
>> 179: 
>> 180: uint SharedRuntime::_unsafe_set_memory_ctr=0;
>> 181: 
> 
> Extra blank line before line 180 could be removed.

Done

> src/hotspot/share/runtime/sharedRuntime.cpp line 1994:
> 
>> 1992:   if (_rethrow_ctr) tty->print_cr("%5u rethrow handler", _rethrow_ctr);
>> 1993: 
>> 1994:   if (_unsafe_set_memory_ctr) tty->print_cr("%5u unsafe set memorys", _unsafe_set_memory_ctr);
> 
> Extra blank line before line 1994 could be removed.

Done.

> src/hotspot/share/runtime/sharedRuntime.hpp line 546:
> 
>> 544: 
>> 545:   static uint _unsafe_set_memory_ctr;      // Slow-path includes alignment checks
>> 546: 
> 
> Extra blank line before line 545 could be removed.

Done.

> test/jdk/sun/misc/CopyMemory.java line 214:
> 
>> 212:         random.setSeed(seed);
>> 213:         System.out.println("Seed set to "+ seed);
>> 214: 
> 
> Looks like these lines were added for debugging, could be removed.

Yes, but I believe we should adopt this for the future since reproducing random test failures is extremely difficult without knowing the seed of the RNG.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561867359
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561867575
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561867857
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561867923
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561868084
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561868158
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561868630