Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v11]
On Thu, 11 Apr 2024 22:00:01 GMT, Sandhya Viswanathan wrote: >> No. Reviewing the code I saw this as a potential error, as >> `arraycopy_avx3_large` could cause a SIGBUS which wouldn't be caught. It >> conforms to the other instances of copy in the code. I think it was missed >> by the original developer. > > Would be good to do it in a separate PR then. Removed. - PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561866645
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v11]
On Thu, 11 Apr 2024 20:58:00 GMT, Scott Gibbons wrote:
>> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 735:
>>
>>> 733:
>>> 734: if (MaxVectorSize == 64) {
>>> 735: UnsafeCopyMemoryMark ucmm(this, !is_oop && !aligned, false,
>>> ucme_exit_pc);
>>
>> This is not related to Unsafe::setMemory?
>
> No. Reviewing the code I saw this as a potential error, as
> `arraycopy_avx3_large` could cause a SIGBUS which wouldn't be caught. It
> conforms to the other instances of copy in the code. I think it was missed
> by the original developer.
Would be good to do it in a separate PR then.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561801825
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v11]
On Thu, 11 Apr 2024 20:08:18 GMT, Sandhya Viswanathan
wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix whitespace error.
>
> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8343:
>
>> 8341: UnsafeCopyMemory::create_table(8);
>> 8342: }
>> 8343:
>
> Did you mean to initialize UnsafeSetMemory::_table here instead?
Yes. Good catch.
> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 155:
>
>> 153: StubRoutines::_arrayof_jint_fill = generate_fill(T_INT, true,
>> "arrayof_jint_fill");
>> 154:
>> 155: // #ifdef _LP64
>
> We could remove the #ifdef _LP64, #endif commented pair.
Done.
> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 735:
>
>> 733:
>> 734: if (MaxVectorSize == 64) {
>> 735: UnsafeCopyMemoryMark ucmm(this, !is_oop && !aligned, false,
>> ucme_exit_pc);
>
> This is not related to Unsafe::setMemory?
No. Reviewing the code I saw this as a potential error, as
`arraycopy_avx3_large` could cause a SIGBUS which wouldn't be caught. It
conforms to the other instances of copy in the code. I think it was missed by
the original developer.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561687577
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561688018
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561695561
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v11]
On Thu, 11 Apr 2024 18:42:56 GMT, Scott Gibbons 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:
>
> Fix whitespace error.
src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 8343:
> 8341: UnsafeCopyMemory::create_table(8);
> 8342: }
> 8343:
Did you mean to initialize UnsafeSetMemory::_table here instead?
src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 155:
> 153: StubRoutines::_arrayof_jint_fill = generate_fill(T_INT, true,
> "arrayof_jint_fill");
> 154:
> 155: // #ifdef _LP64
We could remove the #ifdef _LP64, #endif commented pair.
src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 735:
> 733:
> 734: if (MaxVectorSize == 64) {
> 735: UnsafeCopyMemoryMark ucmm(this, !is_oop && !aligned, false,
> ucme_exit_pc);
This is not related to Unsafe::setMemory?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561587296
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561606554
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561620598
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v11]
> 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: Fix whitespace error. - Changes: - all: https://git.openjdk.org/jdk/pull/18555/files - new: https://git.openjdk.org/jdk/pull/18555/files/41ffcc32..b99499a9 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18555=10 - incr: https://webrevs.openjdk.org/?repo=jdk=18555=09-10 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18555.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18555/head:pull/18555 PR: https://git.openjdk.org/jdk/pull/18555
