Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]
On Wed, 10 Apr 2024 16:17:32 GMT, Jaikiran Pai wrote:
>> Can I please get a review of this change which proposes to fix the issue
>> noted in https://bugs.openjdk.org/browse/JDK-8212895?
>>
>> As noted in that issue, the `ChronoField.INSTANT_SECONDS` currently is
>> initialized to have a minimum and maximum values of `Long.MIN_VALUE` and
>> `LONG.MAX_VALUE` respectively. However, `java.time.Instant` only supports
>> `-31557014167219200L` and `31556889864403199L` as minimum and maximum values
>> for the epoch second.
>>
>> The commit in this PR updates the `ChronoField.INSTANT_SECONDS`'s value
>> range to match the supported min and max values of `Instant` (as suggested
>> by Stephen in that JBS issue). This commit also introduces a test to verify
>> this change. This new test method as well as existing tests in tier1, tier2
>> and tier3 continue to pass with this change.
>
> Jaikiran Pai has updated the pull request incrementally with one additional
> commit since the last revision:
>
> no need for {@code} in javadoc
I've now created the CSR https://bugs.openjdk.org/browse/JDK-8330135. I'll move
it to Finalized once I get a review for it.
-
PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2050930262
Re: RFR: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases
On Tue, 9 Apr 2024 12:01:49 GMT, Claes Redestad wrote:
> This patch suggests a workaround to an issue with huge SCF MH expression
> trees taking excessive JIT compilation resources by reviving (part of) the
> simple bytecode-generating strategy that was originally available as an
> all-or-nothing strategy choice.
>
> Instead of reintroducing a binary strategy choice I propose a threshold
> parameter, controlled by
> `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions
> below or at this threshold there's no change, for expressions with an arity
> above it we use the `StringBuilder`-chain bytecode generator.
>
> There are a few trade-offs at play here which influence the choice of
> threshold. The simple high arity strategy will for example not see any reuse
> of LambdaForms but strictly always generate a class per indy callsite, which
> means we might end up with a higher total number of classes generated and
> loaded in applications if we set this value too low. It may also produce
> worse performance on average. On the other hand there is the observed
> increase in C2 resource usage as expressions grow unwieldy. On the other
> other hand high arity expressions are likely rare to begin with, with less
> opportunities for sharing than the more common low-arity expressions.
>
> I turned the submitted test case into a few JMH benchmarks and did some
> experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`:
>
> Baseline strategy:
> 13 args: 6.3M
> 23 args: 18M
> 123 args: 868M
>
> `-Djava.lang.invoke.StringConcat.highArityThreshold=0`:
> 13 args: 2.11M
> 23 args: 3.67M
> 123 args: 4.75M
>
> For 123 args the memory overhead of the baseline strategy is 180x, but for 23
> args we're down to a 5x memory overhead, and down to a 3x overhead for 13
> args. Since the absolute overhead for 23 is borderline acceptable (+15Mb)
> I've conservatively chosen a threshold at arity 20. This keeps C2 resource
> pressure at a reasonable level (< 18M) while avoiding perturbing performance
> at the vast majority of call sites.
>
> I was asked to use the new class file API for mainline. There's a version of
> this patch implemented using ASM in 7c52a9f which might be a reasonable basis
> for a backport.
src/java.base/share/classes/java/lang/invoke/StringConcatFactory.java line 1430:
> 1428: cb.new_(STRING_BUILDER);
> 1429: cb.dup();
> 1430: cb.invokespecial(STRING_BUILDER, "",
> MethodTypeDesc.ofDescriptor("()V"));
Would there be value in initializing to a larger capacity? Given the number of
append calls, seems the default cap of 16 is unlikely to be sufficient.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18690#discussion_r1561966067
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v13]
On Fri, 12 Apr 2024 00:10:22 GMT, Sandhya Viswanathan wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Addressing yet more review comments > > src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2521: > >> 2519: const Register byteVal = rdx; >> 2520: >> 2521: // Propagate byte to full Register > > The comment refers to lines 2524-2526, please move it down. Still continuing to look through StubGenerator::generate_unsafe_setmemory(), more comments to come. Thank you for your patience. - PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561878666
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v13]
On Fri, 12 Apr 2024 00:07: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: > > Addressing yet more review comments src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2504: > 2502: Label L_exit, L_fillQuadwords, L_fillDwords, L_fillBytes; > 2503: > 2504: setup_arg_regs(3); A comment stating the placement of dest, size, and byteVal after call to setup_arg_regs() would be very helpful. src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2521: > 2519: const Register byteVal = rdx; > 2520: > 2521: // Propagate byte to full Register The comment refers to lines 2524-2526, please move it down. - PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561873770 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561871872
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v12]
On Fri, 12 Apr 2024 00:00:38 GMT, Scott Gibbons wrote: >> 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? Yes we want to do that. It is all guarded by thread->doing_unsafe_access() which is only true when we are getting to this code from unsafe. Similar technique is used in copyMemory as well. - PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561877858
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v12]
On Fri, 12 Apr 2024 00:03:10 GMT, Scott Gibbons wrote:
>> 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.
Removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561868727
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v12]
On Thu, 11 Apr 2024 23:30:07 GMT, Sandhya Viswanathan
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
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 [v13]
> 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 yet more review comments - Changes: - all: https://git.openjdk.org/jdk/pull/18555/files - new: https://git.openjdk.org/jdk/pull/18555/files/89db3eb6..970c5751 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18555=12 - incr: https://webrevs.openjdk.org/?repo=jdk=18555=11-12 Stats: 21 lines in 6 files changed: 6 ins; 11 del; 4 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
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v12]
On Thu, 11 Apr 2024 21:47:01 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:
>
> 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
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: 8327247: C2 uses up to 2GB of RAM to compile complex string concat in extreme cases
On Tue, 9 Apr 2024 12:01:49 GMT, Claes Redestad wrote: > This patch suggests a workaround to an issue with huge SCF MH expression > trees taking excessive JIT compilation resources by reviving (part of) the > simple bytecode-generating strategy that was originally available as an > all-or-nothing strategy choice. > > Instead of reintroducing a binary strategy choice I propose a threshold > parameter, controlled by > `-Djava.lang.invoke.StringConcat.highArityThreshold=`: For expressions > below or at this threshold there's no change, for expressions with an arity > above it we use the `StringBuilder`-chain bytecode generator. > > There are a few trade-offs at play here which influence the choice of > threshold. The simple high arity strategy will for example not see any reuse > of LambdaForms but strictly always generate a class per indy callsite, which > means we might end up with a higher total number of classes generated and > loaded in applications if we set this value too low. It may also produce > worse performance on average. On the other hand there is the observed > increase in C2 resource usage as expressions grow unwieldy. On the other > other hand high arity expressions are likely rare to begin with, with less > opportunities for sharing than the more common low-arity expressions. > > I turned the submitted test case into a few JMH benchmarks and did some > experiments with `-XX:CompileCommand=MemStat,StringConcat::concat,print`: > > Baseline strategy: > 13 args: 6.3M > 23 args: 18M > 123 args: 868M > > `-Djava.lang.invoke.StringConcat.highArityThreshold=0`: > 13 args: 2.11M > 23 args: 3.67M > 123 args: 4.75M > > For 123 args the memory overhead of the baseline strategy is 180x, but for 23 > args we're down to a 5x memory overhead, and down to a 3x overhead for 13 > args. Since the absolute overhead for 23 is borderline acceptable (+15Mb) > I've conservatively chosen a threshold at arity 20. This keeps C2 resource > pressure at a reasonable level (< 18M) while avoiding perturbing performance > at the vast majority of call sites. > > I was asked to use the new class file API for mainline. There's a version of > this patch implemented using ASM in 7c52a9f which might be a reasonable basis > for a backport. Anyone out there? - PR Comment: https://git.openjdk.org/jdk/pull/18690#issuecomment-2050707573
Re: RFR: 8327640: Allow NumberFormat strict parsing [v7]
On Thu, 11 Apr 2024 20:55:11 GMT, Justin Lu wrote:
>> Please review this PR and associated
>> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
>> parsing for NumberFormat.
>>
>> The concrete subclasses that will utilize this leniency value are
>> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for
>> parsing to be used as an input validation technique for localized formatted
>> numbers. The default leniency mode will remain lenient, and can only be set
>> to strict through an intentional `setLenient(boolean)` method call. Leniency
>> operates differently based off the values returned by `isGroupingUsed()`,
>> `getGroupingSize()`, and `isParseIntegerOnly()`.
>>
>> Below is an example of the change, the CSR can be viewed for further detail.
>>
>>
>> DecimalFormat fmt = (DecimalFormat)
>> NumberFormat.getNumberInstance(Locale.US);
>> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
>> fmt.setLenient(false);
>> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
>> fmt.setGroupingSize(2);
>> fmt.parse("12,34,567"); // throws ParseException
>> fmt.setParseIntegerOnly(true)
>> fmt.parse("12,34.56"); // throws ParseException
>> fmt.parse("12,34"); // successfully returns the Number 1234
>>
>>
>> The associated tests are all localized, and the data is converted properly
>> during runtime.
>
> Justin Lu has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Reflect test suggestions
> - reflect impl suggestions
LGTM. Thanks for the changes.
-
Marked as reviewed by naoto (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18339#pullrequestreview-1995566425
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v12]
> 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 - Changes: - all: https://git.openjdk.org/jdk/pull/18555/files - new: https://git.openjdk.org/jdk/pull/18555/files/b99499a9..89db3eb6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18555=11 - incr: https://webrevs.openjdk.org/?repo=jdk=18555=10-11 Stats: 250 lines in 2 files changed: 85 ins; 97 del; 68 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
Integrated: 8330049: Remove unused AbstractLinker::linkerByteOrder
On Wed, 10 Apr 2024 15:38:22 GMT, Jorn Vernee wrote: > Please review this simple cleanup which removes the > `AbstractLinker::linkerByteOrder` method. It was only used in > `AixPPC64Linker`, where we know it will always return `ByteOrder.BIG_ENDIAN` > so we can just replace the call with that. > > Testing: Local run of `run-test-jdk_foreign`. This pull request has now been integrated. Changeset: 0db42906 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/0db42906e390a98b3a6be078f1b8c3f2a03a838f Stats: 69 lines in 12 files changed: 0 ins; 58 del; 11 mod 8330049: Remove unused AbstractLinker::linkerByteOrder Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/18726
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: 8327640: Allow NumberFormat strict parsing [v6]
On Thu, 11 Apr 2024 01:46:06 GMT, Naoto Sato wrote: >> Justin Lu has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 14 additional commits >> since the last revision: >> >> - Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing >> - improve wording consistency and see tags >> - Merge branch 'master' into JDK-8327640-NumberFormat-strictParsing >> - revert cleanup changes; (to go into a separate change) >> - use directed 'inheritDoc' tag >> - update example for lenient parsing >> - replace protected field for private fields in subclasses for consistency >> - drop Format implNote as well >> - setStrict and isStrict should be optional in NumberFormat >>- specify and throw UOE as default >>- override and implement in dFmt and cmpctNFmt >>parseStrict should be protected, and utilized by subclasses. The public >> methods should just >>serve as API. >> - Re-work specification wording from Format down to subclasses >> - ... and 4 more: https://git.openjdk.org/jdk/compare/f894b148...aa1c284b > > Looks good overall. Left some minor comments. > As to the tests, good to see those corner cases, but they should have unit > tests for the new methods, i.e, `isStrict()` and `setStrict()`. Also I think > equality/serialization for those methods should be examined. Thank you for the review @naotoj. All of your comments should be reflected in the two previous commits. As for the test suggestions, I added to existing tests if possible, otherwise I created new test files to address your suggestions. The new DecimalFormat equality and serialization test can definitely have other qualities tested (beyond parseStrict), but perhaps that's best for a separate issue. > src/java.base/share/classes/java/text/DecimalFormat.java line 43: > >> 41: import sun.util.locale.provider.LocaleProviderAdapter; >> 42: import sun.util.locale.provider.ResourceBundleBasedAdapter; >> 43: > > Internal packages typically follow public packages. Fixed. (IntelliJ is adamant on having it that way, and even after I previously reverted it, it seems like it slipped through again. Oddly, it never used to do this, need to look at the settings) - PR Comment: https://git.openjdk.org/jdk/pull/18339#issuecomment-2050518798 PR Review Comment: https://git.openjdk.org/jdk/pull/18339#discussion_r1561677534
Re: RFR: 8327640: Allow NumberFormat strict parsing [v7]
> Please review this PR and associated
> [CSR](https://bugs.openjdk.org/browse/JDK-8327703) which introduces strict
> parsing for NumberFormat.
>
> The concrete subclasses that will utilize this leniency value are
> `DecimalFormat` and `CompactNumberFormat`. Strict leniency allows for parsing
> to be used as an input validation technique for localized formatted numbers.
> The default leniency mode will remain lenient, and can only be set to strict
> through an intentional `setLenient(boolean)` method call. Leniency operates
> differently based off the values returned by `isGroupingUsed()`,
> `getGroupingSize()`, and `isParseIntegerOnly()`.
>
> Below is an example of the change, the CSR can be viewed for further detail.
>
>
> DecimalFormat fmt = (DecimalFormat) NumberFormat.getNumberInstance(Locale.US);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // returns 1000.23
> fmt.setLenient(false);
> fmt.parse("1,,,0,,,00,.23Zabced45"); // Now throws a ParseException
> fmt.setGroupingSize(2);
> fmt.parse("12,34,567"); // throws ParseException
> fmt.setParseIntegerOnly(true)
> fmt.parse("12,34.56"); // throws ParseException
> fmt.parse("12,34"); // successfully returns the Number 1234
>
>
> The associated tests are all localized, and the data is converted properly
> during runtime.
Justin Lu has updated the pull request incrementally with two additional
commits since the last revision:
- Reflect test suggestions
- reflect impl suggestions
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18339/files
- new: https://git.openjdk.org/jdk/pull/18339/files/aa1c284b..eec992e3
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18339=06
- incr: https://webrevs.openjdk.org/?repo=jdk=18339=05-06
Stats: 277 lines in 7 files changed: 260 ins; 6 del; 11 mod
Patch: https://git.openjdk.org/jdk/pull/18339.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18339/head:pull/18339
PR: https://git.openjdk.org/jdk/pull/18339
Re: RFR: 8303689: javac -Xlint could/should report on "dangling" doc comments [v2]
> Please review the updates to support a proposed new
> `-Xlint:dangling-doc-comments` option.
>
> The work can be thought of as in 3 parts:
>
> 1. An update to the `javac` internal class `DeferredLintHandler` so that it
> is possible to specify the appropriately configured `Lint` object when it is
> time to consider whether to generate the diagnostic or not.
>
> 2. Updates to the `javac` front end to record "dangling docs comments" found
> near the beginning of a declaration, and to report them using an instance of
> `DeferredLintHandler`. This allows the warnings to be enabled or disabled
> using the standard mechanisms for `-Xlint` and `@SuppressWarnings`. The
> procedure for handling dangling doc comments is described in this comment in
> `JavacParser`.
>
> * Dangling documentation comments are handled as follows.
> * 1. {@code Scanner} adds all doc comments to a queue of
> * recent doc comments. The queue is flushed whenever
> * it is known that the recent doc comments should be
> * ignored and should not cause any warnings.
> * 2. The primary documentation comment is the one obtained
> * from the first token of any declaration.
> * (using {@code token.getDocComment()}.
> * 3. At the end of the "signature" of the declaration
> * (that is, before any initialization or body for the
> * declaration) any other "recent" comments are saved
> * in a map using the primary comment as a key,
> * using this method, {@code saveDanglingComments}.
> * 4. When the tree node for the declaration is finally
> * available, and the primary comment, if any,
> * is "attached", (in {@link #attach}) any related
> * dangling comments are also attached to the tree node
> * by registering them using the {@link #deferredLintHandler}.
> * 5. (Later) Warnings may be genereated for the dangling
> * comments, subject to the {@code -Xlint} and
> * {@code @SuppressWarnings}.
>
>
> 3. Updates to the make files to disable the warnings in modules for which
> the
> warning is generated. This is often because of the confusing use of `/**` to
> create box or other standout comments.
Jonathan Gibbons has updated the pull request incrementally with one additional
commit since the last revision:
adjust call for `saveDanglingDocComments` for enum members
-
Changes:
- all: https://git.openjdk.org/jdk/pull/18527/files
- new: https://git.openjdk.org/jdk/pull/18527/files/3d6f1f95..56d6dcac
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk=18527=01
- incr: https://webrevs.openjdk.org/?repo=jdk=18527=00-01
Stats: 5 lines in 1 file changed: 3 ins; 2 del; 0 mod
Patch: https://git.openjdk.org/jdk/pull/18527.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/18527/head:pull/18527
PR: https://git.openjdk.org/jdk/pull/18527
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: 8329948: Remove string template feature [v5]
On Thu, 11 Apr 2024 10:15:54 GMT, Maurizio Cimadamore wrote: >> This PR removes support for the string template feature from the Java >> compiler and the Java SE API, as discussed here: >> >> https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Drop spurious jshell test change Carriers is used by pattern matching as well and should remain in runtime. - PR Comment: https://git.openjdk.org/jdk/pull/18688#issuecomment-2050304721
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
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v22]
On Thu, 11 Apr 2024 17:08:49 GMT, Suchismith Roy wrote: > > Thanks! This looks like a good idea. Only the directory handling needs some > > modification. This version tries to load > > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/scratch/0/native/libawt_headless.so", > > but it should load > > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/java/lang/RuntimeTests/loadLibrary/aix/AIXLoadLibraryDriver/native/libfoobar.a". > > > Thanks! This looks like a good idea. Only the directory handling needs some > > modification. This version tries to load > > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/scratch/0/native/libawt_headless.so", > > but it should load > > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/java/lang/RuntimeTests/loadLibrary/aix/AIXLoadLibraryDriver/native/libfoobar.a". > > I am facing the same error. But i do not understand why is it resolved to > libawt_headless.so . stdout: [attempting to load library foobar ]; stderr: [Exception in thread "main" java.lang.UnsatisfiedLinkError: Can't load library: /home/hotspot/openjdk/jdk-suchi/jdk1/build/aix-ppc64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix_LoadAIXLibraryFromArchiveObject_java/scratch/0/native/libawt_headless.so at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2406) at java.base/java.lang.Runtime.load0(Runtime.java:852) at java.base/java.lang.System.load(System.java:2030) at java.base/jdk.internal.loader.NativeLibraries.load(Native Method) at java.base/jdk.internal.loader.NativeLibraries$NativeLibraryImpl.open(NativeLibraries.java:331) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:197) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:139) at java.base/jdk.internal.loader.NativeLibraries.findFromPaths(NativeLibraries.java:259) at java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:251) at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:2435) at java.base/java.lang.Runtime.loadLibrary0(Runtime.java:916) at java.base/java.lang.System.loadLibrary(System.java:2068) at LoadAIXLibraryFromArchiveObject$LoadLibraryApp.main(LoadAIXLibraryFromArchiveObject.java:52) ] exitValue = 1 - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2050279516
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v9]
On Thu, 11 Apr 2024 18:17:01 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: > > Set memory test (#16) > > Add framework for other platforms. Moved fill_to_memory_atomic back to the > .cpp from the .hpp in order to get 32-bit fixed. I added the framework for setting unsafe access marks within all platforms, and fixed a bug with the Linux 32-bit runtime tests. Adding stub intrinsic for setMemory0 for other platforms should be easier now. Passes CI testing. - PR Comment: https://git.openjdk.org/jdk/pull/18555#issuecomment-2050267842
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v10]
> 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 with a new target base due to a merge or a rebase. The pull request now contains 19 commits: - Merge master - Set memory test (#16) Add framework for other platforms. Moved fill_to_memory_atomic back to the .cpp from the .hpp in order to get 32-bit fixed. - Address review comments (#15) * Address review comments - Add movq to locate_operand - Oops - Fixed generate_fill when count > 0x8000 - Fix Windows - Addressing review comments. - Remove dead code - Use non-sse fill (old left in) - ... and 9 more: https://git.openjdk.org/jdk/compare/31ee5108...41ffcc32 - Changes: https://git.openjdk.org/jdk/pull/18555/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18555=09 Stats: 734 lines in 38 files changed: 680 ins; 5 del; 49 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
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v9]
> 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: Set memory test (#16) Add framework for other platforms. Moved fill_to_memory_atomic back to the .cpp from the .hpp in order to get 32-bit fixed. - Changes: - all: https://git.openjdk.org/jdk/pull/18555/files - new: https://git.openjdk.org/jdk/pull/18555/files/b0ac8577..95230e29 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18555=08 - incr: https://webrevs.openjdk.org/?repo=jdk=18555=07-08 Stats: 179 lines in 14 files changed: 115 ins; 49 del; 15 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
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v22]
On Wed, 10 Apr 2024 16:46:30 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Files copy method
`String libraryName = "awt";
File awtSharedObjectPathCopy = new File("/test/lib/libawt.so");
File awtSharedObjectPath = new File("/test/lib/libawt.so");
File awtArchivePath = new File("/test/lib/libawt.a");
awtSharedObjectPath.renameTo(awtArchivePath);
if (awtSharedObjectPath.exists()) {
awtSharedObjectPath.renameTo(awtSharedObjectPathCopy);
throw new RuntimeException("LoadAIXLibraryFromArchiveObject: .so
should not exist.");
`
@jaikiran is this also right way to use the /test/lib directory ?
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2050149972
Re: RFR: 8329538: Accelerate P256 on x86_64 using Montgomery intrinsic [v2]
On Wed, 10 Apr 2024 18:02:38 GMT, Volodymyr Paprotski wrote: > > In `ECOperations.java`, if I understand this correctly, it is to replace > > the existing `PointMultiplier` with montgomery-based PointMuliplier. But > > when I look at the code, I see both are still options. If I read this > > correctly, it checks for the old `IntegerFieldModuloP`, then looks for the > > new `IntegerMontgomeryFieldModuloP`. It appears to use the new one always. > > Why doesn't it just replace the old implementation entry in the `fields` > > Map? Is there a reason to keep it around? > > Hmm, thats a good point I haven't fully considered; i.e. (if I read > correctly) "for `CurveDB.P_256` remove the fallback path to non-montgomery > entirely".. that might also help in cleaning a few things up in the > construction. Maybe even get rid of this nested ECOperations inside > ECOperations.. Perhaps nesting isnt a big deal, but all attempts to make the > ECC stack clearer is positive! > > One functional reason that might justify keeping it as-is, is fuzz-testing; > with the fallback available, I am able to write the included Fuzz tests and > have them check the values against the existing implementation. While I also > included a few KAT tests using openssl-generated values, the fuzz tests check > millions of values and it does add a lot more certainty about correctness of > this code. I hadn't looked at your fuzz test until you mentioned it. I see you are using reflection to change the values. Is that what you mean by "fallback"? I'm assuming there is no to access the older implementation without reflection. > > Can it be removed? For the operations that do not involve multiplication > (i.e. `setSum(*)`), montgomery is expensive. I think I did go through the > uses of this code some time back (i.e. ECDHE, ECDSA and KeyGeneration) and > existing IntegerPolynomialP256 is no longer used (I should verify that again) > and only P256OrderField remains non-montgomery. So removing references to > IntegerPolynomialP256 in ECOperations should be possible and cleaner. > Removing IntegerPolynomialP256 from MontgomeryIntegerPolynomialP256 is harder > (fromMontgomery() uses IntegerPolynomialP256) but perhaps also worth some > thought.. > > I tend to like `ECOperationsFuzzTest.java` and would prefer to keep it, but > it could also be chucked up as part of 'scaffolding' and removed in name of > code quality? I wouldn't rip out the old implementation. I have been wondering if we should make the older implementation available, maybe by security property. I was looking at the static Maps at the top of `ECOperations`, `forParameters`, and the constructors where it checks if the `montgomeryOps` was null or set. It would be nice if we could have one set of `fields` Maps by putting the montgomery entry into the `fields` to replace it. I think that should work because `IntegerMontgomeryFieldModuloP` extends `IntegerFieldModuloP`. `instanceof` or other `montgomeryOps` checks would still need to exist because not all the `fields` support mongomery, and the older implementation would still be accessible for your fuzz tester. At least that is my theory. > > Thanks @ascarpino > > PS: Perhaps there is some middle ground, remove the `ECOperations > montgomeryOps` nesting, and construct (somehow?? singleton makes most things > inaccessible..) the reference ECOperations in the fuzz test instead.. not > sure how yet, but perhaps worth a further thought.. It would be nice to remove the nesting and it would be nice to be a singleton. Maybe some combination of what I mentioned above chance can help that. I haven't fully thought this out either. - PR Comment: https://git.openjdk.org/jdk/pull/18583#issuecomment-2050148475
Re: RFR: 8180892: Correct handling of annotations on parameters [v3]
On Sun, 18 Feb 2024 16:52:15 GMT, Chen Liang wrote: >> This patch aims to correct handling of annotations on parameters with the >> help of `MethodParameters` attribute, which will be always available once >> #9862 is integrated. >> >> It utilizes and expands upon the existing parameter matching logic present >> in `Executable::getAllGenericParameterTypes`, and delegate parameter >> parameterized types, parameter annotation, and parameter type annotation >> accesses through the matched results, if matching is available. If matching >> failed, it falls back to existing heuristics that works without >> `MethodParameters` attributes for annotations, in >> `Executable::handleParameterNumberMismatch` and >> `TypeAnnotationParser::buildAnnotatedTypes` (renamed >> `buildAnnotatedTypesWithHeuristics` in this patch) >> >> `ParameterMappingTest` covers these scenarios with class files that have >> `MethodParameters` or `Signature` attributes stripped or preserved to ensure >> the new Reflection API implementation works for both class files generated >> before #9862 and after its integration. >> >> Also special thanks to Joe Darcy for reviewing 8304918 (#13183); this brings >> much convenience to this patch. > > Chen Liang has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 11 commits: > > - Update copyright year, cleanup and fix volatile, fix test > - Merge branch 'master' into param-implicit-mapping > - Merge branch 'master' into param-implicit-mapping > - Fix assuming match without MethodParameters for type annotations, move > implementation related to getAnnotatedParameterTypes to around it > - copyright years > - Complete ParameterMappingTest to take care of interested scenarios > - Merge branch 'master' into param-implicit-mapping > - test wip > - Introduce base for annotated types if signature is absent but method > parameters is present > - simplify code with further contracts > - ... and 1 more: https://git.openjdk.org/jdk/compare/c2d9fa26...611deabe Keep-alive. - PR Comment: https://git.openjdk.org/jdk/pull/13664#issuecomment-2050140022
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v22]
On Wed, 10 Apr 2024 16:46:30 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Files copy method > Thanks! This looks like a good idea. Only the directory handling needs some > modification. This version tries to load > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/scratch/0/native/libawt_headless.so", > but it should load > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/java/lang/RuntimeTests/loadLibrary/aix/AIXLoadLibraryDriver/native/libfoobar.a". > Thanks! This looks like a good idea. Only the directory handling needs some > modification. This version tries to load > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/scratch/0/native/libawt_headless.so", > but it should load > "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/java/lang/RuntimeTests/loadLibrary/aix/AIXLoadLibraryDriver/native/libfoobar.a". I am facing the same error. But i do not understand why is it resolved to libawt_headless.so . - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2050138340
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Thu, 11 Apr 2024 16:08:31 GMT, Alan Bateman wrote: >> We should probably find a way to not emit the event if n == 0 and the >> operation was interrupted by `Selector.wakeUp`. Since we have another issue >> logged to emit a spin event, I wonder if we should only commit the event >> here if `n != 0`? The case where n == 0 would be handled by the spin event >> (added later) >> @AlanBateman what do you think? > > I think it's okay for now. If there is another phase of this work to help > diagnose spinning issues then it will need to re-visited. I'm very concerned > about the possible changes for that second phase, but this first phase of > instrumentation is not disruptive. OK. I am a little concerned about how often this event will be fired when using the HttpClient - given that it's enabled by default. Idle connections sitting in the pool will fire it at least once per connection every 1500ms. That may not be too bad. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1561313983
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Thu, 11 Apr 2024 09:12:30 GMT, Daniel Fuchs wrote: >>> Maybe that's OK - and maybe in that case the onus is on the user to set a >>> threshold greater than 1500ms? >> >> The threshold is 20ms so these timed-select ops in the HTTP client will >> record an event when they timeout. > > We should probably find a way to not emit the event if n == 0 and the > operation was interrupted by `Selector.wakeUp`. Since we have another issue > logged to emit a spin event, I wonder if we should only commit the event here > if `n != 0`? The case where n == 0 would be handled by the spin event (added > later) > @AlanBateman what do you think? I think it's okay for now. If there is another phase of this work to help diagnose spinning issues then it will need to re-visited. I'm very concerned about the possible changes for that second phase, but this first phase of instrumentation is not disruptive. - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1561279163
Re: RFR: 8329948: Remove string template feature [v5]
On Thu, 11 Apr 2024 10:15:54 GMT, Maurizio Cimadamore wrote: >> This PR removes support for the string template feature from the Java >> compiler and the Java SE API, as discussed here: >> >> https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Drop spurious jshell test change Also, is `java.lang.runtime.Carriers` to be kept? This utility is really helpful, but it probably should be moved to `jdk.internal` if String Templates is dropped. - PR Comment: https://git.openjdk.org/jdk/pull/18688#issuecomment-2049870402
Re: RFR: 8326617: String Template FMT removes unnecessary int to long type cast
On Mon, 2 Oct 2023 23:05:29 GMT, Shaojin Wen wrote:
> In the current version, FMT."v =%d{1}" will call the
> StringConcatHelper.prepend(long/byte[]/long) method, which should behave the
> same as STR."v ={1}". Call StringConcatHelper.prepend(long/byte[]/int),
> should not convert int to long
>
> Please review and don't hesitate to critique my approach and patch.
See #18688, String template is being removed for now.
-
PR Comment: https://git.openjdk.org/jdk/pull/16017#issuecomment-2049863809
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v8]
> 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: Address review comments (#15) * Address review comments - Changes: - all: https://git.openjdk.org/jdk/pull/18555/files - new: https://git.openjdk.org/jdk/pull/18555/files/f81aaa9f..b0ac8577 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18555=07 - incr: https://webrevs.openjdk.org/?repo=jdk=18555=06-07 Stats: 170 lines in 9 files changed: 147 ins; 11 del; 12 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
Re: RFR: 8292955: Collections.checkedMap Map.merge does not properly check key and value [v3]
On Thu, 7 Mar 2024 05:33:16 GMT, Korov wrote: >> When the specified key did not associated with a value, should check the >> `key` and `value` type. > > Korov has updated the pull request incrementally with one additional commit > since the last revision: > > Use testNG builtin functionalities and modify the test function name Keep-alive, maybe people like @viktorklang-ora might look at this simple fix too - PR Comment: https://git.openjdk.org/jdk/pull/18141#issuecomment-2049861147
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v7]
On Thu, 11 Apr 2024 00:38:11 GMT, Sandhya Viswanathan
wrote:
>> Scott Gibbons has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add movq to locate_operand
>
> src/hotspot/cpu/x86/macroAssembler_x86.cpp line 5988:
>
>> 5986: movw(Address(to, 0), value);
>> 5987: addptr(to, 2);
>> 5988: subptr(count, 1<<(shift-1));
>
> At line 5968 also we need the change from cmpl to cmpptr.
> cmpl(count, 2< src/hotspot/cpu/x86/macroAssembler_x86.cpp line 6050:
>
>> 6048: vpbroadcastd(xtmp, xtmp, Assembler::AVX_512bit);
>> 6049:
>> 6050: subptr(count, 16 << shift);
>
> At line 6045 also the cmpl should change to cmpptr:
> cmpl(count, VM_Version::avx3_threshold());
Will do.
> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2497:
>
>> 2495: //
>> 2496: address StubGenerator::generate_unsafe_setmemory(const char *name,
>> 2497: address
>> byte_fill_entry) {
>
> Need to add UnsafeSetMemoryMark on similar lines as UnsafeCopyMemoryMark to
> handle page error.
Will do.
> src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2522:
>
>> 2520: #define rScratch3 r8
>> 2521: #undef rScratch4
>> 2522: #define rScratch4 r11
>
> We could do this setup using const Register declaration instead of using
> #undef/#define pair.
We discussed this and the #define option was your preferred method since the
registers are being re-used. Do you want this changed back again?
> src/hotspot/share/opto/library_call.cpp line 4950:
>
>> 4948:
>> 4949: bool LibraryCallKit::inline_unsafe_setMemory() {
>> 4950: if (callee()->is_static()) return false; // caller must have the
>> capability!
>
> Also need to return false if StubRoutines::unsafe_setmemory() == nullptr.
Will do.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561128613
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561128862
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561129186
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561128151
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1561130102
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v22]
On Wed, 10 Apr 2024 16:46:30 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > Files copy method Thanks! This looks like a good idea. Only the directory handling needs some modification. This version tries to load "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/scratch/0/native/libawt_headless.so", but it should load "test-support/jtreg_test_jdk_java_lang_RuntimeTests_loadLibrary_aix/java/lang/RuntimeTests/loadLibrary/aix/AIXLoadLibraryDriver/native/libfoobar.a". - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2049756576
Integrated: 8319678: Several tests from corelibs areas ignore VM flags
On Wed, 3 Apr 2024 11:47:45 GMT, Mahendra Chhipa wrote: > Updated following tests to use ProcessTools methods: > test/jdk/java/lang/Thread/UncaughtExceptionsTest.java > test/jdk/java/lang/annotation/LoaderLeakTest.java > test/jdk/java/rmi/reliability/benchmark/bench/rmi/Main.java > test/jdk/java/time/nontestng/java/time/chrono/HijrahConfigTest.java > test/jdk/javax/naming/spi/providers/InitialContextTest.java This pull request has now been integrated. Changeset: 2e3682a7 Author:Mahendra Chhipa URL: https://git.openjdk.org/jdk/commit/2e3682a7f2983cd58b9564253dc698760faba4b8 Stats: 161 lines in 6 files changed: 56 ins; 58 del; 47 mod 8319678: Several tests from corelibs areas ignore VM flags Reviewed-by: naoto, jpai - PR: https://git.openjdk.org/jdk/pull/18602
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v5]
> This PR implements Gatherer-inspired encoding of `flatMap` that shows that it > is both competitive performance-wise as well as improve correctness. > > Below is the performance of `Stream::flatMap` (for reference types): > > Before this PR: > > Benchmark(size) Mode CntScore Error Units > FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s > FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s > FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s > FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s > FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s > FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s > FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s > FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s > FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s > FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s > FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s > FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s > > > After this PR: > > > Benchmark(size) Mode CntScoreError Units > FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s > FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s > FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s > FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s > FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s > FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s > FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s > FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s > FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s > FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s > FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s > FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s > > > For streams of size 100k, the following numbers are interesting: > > Before this PR: > > Benchmark(size) Mode Cnt ScoreError Units > FlatMap.par_array10 thrpt 12 0,325 ? 0,004 ops/s > FlatMap.par_iterate 10 thrpt 12 0,106 ? 0,008 ops/s > FlatMap.seq_array10 thrpt 12 0,173 ? 0,003 ops/s > FlatMap.seq_iterate 100... Viktor Klang has updated the pull request incrementally with one additional commit since the last revision: Revert "Hoisted downstream fastpath" This reverts commit 8356e69e88160efe6d49ff9f10bd5d45573cc907. - Changes: - all: https://git.openjdk.org/jdk/pull/18625/files - new: https://git.openjdk.org/jdk/pull/18625/files/8356e69e..bedbab85 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18625=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18625=03-04 Stats: 54 lines in 4 files changed: 2 ins; 35 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/18625.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18625/head:pull/18625 PR: https://git.openjdk.org/jdk/pull/18625
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container [v2]
> Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Severin Gehwolf has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains ten additional commits since the last revision: - Merge branch 'master' into jdk-8261242-is-containerized-fix - jcheck fixes - Fix tests - Implement Metrics.isContainerized() - Some clean-up - Drop cgroups testing on plain Linux - Implement fall-back logic for non-ro controller mounts - Make find_ro static and local to compilation unit - 8261242: [Linux] OSContainer::is_containerized() returns true - Changes: - all: https://git.openjdk.org/jdk/pull/18201/files - new: https://git.openjdk.org/jdk/pull/18201/files/98325f18..0df26ebd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18201=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18201=00-01 Stats: 407791 lines in 3887 files changed: 43423 ins; 33650 del; 330718 mod Patch: https://git.openjdk.org/jdk/pull/18201.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18201/head:pull/18201 PR: https://git.openjdk.org/jdk/pull/18201
Re: RFR: 8196106: Support nested infinite or recursive flat mapped streams [v4]
> This PR implements Gatherer-inspired encoding of `flatMap` that shows that it > is both competitive performance-wise as well as improve correctness. > > Below is the performance of `Stream::flatMap` (for reference types): > > Before this PR: > > Benchmark(size) Mode CntScore Error Units > FlatMap.par_array10 thrpt 12 294008,937 ? 54369,110 ops/s > FlatMap.par_array 100 thrpt 1262411,229 ? 14868,119 ops/s > FlatMap.par_array 1000 thrpt 12 8263,821 ? 452,622 ops/s > FlatMap.par_iterate 10 thrpt 1223029,978 ? 4274,449 ops/s > FlatMap.par_iterate 100 thrpt 1210532,907 ? 321,694 ops/s > FlatMap.par_iterate1000 thrpt 12 981,571 ? 135,270 ops/s > FlatMap.seq_array10 thrpt 12 2955648,495 ? 32539,142 ops/s > FlatMap.seq_array 100 thrpt 1241851,009 ? 377,546 ops/s > FlatMap.seq_array 1000 thrpt 12 1740,281 ? 1229,974 ops/s > FlatMap.seq_iterate 10 thrpt 12 321727,690 ? 5149,356 ops/s > FlatMap.seq_iterate 100 thrpt 12 8437,198 ?56,635 ops/s > FlatMap.seq_iterate1000 thrpt 12 76,994 ? 0,965 ops/s > > > After this PR: > > > Benchmark(size) Mode CntScoreError Units > FlatMap.par_array10 thrpt 12 283350,051 ? 35567,223 ops/s > FlatMap.par_array 100 thrpt 1253846,906 ? 19241,913 ops/s > FlatMap.par_array 1000 thrpt 12 8230,909 ?156,362 ops/s > FlatMap.par_iterate 10 thrpt 1226328,500 ? 5411,401 ops/s > FlatMap.par_iterate 100 thrpt 1210470,862 ?249,991 ops/s > FlatMap.par_iterate1000 thrpt 12 986,511 ?224,050 ops/s > FlatMap.seq_array10 thrpt 12 5654826,565 ? 27317,453 ops/s > FlatMap.seq_array 100 thrpt 12 187929,786 ?542,787 ops/s > FlatMap.seq_array 1000 thrpt 12 2385,346 ? 9,827 ops/s > FlatMap.seq_iterate 10 thrpt 12 812722,403 ? 160500,399 ops/s > FlatMap.seq_iterate 100 thrpt 1213542,472 ?118,769 ops/s > FlatMap.seq_iterate1000 thrpt 12 157,056 ? 1,814 ops/s > > > For streams of size 100k, the following numbers are interesting: > > Before this PR: > > Benchmark(size) Mode Cnt ScoreError Units > FlatMap.par_array10 thrpt 12 0,325 ? 0,004 ops/s > FlatMap.par_iterate 10 thrpt 12 0,106 ? 0,008 ops/s > FlatMap.seq_array10 thrpt 12 0,173 ? 0,003 ops/s > FlatMap.seq_iterate 100... Viktor Klang has updated the pull request incrementally with two additional commits since the last revision: - Hoisted downstream fastpath - Making the FlatMap bench more complete - Changes: - all: https://git.openjdk.org/jdk/pull/18625/files - new: https://git.openjdk.org/jdk/pull/18625/files/d6d35d53..8356e69e Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18625=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18625=02-03 Stats: 176 lines in 6 files changed: 150 ins; 2 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/18625.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18625/head:pull/18625 PR: https://git.openjdk.org/jdk/pull/18625
Re: RFR: 8314592: Add shortcut to SymbolLookup::find
On Mon, 25 Mar 2024 14:56:23 GMT, Per Minborg wrote: > While `SymbolLookup` correctly uses an `Optional` return to denote whether a > symbol has been found by the lookup or not (which enables composition of > symbol lookups), many clients end up just calling `Optional::get`, or > `Optional::orElseThrow()` on the result. > > This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that > will do a lookup and, if no symbol can be found, throws an > `IllegalArgumentException` with a relevant exception message. src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 177: > 175: } > 176: throw new IllegalArgumentException( > 177: "Unable to to find a symbol with the name: " + name); Another, more succinct, text could be "Symbol not found: " - PR Review Comment: https://git.openjdk.org/jdk/pull/18474#discussion_r1560897671
Re: RFR: 8314592: Add shortcut to SymbolLookup::find
On Mon, 25 Mar 2024 14:56:23 GMT, Per Minborg wrote:
> While `SymbolLookup` correctly uses an `Optional` return to denote whether a
> symbol has been found by the lookup or not (which enables composition of
> symbol lookups), many clients end up just calling `Optional::get`, or
> `Optional::orElseThrow()` on the result.
>
> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that
> will do a lookup and, if no symbol can be found, throws an
> `IllegalArgumentException` with a relevant exception message.
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 152:
> 150:
> 151: /**
> 152: * {@return the address of the symbol with the provided {@code name}
> or throws an
I suggest using the same javadoc structure as the main `find` method. Note that
there we have a summary, then a more detailed `@return` which talks about zero
length memory segments. This should do the same.
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 155:
> 153: * {@linkplain IllegalArgumentException} if no such address
> can be found}
> 154: *
> 155: * This is a convenience method that provides better exception
> messages compared
I would reframe this as: "this is equivalent to the following code, but with
better exception message" (for consistency with other API points in FFM where
we show what a method boils down to)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18474#discussion_r1560889723
PR Review Comment: https://git.openjdk.org/jdk/pull/18474#discussion_r1560893974
Re: RFR: 8319516: AIX System::loadLibrary needs support to load a shared library from an archive object [v22]
On Wed, 10 Apr 2024 16:46:30 GMT, Suchismith Roy wrote:
>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Files copy method
Hello Martin,
> Maybe using that directory is easier. Does anybody else have a suggestion?
Each jtreg test, when it is run, will be run in a "scratch" directory. Details
about that are explained here
https://openjdk.org/jtreg/faq.html#scratch-directory. That directory can be
used to create test specific content.
As for the test, I think what you will need is something like this (I don't
know or have access to AIX, so this obviously isn't tested)
import java.nio.file.Files;
import java.nio.file.Path;
import jdk.test.lib.process.ProcessTools;
/**
* @test
* @bug 8319516
* @summary verify that System.loadLibrary on AIX is able to load libraries
from ".a" (archive) file
* @requires os.family == "aix"
* @library /test/lib/
* @build jdk.test.lib.process.ProcessTools
* @run driver AIXLoadLibraryDriver
*/
public class AIXLoadLibraryDriver {
private static final String TEST_LIBRARY_NAME = "foobar";
// creates a ".a" archive file in a test specific directory and then
// launches a java application passing this directory through
"-Djava.library.path".
// the java application then attempts to load the library using
System.loadLibrary()
public static void main(final String[] args) throws Exception {
final String javaHome = System.getProperty("java.home");
final Path libawtSo = Path.of(javaHome).resolve("lib", "libawt.so");
if (!Files.exists(libawtSo)) {
throw new AssertionError(libawtSo + " is missing");
}
final String archiveFileName = "lib" + TEST_LIBRARY_NAME + ".a";
// copy over libawt.so as an archive file to test specific scratch dir
final Path testNativeLibDir = Path.of("native").toAbsolutePath();
Files.createDirectories(testNativeLibDir);
final Path libDummyArchive = testNativeLibDir.resolve(archiveFileName);
Files.copy(libawtSo, libDummyArchive);
// launch a java application which calls System.loadLibrary and is
passed
// the directory containing the native library archive file, through
// -Djava.library.path
System.out.println("Launching application with library path " +
testNativeLibDir);
final ProcessBuilder processBuilder =
ProcessTools.createTestJavaProcessBuilder(
"-Djava.library.path=" + testNativeLibDir,
AIXLoadLibraryDriver.LoadLibraryApp.class.getName());
ProcessTools.executeCommand(processBuilder).shouldHaveExitValue(0);
}
static class LoadLibraryApp {
public static void main(final String[] args) throws Exception {
System.out.println("attempting to load library " +
TEST_LIBRARY_NAME);
System.loadLibrary(TEST_LIBRARY_NAME);
System.out.println(TEST_LIBRARY_NAME + " successfully loaded");
}
}
}
-
PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2049416575
RFR: 8314592: Add shortcut to SymbolLookup::find
While `SymbolLookup` correctly uses an `Optional` return to denote whether a symbol has been found by the lookup or not (which enables composition of symbol lookups), many clients end up just calling `Optional::get`, or `Optional::orElseThrow()` on the result. This PR proposes to add a convenience method `SymbolLookup::findOrThrow` that will do a lookup and, if no symbol can be found, throws an `IllegalArgumentException` with a relevant exception message. - Commit messages: - Remove white space - Add SymbolLookup::findOrThrow Changes: https://git.openjdk.org/jdk/pull/18474/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18474=00 Issue: https://bugs.openjdk.org/browse/JDK-8314592 Stats: 93 lines in 21 files changed: 30 ins; 0 del; 63 mod Patch: https://git.openjdk.org/jdk/pull/18474.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18474/head:pull/18474 PR: https://git.openjdk.org/jdk/pull/18474
Withdrawn: 8329997: Add MemorySegment::reinterpretate overloads for alignment constraints
On Wed, 10 Apr 2024 12:49:11 GMT, Per Minborg wrote: > This PR proposes to add two overloads `MemorySegment::reinterpretate` to > allow aligned reinterpretation. This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/18717
Re: RFR: 8329948: Remove string template feature [v5]
> This PR removes support for the string template feature from the Java > compiler and the Java SE API, as discussed here: > > https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Drop spurious jshell test change - Changes: - all: https://git.openjdk.org/jdk/pull/18688/files - new: https://git.openjdk.org/jdk/pull/18688/files/51e261bd..bbe05c97 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18688=04 - incr: https://webrevs.openjdk.org/?repo=jdk=18688=03-04 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18688.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18688/head:pull/18688 PR: https://git.openjdk.org/jdk/pull/18688
Re: RFR: 8329948: Remove string template feature [v4]
On Thu, 11 Apr 2024 09:12:06 GMT, Jan Lahoda wrote:
>> Maurizio Cimadamore has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Fix issues in digit classes
>
> test/langtools/jdk/jshell/CompletionSuggestionTest.java line 327:
>
>> 325: assertSignature("String.format(|",
>> 326: "String String.format(String, Object...)",
>> 327: "String String.format(java.util.Locale, String,
>> Object...)"
>
> Nit: this change appears to be unnecessary?
I'll revert - thanks
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1560775208
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v4]
On Thu, 11 Apr 2024 09:54:53 GMT, Mahendra Chhipa wrote: >> Updated following tests to use ProcessTools methods: >> test/jdk/java/lang/Thread/UncaughtExceptionsTest.java >> test/jdk/java/lang/annotation/LoaderLeakTest.java >> test/jdk/java/rmi/reliability/benchmark/bench/rmi/Main.java >> test/jdk/java/time/nontestng/java/time/chrono/HijrahConfigTest.java >> test/jdk/javax/naming/spi/providers/InitialContextTest.java > > Mahendra Chhipa has updated the pull request incrementally with one > additional commit since the last revision: > > Removed commented code. Marked as reviewed by jpai (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18602#pullrequestreview-1993775818
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v3]
On Thu, 11 Apr 2024 09:26:27 GMT, Jaikiran Pai wrote:
>> Mahendra Chhipa has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Implemented the review comments.
>
> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 301:
>
>> 299: .map(Path::toString)
>> 300: .collect(Collectors.joining(File.pathSeparator));
>> 301: //System.setProperty("test.noclasspath", "true");
>
> This newly introduced commented code can be removed.
Sorry, I missed this. Removed in current commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560754800
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v4]
> Updated following tests to use ProcessTools methods: > test/jdk/java/lang/Thread/UncaughtExceptionsTest.java > test/jdk/java/lang/annotation/LoaderLeakTest.java > test/jdk/java/rmi/reliability/benchmark/bench/rmi/Main.java > test/jdk/java/time/nontestng/java/time/chrono/HijrahConfigTest.java > test/jdk/javax/naming/spi/providers/InitialContextTest.java Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Removed commented code. - Changes: - all: https://git.openjdk.org/jdk/pull/18602/files - new: https://git.openjdk.org/jdk/pull/18602/files/4d258b00..8aba98ce Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18602=03 - incr: https://webrevs.openjdk.org/?repo=jdk=18602=02-03 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/18602.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18602/head:pull/18602 PR: https://git.openjdk.org/jdk/pull/18602
Re: RFR: 8261242: [Linux] OSContainer::is_containerized() returns true when run outside a container
On Mon, 11 Mar 2024 16:55:36 GMT, Severin Gehwolf wrote: > Please review this enhancement to the container detection code which allows > it to figure out whether the JVM is actually running inside a container > (`podman`, `docker`, `crio`), or with some other means that enforces > memory/cpu limits by means of the cgroup filesystem. If neither of those > conditions hold, the JVM runs in not containerized mode, addressing the issue > described in the JBS tracker. For example, on my Linux system > `is_containerized() == false" is being indicated with the following trace log > line: > > > [0.001s][debug][os,container] OSContainer::init: is_containerized() = false > because no cpu or memory limit is present > > > This state is being exposed by the Java `Metrics` API class using the new > (still JDK internal) `isContainerized()` method. Example: > > > java -XshowSettings:system --version > Operating System Metrics: > Provider: cgroupv1 > System not containerized. > openjdk 23-internal 2024-09-17 > OpenJDK Runtime Environment (fastdebug build > 23-internal-adhoc.sgehwolf.jdk-jdk) > OpenJDK 64-Bit Server VM (fastdebug build 23-internal-adhoc.sgehwolf.jdk-jdk, > mixed mode, sharing) > > > The basic property this is being built on is the observation that the cgroup > controllers typically get mounted read only into containers. Note that the > current container tests assert that `OSContainer::is_containerized() == true` > in various tests. Therefore, using the heuristic of "is any memory or cpu > limit present" isn't sufficient. I had considered that in an earlier > iteration, but many container tests failed. > > Overall, I think, with this patch we improve the current situation of > claiming a containerized system being present when it's actually just a > regular Linux system. > > Testing: > > - [x] GHA (risc-v failure seems infra related) > - [x] Container tests on Linux x86_64 of cgroups v1 and cgroups v2 (including > gtests) > - [x] Some manual testing using cri-o > > Thoughts? Gentle ping. - PR Comment: https://git.openjdk.org/jdk/pull/18201#issuecomment-2049297883
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v3]
On Thu, 11 Apr 2024 09:07:10 GMT, Mahendra Chhipa wrote: >> Updated following tests to use ProcessTools methods: >> test/jdk/java/lang/Thread/UncaughtExceptionsTest.java >> test/jdk/java/lang/annotation/LoaderLeakTest.java >> test/jdk/java/rmi/reliability/benchmark/bench/rmi/Main.java >> test/jdk/java/time/nontestng/java/time/chrono/HijrahConfigTest.java >> test/jdk/javax/naming/spi/providers/InitialContextTest.java > > Mahendra Chhipa has updated the pull request incrementally with one > additional commit since the last revision: > > Implemented the review comments. Hello Mahendra, thank you for the updates. There's one review comment I've added about a stray code comment, other than that the changes look OK to me. I haven't checked which tiers these tests reside in. Before integrating, it would be good to run the specific tiers (or tier1, tier2 and tier3) and make sure these changes don't cause issues. - Marked as reviewed by jpai (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18602#pullrequestreview-1993707442
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v3]
On Thu, 11 Apr 2024 09:07:10 GMT, Mahendra Chhipa wrote:
>> Updated following tests to use ProcessTools methods:
>> test/jdk/java/lang/Thread/UncaughtExceptionsTest.java
>> test/jdk/java/lang/annotation/LoaderLeakTest.java
>> test/jdk/java/rmi/reliability/benchmark/bench/rmi/Main.java
>> test/jdk/java/time/nontestng/java/time/chrono/HijrahConfigTest.java
>> test/jdk/javax/naming/spi/providers/InitialContextTest.java
>
> Mahendra Chhipa has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Implemented the review comments.
test/jdk/javax/naming/spi/providers/InitialContextTest.java line 301:
> 299: .map(Path::toString)
> 300: .collect(Collectors.joining(File.pathSeparator));
> 301: //System.setProperty("test.noclasspath", "true");
This newly introduced commented code can be removed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560711526
Re: RFR: 8329948: Remove string template feature [v4]
On Wed, 10 Apr 2024 10:59:25 GMT, Maurizio Cimadamore
wrote:
>> This PR removes support for the string template feature from the Java
>> compiler and the Java SE API, as discussed here:
>>
>> https://mail.openjdk.org/pipermail/amber-spec-experts/2024-April/004106.html
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix issues in digit classes
javac and JShell changes look good to me (with a nit in JShell tests).
For consideration: using `{` will now produce the "illegal escape character"
error. Which is technically correct, but maybe we could add a special error,
saying that StringTemplates are removed for now? So that if someone will try to
compile source code with StringTemplates, they would now this was intentional.
Just for consideration.
test/langtools/jdk/jshell/CompletionSuggestionTest.java line 327:
> 325: assertSignature("String.format(|",
> 326: "String String.format(String, Object...)",
> 327: "String String.format(java.util.Locale, String,
> Object...)"
Nit: this change appears to be unnecessary?
-
Marked as reviewed by jlahoda (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18688#pullrequestreview-1993673798
PR Review Comment: https://git.openjdk.org/jdk/pull/18688#discussion_r1560692978
Re: RFR: 8310994: Add JFR event for selection operations [v3]
On Mon, 26 Feb 2024 17:40:59 GMT, Alan Bateman wrote: >> Is n == 0 intended to detect a spinning condition where the selector goes >> back into select when the event has not been handled? >> >> In that case should we still emit an event if a timeout is present and the >> duration is greater than the timeout? For instance, in the HttpClient, we >> have a selector thread that will wake up at regular interval - we set up a >> timeout which is the min between the next expected timer event and 1500ms. >> So that could potentially fire an event every 1500ms if for instance the >> connection threshold is less than 1500ms and the connection is idle. >> >> Maybe that's OK - and maybe in that case the onus is on the user to set a >> threshold greater than 1500ms? >> >> Or should it be ((n == 0 && durationToMillis(duration) < >> timeoutToMillis(timeout)) || ...)) (duration and timeout probably are not in >> the same unit of time) - also if shouldCommit return false will the event >> actually be emitted down the road? Because if not then the ( n== 0 || ...) >> won't work. > >> Maybe that's OK - and maybe in that case the onus is on the user to set a >> threshold greater than 1500ms? > > The threshold is 20ms so these timed-select ops in the HTTP client will > record an event when they timeout. We should probably find a way to not emit the event if n == 0 and the operation was interrupted by `Selector.wakeUp`. Since we have another issue logged to emit a spin event, I wonder if we should only commit the event here if `n != 0`? The case where n == 0 would be handled by the spin event (added later) @AlanBateman what do you think? - PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1560693482
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v3]
> Updated following tests to use ProcessTools methods: > test/jdk/java/lang/Thread/UncaughtExceptionsTest.java > test/jdk/java/lang/annotation/LoaderLeakTest.java > test/jdk/java/rmi/reliability/benchmark/bench/rmi/Main.java > test/jdk/java/time/nontestng/java/time/chrono/HijrahConfigTest.java > test/jdk/javax/naming/spi/providers/InitialContextTest.java Mahendra Chhipa has updated the pull request incrementally with one additional commit since the last revision: Implemented the review comments. - Changes: - all: https://git.openjdk.org/jdk/pull/18602/files - new: https://git.openjdk.org/jdk/pull/18602/files/04a35331..4d258b00 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18602=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18602=01-02 Stats: 21 lines in 3 files changed: 0 ins; 10 del; 11 mod Patch: https://git.openjdk.org/jdk/pull/18602.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18602/head:pull/18602 PR: https://git.openjdk.org/jdk/pull/18602
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v2]
On Wed, 10 Apr 2024 10:57:33 GMT, Jaikiran Pai wrote:
>> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 286:
>>
>>> 284: OutputAnalyzer outputAnalyzer =
>>> ProcessTools.executeCommand(commands.toArray(new String[commands.size()]));
>>> 285: if(outputAnalyzer.getExitValue() != 0) {
>>> 286: throw new RuntimeException(outputAnalyzer.getOutput());
>>
>> Same comment as above `outputAnalyzer.shouldHaveExitValue(0)` would be
>> better.
>
> Applies to some other files in this PR as well.
Thanks. Implemented in this commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560682745
Re: RFR: 8319678: Several tests from corelibs areas ignore VM flags [v2]
On Wed, 10 Apr 2024 10:40:55 GMT, Jaikiran Pai wrote:
>> Mahendra Chhipa has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Implemented review comments.
>> Updated EscapePath test.
>
> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 65:
>
>> 63: * @library /test/lib
>> 64: * @build jdk.test.lib.process.ProcessTools
>> 65: * @run main/othervm InitialContextTest
>
> Hello Mahendra, I see that this test and one other test is being changed to
> `/othervm` and I suspect it's because, we now call:
>
>
> System.setProperty("test.noclasspath", "true");
>
> for the `ProcessTools` to skip adding the default `-cp` option to the
> launched Java process.
> In reality, we don't have to set that system property and thus you don't have
> to change this and the other test to `othervm`. See a previous discussion
> about the classpath handling by `ProcessTools` here
> https://github.com/openjdk/jdk/pull/17787/files/a9fcc6c2900356250b29b3d11b402790a84d9317#r1484276695
> - essentially, whatever classpath you end up passing as part of the command
> to `ProcessTools.createTestJavaProcessBuilder()` will end up getting used.
Thanks. Implemented in this commit.
> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 268:
>
>> 266: OutputAnalyzer outputAnalyzer =
>> ProcessTools.executeCommand(commands);
>> 267: if(outputAnalyzer.getExitValue() != 0) {
>> 268: throw new RuntimeException(outputAnalyzer.getOutput());
>
> I think it would be better to just call:
>
> outputAnalyzer.shouldHaveExitValue(0);
>
> that way it even prints the stdout/stderr logs and throws the exception.
Thanks. Implemented in this commit.
> test/jdk/javax/naming/spi/providers/InitialContextTest.java line 271:
>
>> 269: }
>> 270: } catch (Exception ex) {
>> 271: throw new RuntimeException(ex.getMessage());
>
> This and other similar places in this PR will end up swallowing the
> underlying exception. It would be better to just have the method have a
> `throws Exception` or change this to:
>
>
> throw new RuntimeException(ex);
Thanks. Implemented in this commit.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560681857
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560682373
PR Review Comment: https://git.openjdk.org/jdk/pull/18602#discussion_r1560683141
Re: RFR: 8325679: Optimize ArrayList subList sort
On Wed, 14 Feb 2024 23:37:03 GMT, Stuart Marks wrote: >> Somewhat surprisingly, `ArrayList$Sublist.sort()` is not specialized and >> will thus fall back to slower default method of `List.sort()` instead of >> sorting a range of the array in-place in its backing root `ArrayList`. >> >> This doesn't change observable behavior, so haven't added tests, and `tier1` >> tests still all pass except for >> `test/jdk/java/util/Locale/LocaleProvidersFormat.java` which also currently >> fails on master too on the machine I tested on. > > (Discussion mainly of historical interest.) > > @pavelrappo Correct, historically, `Collections.sort` would fail to sort > `CopyOnWriteArrayList`. You have to go back to JDK 7 to see this. The sorting > approach used by `Collections.sort` (still present in the default > implementation of `List.sort`) gets an array using `toArray()`, sorts it, and > then copies the sorted elements back using `ListIterator.set`. Since > `CopyOnWriteArrayList` doesn't support modifications using `ListIterator`, it > fails with `UnsupportedOperationException`. The overrides of `List.sort` have > fixed this problem. > > COWAL still has some problems with other things that use similar techniques, > such as `Collections.shuffle`. That uses get/set to swap elements, which > COWAL does support, but it copies the array on each set() operation. This > results in N copies of the array being made for an N-element list. Hey folks (@stuart-marks or @JimLaskey) it's been a while – can I get a review? - PR Comment: https://git.openjdk.org/jdk/pull/17818#issuecomment-2049131137
Re: RFR: 8310994: Add JFR event for selection operations [v6]
On Wed, 10 Apr 2024 23:51:34 GMT, Tim Prinzing wrote:
>> Added mirror event with static methods: jdk.internal.event.SelectionEvent
>> that provides the duration of select calls and the count of how many keys
>> are available.
>>
>> Emit the event from SelectorImpl::lockAndDoSelect
>>
>> Test at jdk.jfr.event.io.TestSelectionEvents
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove selector spin event attempt and associated test.
src/jdk.jfr/share/classes/jdk/jfr/events/SelectorSelectEvent.java line 44:
> 42:
> 43: @Label("Timeout Value")
> 44: @Description("Milliseconds to block, or zero to block indefinitely")
The unit should not be mentioned. A GUI will typically convert it from the
TimeSpan information and it will just look confusing if the descriptions says
milliseconds when it's, for instance, displayed as 1 s.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1560583317
Re: RFR: 8310994: Add JFR event for selection operations [v6]
On Wed, 10 Apr 2024 23:51:34 GMT, Tim Prinzing wrote:
>> Added mirror event with static methods: jdk.internal.event.SelectionEvent
>> that provides the duration of select calls and the count of how many keys
>> are available.
>>
>> Emit the event from SelectorImpl::lockAndDoSelect
>>
>> Test at jdk.jfr.event.io.TestSelectionEvents
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove selector spin event attempt and associated test.
src/jdk.jfr/share/classes/jdk/jfr/events/SelectorSelectEvent.java line 33:
> 31:
> 32: @Name(Type.EVENT_NAME_PREFIX + "SelectorSelect")
> 33: @Label("SelectorSelect")
This label should "Selector Select" and the description could be dropped as it
doesn't add information.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1560574399
Re: RFR: 8310994: Add JFR event for selection operations [v6]
On Wed, 10 Apr 2024 23:51:34 GMT, Tim Prinzing wrote:
>> Added mirror event with static methods: jdk.internal.event.SelectionEvent
>> that provides the duration of select calls and the count of how many keys
>> are available.
>>
>> Emit the event from SelectorImpl::lockAndDoSelect
>>
>> Test at jdk.jfr.event.io.TestSelectionEvents
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> remove selector spin event attempt and associated test.
src/jdk.jfr/share/classes/jdk/jfr/events/SelectorSelectEvent.java line 40:
> 38:
> 39: @Label("SelectionKey Count")
> 40: @Description("Number of channels ready for I/O or added to ready set")
We'll need to give you a better description for this. The value is the number
of selection keys were updated by the selection operation. In many usages this
will be the same as the number of keys added to the selected-key set (not
"ready set") but they can be different.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16710#discussion_r1560501216
