On Thu, 11 Apr 2024 23:30:07 GMT, Sandhya Viswanathan
<sviswanat...@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
