On Thu, 11 Apr 2024 21:47:01 GMT, Scott Gibbons <sgibb...@openjdk.org> wrote:
>> This code makes an intrinsic stub for `Unsafe::setMemory` for x86_64. See >> [this PR](https://github.com/openjdk/jdk/pull/16760) for discussion around >> this change. >> >> Overall, making this an intrinsic improves overall performance of >> `Unsafe::setMemory` by up to 4x for all buffer sizes. >> >> Tested with tier-1 (and full CI). I've added a table of the before and >> after numbers for the JMH I ran (`MemorySegmentZeroUnsafe`). >> >> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt) > > 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? 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. 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? 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. 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. 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. 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. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561853120 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561831702 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561820596 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561822279 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561822556 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561823861 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561828829
