Withdrawn: 8317993: Add capturing factories to classes in java.util.function package

[Per Minborg]

On Tue, 17 Oct 2023 08:05:09 GMT, Per Minborg  wrote:

> This PR proposes to add a number of "capturing factories" in classes in the 
> `java.util.function` package.
> 
> The PR additionally (an optionally) proposes to add a new function 
> `UnaryOperator::andThenUnary` to allow composition while retaining the 
> `UnaryOperator` type.
> 
> With the new changes, it is possible to write code like this (example from 
> `java.util.function.Function`):
> 
> 
> // Resolve ambiguity
> var function = Function.of(String::isEmpty); // Function
> var predicate = Predicate.of(String::isEmpty); // Predicate
> 
> // Fluent composition
> var chained = Function.of(String::length)   // Function
>.andThen(Integer::byteValue);  // Function Byte>
> 
> 
> Please see the original bug report for a comprehensive description of these 
> proposed changes.
> 
> Note: It is not the objective to promote `var` declaration or to prevent 
> previous ways of capturing lambdas and method references. The comments in the 
> code above is for explaining the binding and once that becomes obvious, such 
> comments are likely to not appear in real code. Users that prefers having a 
> normal type declaration can still do that.
> 
> Note: Functional interfaces of primitives have not been considered (in this 
> round). Otherwise, functional interfaces that might be ambiguous or that 
> supports composition have been included. Hence, `Supplier` did not get a 
> factory method.

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.org/jdk/pull/16213

Re: RFR: 8318650: Optimized subword gather for x86 targets. [v2]

[Xiaohong Gong]

On Fri, 27 Oct 2023 05:09:15 GMT, Jatin Bhateja  wrote:

>> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java 
>> line 3062:
>> 
>>> 3060: // Constant folding should sweep out following conditonal 
>>> logic.
>>> 3061: if (isp.length() > IntVector.SPECIES_PREFERRED.length()) {
>>> 3062: lsp = IntVector.SPECIES_PREFERRED;
>> 
>> What happens if the needed index vector length is larger that the max vector 
>> length of int type? 
>> For example, byte vectors with `SPECIES_128`, may needs an index vector with 
>> `IntVector.SPECIES_512` species? And on some architectures like NEON, or SVE 
>> with 128-bit max vector size, the preferred species is only `SPECIES_128`.
>
> For x86 backend implementation we emit a gather loop hence index species is 
> not used for sub-word, I am performing the index out of bound in a loop which 
> operate a max integer vector granularity supported by target.
> https://github.com/openjdk/jdk/pull/16354/files#diff-13cc2d6ec18e487ddae05cda671bdb6bb7ffd42ff7bc51a2e00c8c5e622bd55dR3641
> 
> Because there is cap on max integral vector size, thus for targets supporting 
> direct sub-word gather instruction will have to emit an instruction sequence 
> comprising of partial indexMap loads into integral vectors and issue multiple 
> gather operations.

I see, thanks for the explanation! It's different from SVE, which the index 
should be an int vector with the same length of the target vector.  We have to 
generate the index vectors. So the array length check to the given index map is 
necessary in java side.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16354#discussion_r1374215257

Integrated: 8315097: Rename createJavaProcessBuilder

[Leo Korinth]

On Mon, 28 Aug 2023 15:54:08 GMT, Leo Korinth  wrote:

> This pull request renames `createJavaProcessBuilder` to 
> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to 
> `createTestJavaProcessBuilder`. Both are implemented through a private 
> `createJavaProcessBuilder`. It also updates the java doc.
> 
> This is so that it should be harder to by mistake use 
> `createLimitedTestJavaProcessBuilder` that is problematic because it will not 
> forward JVM flags to the tested JVM.

This pull request has now been integrated.

Changeset: d52a995f
Author:Leo Korinth 
URL:   
https://git.openjdk.org/jdk/commit/d52a995f35de26c2cc4074297a75141e4a363e1b
Stats: 1574 lines in 560 files changed: 44 ins; 10 del; 1520 mod

8315097: Rename createJavaProcessBuilder

Reviewed-by: lmesnik, dholmes, rriggs, stefank

-

PR: https://git.openjdk.org/jdk/pull/15452

Re: RFR: 8318586: Explicitly handle upcall stub allocation failure [v3]

[Maurizio Cimadamore]

On Fri, 27 Oct 2023 04:54:04 GMT, David Holmes  wrote:

>> src/java.base/share/classes/java/lang/foreign/Linker.java line 532:
>> 
>>> 530:  * @throws IllegalArgumentException if an invalid combination of 
>>> linker options is given.
>>> 531:  * @throws IllegalCallerException If the caller is in a module 
>>> that does not have native access enabled.
>>> 532:  * @throws OutOfMemoryError if the runtime does not have the 
>>> memory needed to create the downcall handle.
>> 
>> Suggestions for the phrasing here are welcome. I think we should use 
>> something that works for both downcall handles and upcall stubs though.
>
> OOME is pretty much understood to be possible anywhere, given it is a 
> VirtualMachineError. We often do not document it explicitly. The risk with 
> documenting it is that it gives the impression that other methods, which 
> don't document it, can never throw it. A rough grep for `@throws 
> OutOfMemoryError` reveals only 15 classes in java.base that explicitly 
> document this.

Taking inspiration from other methods that throw this exception, maybe 
something like this might work:

`if the downcall method handle cannot be allocated by the Linker`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16311#discussion_r1374268730

Re: RFR: 8318586: Explicitly handle upcall stub allocation failure [v3]

[Maurizio Cimadamore]

On Fri, 27 Oct 2023 04:54:04 GMT, David Holmes  wrote:

>> src/java.base/share/classes/java/lang/foreign/Linker.java line 532:
>> 
>>> 530:  * @throws IllegalArgumentException if an invalid combination of 
>>> linker options is given.
>>> 531:  * @throws IllegalCallerException If the caller is in a module 
>>> that does not have native access enabled.
>>> 532:  * @throws OutOfMemoryError if the runtime does not have the 
>>> memory needed to create the downcall handle.
>> 
>> Suggestions for the phrasing here are welcome. I think we should use 
>> something that works for both downcall handles and upcall stubs though.
>
> OOME is pretty much understood to be possible anywhere, given it is a 
> VirtualMachineError. We often do not document it explicitly. The risk with 
> documenting it is that it gives the impression that other methods, which 
> don't document it, can never throw it. A rough grep for `@throws 
> OutOfMemoryError` reveals only 15 classes in java.base that explicitly 
> document this.

That said, I also agree with @dholmes-ora - e.g. I'm not sure how much OOME is 
important to document here, since it reflects an internal state of the JVM, 
rather than something the client can do something about.

E.g. if you create an allocator with `SegmentAllocator::slicingAllocator`, at 
some point you are going to run out of space in the underlying segment, so it 
makes sense to report failures (and to document why that happens). But in this 
case the documentation is going to be very vague, and I don't think it provides 
a lot of value.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16311#discussion_r1374271936

Re: RFR: 8315097: Rename createJavaProcessBuilder [v7]

[Leo Korinth]

On Wed, 25 Oct 2023 08:44:29 GMT, Leo Korinth  wrote:

>> This pull request renames `createJavaProcessBuilder` to 
>> `createLimitedTestJavaProcessBuilder` and renames `createTestJvm` to 
>> `createTestJavaProcessBuilder`. Both are implemented through a private 
>> `createJavaProcessBuilder`. It also updates the java doc.
>> 
>> This is so that it should be harder to by mistake use 
>> `createLimitedTestJavaProcessBuilder` that is problematic because it will 
>> not forward JVM flags to the tested JVM.
>
> Leo Korinth has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   fix copyright year and indentation

Thanks, see:
https://bugs.openjdk.org/browse/JDK-8318962

-

PR Comment: https://git.openjdk.org/jdk/pull/15452#issuecomment-1782552641

Re: RFR: 8318737: Fallback linker passes bad JNI handle [v3]

[Jorn Vernee]

On Fri, 27 Oct 2023 05:26:47 GMT, David Holmes  wrote:

> @JornVernee please don't forget that all non-trivial (as defined by the dev 
> guide) hotspot changes require at least 2 reviews and a 24 hour wait before 
> integration. Thanks.

Right, sorry. I think I confused this PR with 
https://github.com/openjdk/jdk/pull/16311 for a moment, and got in my head that 
shipilev had already approved.

> One minor nit but the hotspot changes, aposteri, look good.

Do you want me to create a separate PR to remove the comment?

-

PR Comment: https://git.openjdk.org/jdk/pull/16349#issuecomment-1782568248
PR Comment: https://git.openjdk.org/jdk/pull/16349#issuecomment-1782568682

Re: RFR: 8254693: Add Panama feature to pass heap segments to native code [v12]

[Jorn Vernee]

On Fri, 27 Oct 2023 03:49:17 GMT, Yasumasa Suenaga  wrote:

>> src/hotspot/cpu/x86/downcallLinker_x86_64.cpp line 110:
>> 
>>> 108:   __ mov(rsp, r12); // restore sp
>>> 109:   __ reinit_heapbase();
>>> 110: }
>> 
>> This is a minor cleanup to share this code for the three use sites below.
>
> Question: `r12` does not need to remember?
> 
> According to [CallingSequences in OpenJDK 
> Wiki](https://wiki.openjdk.org/display/HotSpot/CallingSequences), `r12` may 
> be reserved for HeapBase if COOP is enabled.  
> (`r12` is also used in another places in downcallLinker_x86_64.cpp without 
> restoring...)

You mean `reinit_heapbase` can be removed? I'm not sure whether the caller 
expects it to be preserved.

Note that `r12` is used in this case to save and restore `rsp`. This is needed 
since we access data in the frame relative to `rsp`.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16201#discussion_r1374300821

Re: RFR: 8318586: Explicitly handle upcall stub allocation failure [v3]

[Jorn Vernee]

On Fri, 27 Oct 2023 08:52:42 GMT, Maurizio Cimadamore  
wrote:

>> OOME is pretty much understood to be possible anywhere, given it is a 
>> VirtualMachineError. We often do not document it explicitly. The risk with 
>> documenting it is that it gives the impression that other methods, which 
>> don't document it, can never throw it. A rough grep for `@throws 
>> OutOfMemoryError` reveals only 15 classes in java.base that explicitly 
>> document this.
>
> That said, I also agree with @dholmes-ora - e.g. I'm not sure how much OOME 
> is important to document here, since it reflects an internal state of the 
> JVM, rather than something the client can do something about.
> 
> E.g. if you create an allocator with `SegmentAllocator::slicingAllocator`, at 
> some point you are going to run out of space in the underlying segment, so it 
> makes sense to report failures (and to document why that happens). But in 
> this case the documentation is going to be very vague, and I don't think it 
> provides a lot of value.

Ok. I figured it was similar to Unsafe::allocateMemory, which also documents to 
OOME. But then again, the user is not directly interested in memory in this 
case.

I"ll remove these `@throws` tags then

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16311#discussion_r1374303739

Integrated: 8318964: Fix build failures caused by 8315097

[Leo Korinth]

Update method name after huge renaming conflict

-

Commit messages:
 - 8318964: Fix build failures caused by 8315097

Changes: https://git.openjdk.org/jdk/pull/16395/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16395&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8318964
  Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/16395.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16395/head:pull/16395

PR: https://git.openjdk.org/jdk/pull/16395

Re: Integrated: 8318964: Fix build failures caused by 8315097

[Axel Boldt-Christmas]

On Fri, 27 Oct 2023 09:48:00 GMT, Leo Korinth  wrote:

> Update method name after huge renaming conflict

Marked as reviewed by aboldtch (Reviewer).

Trivial fix.

-

PR Review: https://git.openjdk.org/jdk/pull/16395#pullrequestreview-1701406738
PR Comment: https://git.openjdk.org/jdk/pull/16395#issuecomment-1782626722

Integrated: 8318964: Fix build failures caused by 8315097

[Leo Korinth]

On Fri, 27 Oct 2023 09:48:00 GMT, Leo Korinth  wrote:

> Update method name after huge renaming conflict

This pull request has now been integrated.

Changeset: b9dcd4b7
Author:Leo Korinth 
URL:   
https://git.openjdk.org/jdk/commit/b9dcd4b74138dd77faa46525f101b985248fffc5
Stats: 5 lines in 5 files changed: 0 ins; 0 del; 5 mod

8318964: Fix build failures caused by 8315097

Reviewed-by: aboldtch, rcastanedalo

-

PR: https://git.openjdk.org/jdk/pull/16395

Re: Integrated: 8318964: Fix build failures caused by 8315097

[Leo Korinth]

On Fri, 27 Oct 2023 09:48:00 GMT, Leo Korinth  wrote:

> Update method name after huge renaming conflict

Thanks!!!

-

PR Comment: https://git.openjdk.org/jdk/pull/16395#issuecomment-1782627439

Re: Integrated: 8318964: Fix build failures caused by 8315097

[Roberto Castañeda Lozano]

On Fri, 27 Oct 2023 09:48:00 GMT, Leo Korinth  wrote:

> Update method name after huge renaming conflict

Looks good and trivial.

-

Marked as reviewed by rcastanedalo (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16395#pullrequestreview-1701407759

Re: RFR: 8318586: Explicitly handle upcall stub allocation failure [v3]

[Jorn Vernee]

On Thu, 26 Oct 2023 18:11:01 GMT, Jorn Vernee  wrote:

>> Explicitly handle UpcallStub allocation failures by terminating. We 
>> currently might try to use the returned `nullptr` which would fail sooner or 
>> later. This patch just makes the termination explicit.
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Throw OOME for allocation failures

I've also removed the test that tries to trigger an OOME when allocating 
downcall stubs. It seems not really possible to isolate that particular code 
path (unless a direct whitebox API is added maybe, but that also kinda defeats 
the purpose of testing), leading to a flaky test. I've left the test for upcall 
stubs, as that seems to work well enough (but, might need to drop that as well).

-

PR Comment: https://git.openjdk.org/jdk/pull/16311#issuecomment-1782629405

Re: RFR: 8318586: Explicitly handle upcall stub allocation failure [v4]

[Jorn Vernee]

> Explicitly handle UpcallStub allocation failures by terminating. We currently 
> might try to use the returned `nullptr` which would fail sooner or later. 
> This patch just makes the termination explicit.

Jorn Vernee has updated the pull request incrementally with three additional 
commits since the last revision:

 - remove unused imports
 - remove flaky downcall stub oome test
 - remove @throws

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16311/files
  - new: https://git.openjdk.org/jdk/pull/16311/files/6cfd8bed..52e875eb

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16311&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16311&range=02-03

  Stats: 39 lines in 2 files changed: 0 ins; 39 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/16311.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16311/head:pull/16311

PR: https://git.openjdk.org/jdk/pull/16311

Integrated: 8288899: java/util/concurrent/ExecutorService/CloseTest.java failed with "InterruptedException: sleep interrupted"

[Doug Lea]

On Sat, 3 Jun 2023 14:08:02 GMT, Doug Lea  wrote:

> Addresses Jdk 8288899 : java/util/concurrent/ExecutorService/CloseTest.java 
> failed with "InterruptedException: sleep interrupted" and related issues.
> 
> This is a major ForkJoin update (and hard to review -- sorry) that finally 
> addresses incompatibilities between ExecutorService and ForkJoinPool (which 
> claims to implement it), with the goal of avoiding continuing bug reports and 
> incompatibilities. Doing this required reworking internal control to use 
> phaser/seqlock-style versioning schemes (affecting nearly every method) that 
> ensure consistent data structures and actions without requiring global 
> synchronization or locking on every task execution that would massively 
> degrade performance. The previous lack of a solution to this was the main 
> reason for these incompatibilities.

This pull request has now been integrated.

Changeset: 667cca9d
Author:Doug Lea 
URL:   
https://git.openjdk.org/jdk/commit/667cca9d7aef1ff4abe630cefaac34c0b1646925
Stats: 4526 lines in 18 files changed: 2460 ins; 664 del; 1402 mod

8288899: java/util/concurrent/ExecutorService/CloseTest.java failed with 
"InterruptedException: sleep interrupted"

Reviewed-by: alanb

-

PR: https://git.openjdk.org/jdk/pull/14301

Re: RFR: 8254693: Add Panama feature to pass heap segments to native code [v12]

[Quan Anh Mai]

On Fri, 27 Oct 2023 03:49:17 GMT, Yasumasa Suenaga  wrote:

>> src/hotspot/cpu/x86/downcallLinker_x86_64.cpp line 110:
>> 
>>> 108:   __ mov(rsp, r12); // restore sp
>>> 109:   __ reinit_heapbase();
>>> 110: }
>> 
>> This is a minor cleanup to share this code for the three use sites below.
>
> Question: `r12` does not need to remember?
> 
> According to [CallingSequences in OpenJDK 
> Wiki](https://wiki.openjdk.org/display/HotSpot/CallingSequences), `r12` may 
> be reserved for HeapBase if COOP is enabled.  
> (`r12` is also used in another places in downcallLinker_x86_64.cpp without 
> restoring...)

@YaSuenag `r12` is restored in `reinit_heapbase()` if needed and no, `r12` does 
not need remembering because it is a constant and can be restored from 
somewhere else.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16201#discussion_r1374370475

Re: RFR: 8305734: BitSet.get(int, int) always returns the empty BitSet when the Integer.MAX VALUE is set

[null]

On Sat, 22 Jul 2023 04:44:21 GMT, Stuart Marks  wrote:

>> See https://bugs.java.com/bugdatabase/view_bug?bug_id=8305734 and 
>> https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8311905
>
> Hi, thanks for the background and the discussion of the alternatives. I'm not 
> sure that drafting the CSR is the right next step; there are a number of 
> foibles about editing the specifications that come into play when preparing 
> the CSR. However, what you've written above is a good design discussion about 
> the different alternatives and how they affect the specification and the 
> implementation.
> 
> One thing that we need to consider is compatibility. From an excessively 
> pedantic standpoint, any change in behavior is a potential incompatibility 
> (somebody might be relying on that bug!) but I think that fixing obviously 
> incorrect behavior is reasonable. As much as possible I'd like to make sure 
> that things that worked, at least partially, continue to work.
> 
> With this in mind I'm leaning toward allowing a BitSet to contain bits 
> including the Integer.MAX_VALUE bit, and adjusting various specs accordingly. 
> I think the best way to specify this is to say that length() returns an int 
> in the range [0, 2^31] as an unsigned value. In practice of course this means 
> it can return any Java `int` value in the range [0, MAX_VALUE] and also 
> Integer.MIN_VALUE. A sufficiently careful programmer can use such values 
> correctly, as long as they avoid doing certain things such as comparing 
> against zero. (We might want to have a note in the spec to that effect.)
> 
> It's good that you analyzed the `valueOf` methods. It looks to me like the 
> implementation will store an actual array potentially containing bits beyond 
> the MAX_VALUE bit, and this will affect things like length() and size() and 
> such bits won't be accessible via get() or set(). So, on the one hand, this 
> behavior seems clearly broken and ought to be fixed by limiting the input 
> array along the lines suggested by your three options.
> 
> On the other hand, it seems that from looking at the code, it's possible to 
> create an "over-size" BitSet with valueOf(), and perform bulk bit 
> manipulations on it and other BitSets using methods like and(), or(), and 
> xor(). It also appears possible to read out the bits successfully using 
> toByteArray() or toLongArray(). Thus an application might be able to 
> manipulate bit arrays of up to about 2^37 bits long by using BitSet with long 
> arrays or LongBuffers. Restricting the input of valueOf() to 2^31 bits would 
> break such applications.
> 
> Also note that the specification says the bits are numbered by nonnegative 
> integers (that is, zero to 2^31) which would seem to preclude longer bit 
> arrays. However, if somebody...

Hey @stuart-marks (cc @liach @AlanBateman @ExE-Boss),
I think this discussion has stalled a bit. Would it be an idea to at least push 
this commit through, so the get(i,j) bug at least is fixed, and then maybe move 
the larger discussion to a different place? (Eg. a pull request for 
[JDK-8311905](https://bugs.openjdk.org/browse/JDK-8311905)?)
I don't believe the CSR is _essential_ to fix the get(i,j) issue, and clearly 
there is some disagreement on what the right solution is.
If we do end up changing the spec for the BitSet, there will almost certainly 
be larger changes in the class than just the get(i,j) method.

-

PR Comment: https://git.openjdk.org/jdk/pull/13388#issuecomment-1782673878

Re: RFR: 8305734: BitSet.get(int, int) always returns the empty BitSet when the Integer.MAX VALUE is set

[Alan Bateman]

On Sat, 22 Jul 2023 04:44:21 GMT, Stuart Marks  wrote:

>> See https://bugs.java.com/bugdatabase/view_bug?bug_id=8305734 and 
>> https://bugs.java.com/bugdatabase/view_bug?bug_id=JDK-8311905
>
> Hi, thanks for the background and the discussion of the alternatives. I'm not 
> sure that drafting the CSR is the right next step; there are a number of 
> foibles about editing the specifications that come into play when preparing 
> the CSR. However, what you've written above is a good design discussion about 
> the different alternatives and how they affect the specification and the 
> implementation.
> 
> One thing that we need to consider is compatibility. From an excessively 
> pedantic standpoint, any change in behavior is a potential incompatibility 
> (somebody might be relying on that bug!) but I think that fixing obviously 
> incorrect behavior is reasonable. As much as possible I'd like to make sure 
> that things that worked, at least partially, continue to work.
> 
> With this in mind I'm leaning toward allowing a BitSet to contain bits 
> including the Integer.MAX_VALUE bit, and adjusting various specs accordingly. 
> I think the best way to specify this is to say that length() returns an int 
> in the range [0, 2^31] as an unsigned value. In practice of course this means 
> it can return any Java `int` value in the range [0, MAX_VALUE] and also 
> Integer.MIN_VALUE. A sufficiently careful programmer can use such values 
> correctly, as long as they avoid doing certain things such as comparing 
> against zero. (We might want to have a note in the spec to that effect.)
> 
> It's good that you analyzed the `valueOf` methods. It looks to me like the 
> implementation will store an actual array potentially containing bits beyond 
> the MAX_VALUE bit, and this will affect things like length() and size() and 
> such bits won't be accessible via get() or set(). So, on the one hand, this 
> behavior seems clearly broken and ought to be fixed by limiting the input 
> array along the lines suggested by your three options.
> 
> On the other hand, it seems that from looking at the code, it's possible to 
> create an "over-size" BitSet with valueOf(), and perform bulk bit 
> manipulations on it and other BitSets using methods like and(), or(), and 
> xor(). It also appears possible to read out the bits successfully using 
> toByteArray() or toLongArray(). Thus an application might be able to 
> manipulate bit arrays of up to about 2^37 bits long by using BitSet with long 
> arrays or LongBuffers. Restricting the input of valueOf() to 2^31 bits would 
> break such applications.
> 
> Also note that the specification says the bits are numbered by nonnegative 
> integers (that is, zero to 2^31) which would seem to preclude longer bit 
> arrays. However, if somebody...

> Hey @stuart-marks (cc @liach @AlanBateman @ExE-Boss), I think this discussion 
> has stalled a bit. Would it be an idea to at least push this commit through.

No, as I don't think there is agreement yet on this direction. Also all of the 
options involve a spec change.

-

PR Comment: https://git.openjdk.org/jdk/pull/13388#issuecomment-1782681710

Re: RFR: 8254693: Add Panama feature to pass heap segments to native code [v12]

[Martin Doerr]

On Fri, 27 Oct 2023 10:23:43 GMT, Quan Anh Mai  wrote:

>> Question: `r12` does not need to remember?
>> 
>> According to [CallingSequences in OpenJDK 
>> Wiki](https://wiki.openjdk.org/display/HotSpot/CallingSequences), `r12` may 
>> be reserved for HeapBase if COOP is enabled.  
>> (`r12` is also used in another places in downcallLinker_x86_64.cpp without 
>> restoring...)
>
> @YaSuenag `r12` is restored in `reinit_heapbase()` if needed and no, `r12` 
> does not need remembering because it is a constant and can be restored from 
> somewhere else.

I think your code is fine. Restoring `r12_heapbase` at this point is not bad 
because `runtime_call` is only for slow paths. I don't think it should be moved.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16201#discussion_r1374436854

Re: RFR: 8316662: Remove one allocation per conversion in Double.toString(double) and Float.toString(float) [v3]

[Raffaello Giulietti]

> By correctly sizing an intermediate `byte[]` and making use of the internal 
> `newStringNoRepl()` method, one allocation per conversion can be avoided when 
> the runtime uses compact strings.

Raffaello Giulietti 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 three additional 
commits since the last revision:

 - Merge branch 'master' into 8316662
 - Uppercase JLA.
 - 8316662: Remove one allocation per conversion in Double.toString(double) and 
Float.toString(float)

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/15861/files
  - new: https://git.openjdk.org/jdk/pull/15861/files/092c2046..87e09f38

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=15861&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15861&range=01-02

  Stats: 66648 lines in 2489 files changed: 41106 ins; 11953 del; 13589 mod
  Patch: https://git.openjdk.org/jdk/pull/15861.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/15861/head:pull/15861

PR: https://git.openjdk.org/jdk/pull/15861

Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v9]

[null]

On Wed, 20 Sep 2023 16:33:56 GMT, Paul Sandoz  wrote:

>>> > Alan, you mentioned that DualPivotQuicksort will need detailed review. 
>>> > Can we go ahead and start reviewing? Laurent checked performance, JMH 
>>> > results look fine.
>>> 
>>> As before, I think the main question with this change is whether adding 
>>> radix sort to the mix is worth the complexity and additional code to 
>>> maintain. Also as we discussed in the previous PR, the additional memory 
>>> needed for the radix sort may have an effect on other things that are going 
>>> on concurrently. I know it has been updated to handle OOME but I think 
>>> potential reviewers would need to be comfortable with that part.
>> 
>> I too share concerns about the potential increased use of memory for sorting 
>> ints/longs/floats/doubles. With modern SIMD hardware and data parallel 
>> techniques we can apply quicksort much more efficiently. I think it is 
>> important to determine to what extent this reduces the need for radix sort. 
>> To determine that we would need to carefully measure against the AVX-512 
>> implementation (with ongoing improvements) with appropriately initialized 
>> data to sort, and further measure against an AVX2 version.
>
>> Hi Paul (@PaulSandoz), Alan (@AlanBateman), Any update? Do you agree with 
>> Radix sort in parallel case only?
> 
> I think its definitely a better fit, but another aspect of my previous 
> comment was wondering if we need a radix sort if the vectorized quicksort 
> implementation is fast enough. IMO we need to compare performance results 
> with the vectorized quick sort, and be aware of future enhancements to that.

Hello @PaulSandoz and @AlanBateman !

Did you have time to look at the latest benchmarking?

-

PR Comment: https://git.openjdk.org/jdk/pull/13568#issuecomment-1782915551

Update on JEP-461: Stream Gatherers (Preview)

[Viktor Klang]

Greetings,

As you may have already seen, Stream Gatherers is now a Preview JEP with 
Candidate status

Work-in-progress (interfaces, implementation, tests, benches, and 
documentation) for JEP-461 is currently available 
here.

While the design has held up 
well, there are some important improvements made since the original design 
document was written.

Notable changes (without any particular order):

  *   Stream::gather() now has a default implementation.

  *   Gatherer::supplier() was renamed to Gatherer::initializer() to better 
reflect intent.

  *   Gatherer.Sink was renamed to Gatherer.Downstream to better signal 
what it represents.

  *   Gatherer::collect(Collector) and its companion type 
Gatherer.ThenCollector was dropped due to compatibility concerns with existing 
code which operates on Collector.

  *
Gatherer.Characteristics have been eliminated and superseded by having default 
values that are used as sentinels.
 (discussed further down the list)
This is important because with the Characteristics-model keeping alignment 
between Characteristics and actual implementation proved brittle, and under 
composition of Gatherers computing the union was inefficient and mostly lead to 
an empty set in the end.

  *   Gatherer.defaultInitializer(), Gatherer.defaultCombiner(), and 
Gatherer.defaultFinisher() were added as static methods—these are the sentinels 
used to elide calling the initializer, to elide calling the combiner (avoid 
parallelization), and to elide calling the finisher, respectively.

  *   Gatherer::initializer(), Gatherer::combiner(), and Gatherer::finisher() 
default implementations return the respective sentinels.

  *   A subtype of Integrator named Greedy was added, together with a factory 
method to guide lambda-to-nominal-type conversion. This allows creators of 
Gatherers to signal that an Integrator will never initiate a short-circuit (but 
may relay one from downstream), and that is available during evaluation to 
determine if the operation can short-circuit or not.

  *   Factories for creating anonymous Gatherers were expanded upon to include 
Gatherer.of() and Gatherer.ofSequential() with different sets of parameters, 
primarily to make it more ergonomical and easier to read and write the code 
using those factories.

  *   A curated set of initial built-in Gatherers is located in 
java.util.stream.Gatherers

I recently presented Gatherers at 
Devoxx, which I'd recommend 
watching for an introduction to the feature.
[cid:d048f613-b144-4ff9-a2d3-98e0765aab57]
Teaching old Streams new tricks By Viktor 
Klang
Have you ever wanted to perform an operation on a java.util.stream.Stream only 
to find that the existing set of operations didn't provide what you 
needed—forcing you to break out early from the Stream and perform the logic 
outside of it? As a matter of fact, java.util.stream was the first JDK API 
designed with lambdas in mind and was ...
www.youtube.com
  

Cheers,
√


Viktor Klang
Software Architect, Java Platform Group
Oracle

Re: RFR: 8316969: Improve CDS module graph support for --module option [v3]

[Alan Bateman]

On Thu, 26 Oct 2023 21:08:07 GMT, Calvin Cheung  wrote:

> I've pushed another update with the following changes:
> 
> * in the VM code, skip archiving full module graph if there's an incubator 
> module by checking if the ArchivedBootLayer::archivedBootLayer is available;

I think we still have the issue when the archive is created with a main module 
(meaning a named module) but you run with the class path as the initial module, 
e.g. java -version. Is the update missing the changes to ArchivedModuleGraph as 
I think that will need to check that the main module matches the archive.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374624923

Re: RFR: 8254693: Add Panama feature to pass heap segments to native code [v12]

[Yasumasa Suenaga]

On Fri, 27 Oct 2023 11:28:29 GMT, Martin Doerr  wrote:

>> @YaSuenag `r12` is restored in `reinit_heapbase()` if needed and no, `r12` 
>> does not need remembering because it is a constant and can be restored from 
>> somewhere else.
>
> I think your code is fine. Restoring `r12_heapbase` at this point is not bad 
> because `runtime_call` is only for slow paths. I don't think it should be 
> moved.

Thanks everyone! I understood `r12` is restored in `reinit_heapbase()`. It is 
necessary. I have no comments.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16201#discussion_r1374678254

Re: RFR: 8318586: Explicitly handle upcall stub allocation failure [v5]

[Jorn Vernee]

> Explicitly handle UpcallStub allocation failures by terminating. We currently 
> might try to use the returned `nullptr` which would fail sooner or later. 
> This patch just makes the termination explicit.

Jorn Vernee has updated the pull request incrementally with one additional 
commit since the last revision:

  add nullptr checks to other platforms

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16311/files
  - new: https://git.openjdk.org/jdk/pull/16311/files/52e875eb..a75d072d

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16311&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16311&range=03-04

  Stats: 51 lines in 8 files changed: 51 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/16311.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16311/head:pull/16311

PR: https://git.openjdk.org/jdk/pull/16311

Re: RFR: 8318839: Update test thread factory to catch all exceptions

[Leonid Mesnik]

On Fri, 27 Oct 2023 05:55:43 GMT, David Holmes  wrote:

>> It is still used in tests and we should ignore it like jtreg doing.
>
> Shouldn't this code first retrieve the current default exception handler, and 
> then check whether t is a virtual thread, and if so handle the exception as 
> appropriate (not sure System.exit is appropriate ..). Then for non-virtual 
> threads it delegates to the previous default handler.
> 
> final UncaughtExceptionHandler originalUEH = 
> Thread.getDefaultUncaughtExceptionHandler();
> Thread.setDefaultUncaughtExceptionHandler((t, e) -> {
> if (t.isVirtual()) {
> // ...
> } else {
> originalUEH.uncaughtException(t, e);
> }
> });

There shouldn't be an original UHE. jtreg doesn't set it. The goal is to add 
this handler for all threads, not only virtual. 
Please note, that it is planned to add it only until the similar problem in 
jtreg is completely fixed. 
I thought to add something like 

Thread.setDefaultUncaughtExceptionHandler((t, e) -> {
  UHE.ecxeptionThrown = e;
}

...

 @Override
public Thread newThread(Runnable task) {
return VIRTUAL_TF.newThread(new Runnable() -> {
task.run();
if (UHE.ecxeptionThrown != null) {
 throw new RuntimeException(UHE.ecxeptionThrown);
}
);
}
}


So test actually throws exceptions. It might miss exceptions for threads that 
finish later than the main thread. but might it is ok.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16369#discussion_r1374790707

Re: RFR: 8316969: Improve CDS module graph support for --module option [v3]

[Calvin Cheung]

On Fri, 27 Oct 2023 13:57:38 GMT, Alan Bateman  wrote:

>> I've pushed another update with the following changes:
>> 
>> - in the VM code, skip archiving full module graph if there's an incubator 
>> module by checking if the ArchivedBootLayer::archivedBootLayer is available;
>> - included your suggested code changes in ModuleBootstrap.java;
>> - added a test scenario with an incubator module.
>
>> I've pushed another update with the following changes:
>> 
>> * in the VM code, skip archiving full module graph if there's an incubator 
>> module by checking if the ArchivedBootLayer::archivedBootLayer is available;
> 
> I think we still have the issue when the archive is created with a main 
> module (meaning a named module) but you run with the class path as the 
> initial module, e.g. java -version. Is the update missing the changes to 
> ArchivedModuleGraph as I think that will need to check that the main module 
> matches the archive.

I reran the script you sent me few days ago and got the expected results with 
the latest changes.
The checking of the main module name matches between dump time and runtime is 
performed in the VM code. If an archive (even the default CDS archive 
classes.jsa) is created with the -m option, running with just `java -version` 
will disable the optimized module handling.

[0.136s][info ][cds] _archived_main_module_name m
[0.136s][info ][cds] Module m specified during dump time but not during 
runtime
[0.136s][info ][cds] Disabling optimized module handling
[0.136s][info ][cds] optimized module handling: disabled
[0.136s][info ][cds] full module graph: disabled

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374796890

Re: RFR: 8316969: Improve CDS module graph support for --module option [v6]

[Ioi Lam]

On Thu, 26 Oct 2023 21:24:46 GMT, Calvin Cheung  wrote:

>> Please review this changeset for adding support for `--module` (-m) option 
>> for CDS.
>> Changes in the `ModuleBootstrap.java` are needed so that the 
>> `ArchivedModuleGraph.archive` and `ArchivedBootLayer.archive` are called if 
>> the main module is specified. The module name will be stored in the ro 
>> region of the CDS archive. During runtime, the archived module name will be 
>> compared with the runtime module name. If comparison fails, the archived 
>> full module graph won't be used.
>> 
>> Note: this RFE is a subtask of 
>> [JDK-8266329](https://bugs.openjdk.org/browse/JDK-8266329). More subtask(s) 
>> will be created to support other options such as `--add-modules`.
>> 
>> Passed tiers 1 - 4 testing.
>
> Calvin Cheung has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains ten commits:
> 
>  - Merge master
>  - skip archiving full module graph is there is an incubator module
>  - fix typo
>  - simplify some code in modules.cpp
>  - comments from Alan and Ioi; add missing @run tag in the test
>  - Merge branch 'master' into improve-CDS-module-graph
>  - better way to check if a module is a JDK module
>  - initial review comments from Ioi
>  - 8316969: Improve CDS module graph support for --module option

The new version looks really good now. I found one merge error and have a 
suggestion for adding one more test.

src/hotspot/share/cds/metaspaceShared.cpp line 790:

> 788:   ArchiveHeapWriter::init();
> 789:   if (use_full_module_graph()) {
> 790: HeapShared::reset_archived_object_states(CHECK);

This block of code needs to be inside `if (CDSConfig::is_dumping_heap())`

test/hotspot/jtreg/runtime/cds/appcds/jigsaw/module/ModuleOption.java line 107:

> 105: oa.shouldHaveExitValue(0)
> 106:   // module graph won't be archived with an incubator module
> 107:   .shouldContain("archivedBootLayer not available, disabling 
> full module graph");

For thoroughness, I think we should also check for this log in heapShared.cpp:


if (record->is_full_module_graph() && 
!CDSConfig::is_loading_full_module_graph()) {
  if (log_is_enabled(Info, cds, heap)) {
ResourceMark rm(THREAD);
log_info(cds, heap)("subgraph %s cannot be used because full module 
graph is disabled",
k->external_name());
  }
  return nullptr;
}

-

Changes requested by iklam (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16016#pullrequestreview-1702150002
PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374801536
PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374805801

Re: RFR: JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method [v3]

[Justin Lu]

> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8317630) 
> which makes the implications of overriding _ChoiceFormat::applyPattern_ and 
> _MessageFormat::applyPattern_ apparent by adding a implSpec tag to the 
> method. 
> 
> That is, the constructors of both ChoiceFormat and MessageFormat call the 
> public and non-final applyPattern method; any changes in the applyPattern of 
> a subclass will be reflected in the constructors as well.

Justin Lu has updated the pull request incrementally with two additional 
commits since the last revision:

 - additional cleanup/wording changes
 - replace spec fix with private methods

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16064/files
  - new: https://git.openjdk.org/jdk/pull/16064/files/dc9cbae6..74847121

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16064&range=02
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16064&range=01-02

  Stats: 64 lines in 2 files changed: 35 ins; 13 del; 16 mod
  Patch: https://git.openjdk.org/jdk/pull/16064.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16064/head:pull/16064

PR: https://git.openjdk.org/jdk/pull/16064

Re: RFR: 8316969: Improve CDS module graph support for --module option [v3]

[Alan Bateman]

On Fri, 27 Oct 2023 16:23:04 GMT, Calvin Cheung  wrote:

> I reran the script you sent me few days ago and got the expected results with 
> the latest changes. The checking of the main module name matches between dump 
> time and runtime is performed in the VM code. If an archive (even the default 
> CDS archive classes.jsa) is created with the -m option, running with just 
> `java -version` will disable the optimized module handling.
> 
> ```
> [0.136s][info ][cds] _archived_main_module_name m
> [0.136s][info ][cds] Module m specified during dump time but not 
> during runtime
> [0.136s][info ][cds] Disabling optimized module handling
> [0.136s][info ][cds] optimized module handling: disabled
> [0.136s][info ][cds] full module graph: disabled
> ```

Can you check that ArchivedModuleGraph.archivedModuleGraph is null when 
disabled? When I tried it, it wasn't null so ModuleBootstrap has a module graph 
containing the modules for m.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374875540

Re: RFR: JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method [v3]

[Naoto Sato]

On Fri, 27 Oct 2023 17:31:43 GMT, Justin Lu  wrote:

>> Please review this PR which updates ChoiceFormat and MessageFormat to no 
>> longer call overridable methods in their constructors.
>> 
>> The overridable methods called in the constructors are: 
>> _ChoiceFormat::applyPattern_, _ChoiceFormat::setChoices_, and 
>> _MessageFormat::applyPattern_. The code should be updated so that both the 
>> methods and constructors call a separate private method. Some other drive-by 
>> cleanup changes were included in the change as well.
>
> Justin Lu has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - additional cleanup/wording changes
>  - replace spec fix with private methods

src/java.base/share/classes/java/text/ChoiceFormat.java line 560:

> 558:  * @see #previousDouble
> 559:  */
> 560: public static double nextDouble (double d) {

Is removing `final` OK here? Wouldn't this allow defining the static method in 
the subclass?

src/java.base/share/classes/java/text/ChoiceFormat.java line 575:

> 573:  * @see #nextDouble
> 574:  */
> 575: public static double previousDouble (double d) {

same here.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16064#discussion_r1374892774
PR Review Comment: https://git.openjdk.org/jdk/pull/16064#discussion_r1374892964

Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v4]

[Joe Wang]

> Add a new factory method so that a CatalogResolver can be created with a 
> resolve property on top of the Catalog object.

Joe Wang 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 four additional commits since the 
last revision:

 - Merge branch 'master' into JDK-8316996
   Merge before update
 - add a test for the NPE case
 - addressing review comments, plus use URL for relative URI
 - 8316996: Catalog API Enhancement: add a factory method

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16045/files
  - new: https://git.openjdk.org/jdk/pull/16045/files/f1d726ec..a0b5d4a8

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16045&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16045&range=02-03

  Stats: 46092 lines in 1883 files changed: 27518 ins; 9505 del; 9069 mod
  Patch: https://git.openjdk.org/jdk/pull/16045.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16045/head:pull/16045

PR: https://git.openjdk.org/jdk/pull/16045

Re: RFR: JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method [v3]

[Justin Lu]

On Fri, 27 Oct 2023 18:00:38 GMT, Naoto Sato  wrote:

>> Justin Lu has updated the pull request incrementally with two additional 
>> commits since the last revision:
>> 
>>  - additional cleanup/wording changes
>>  - replace spec fix with private methods
>
> src/java.base/share/classes/java/text/ChoiceFormat.java line 560:
> 
>> 558:  * @see #previousDouble
>> 559:  */
>> 560: public static double nextDouble (double d) {
> 
> Is removing `final` OK here? Wouldn't this allow defining the static method 
> in the subclass?

Right, we don't want those methods to now have the ability to be hidden. Got 
carried away with the IDE suggested tips. Reverted here and in the other 
occurrence, thanks for correcting.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16064#discussion_r1374928055

Re: RFR: JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method [v4]

[Justin Lu]

> Please review this PR which updates ChoiceFormat and MessageFormat to no 
> longer call overridable methods in their constructors.
> 
> The overridable methods called in the constructors are: 
> _ChoiceFormat::applyPattern_, _ChoiceFormat::setChoices_, and 
> _MessageFormat::applyPattern_. The code should be updated so that both the 
> methods and constructors call a separate private method. Some other drive-by 
> cleanup changes were included in the change as well.

Justin Lu has updated the pull request incrementally with one additional commit 
since the last revision:

  re-apply 'final' to the static methods in ChoiceFormat

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16064/files
  - new: https://git.openjdk.org/jdk/pull/16064/files/74847121..a44ea641

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16064&range=03
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16064&range=02-03

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/16064.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16064/head:pull/16064

PR: https://git.openjdk.org/jdk/pull/16064

Re: RFR: JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method [v4]

[Naoto Sato]

On Fri, 27 Oct 2023 18:40:56 GMT, Justin Lu  wrote:

>> Please review this PR which updates ChoiceFormat and MessageFormat to no 
>> longer call overridable methods in their constructors.
>> 
>> The overridable methods called in the constructors are: 
>> _ChoiceFormat::applyPattern_, _ChoiceFormat::setChoices_, and 
>> _MessageFormat::applyPattern_. The code should be updated so that both the 
>> methods and constructors call a separate private method. Some other drive-by 
>> cleanup changes were included in the change as well.
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   re-apply 'final' to the static methods in ChoiceFormat

Marked as reviewed by naoto (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/16064#pullrequestreview-1702372815

Re: RFR: 8316969: Improve CDS module graph support for --module option [v3]

[Calvin Cheung]

On Fri, 27 Oct 2023 17:40:12 GMT, Alan Bateman  wrote:

>> I reran the script you sent me few days ago and got the expected results 
>> with the latest changes.
>> The checking of the main module name matches between dump time and runtime 
>> is performed in the VM code. If an archive (even the default CDS archive 
>> classes.jsa) is created with the -m option, running with just `java 
>> -version` will disable the optimized module handling.
>> 
>> [0.136s][info ][cds] _archived_main_module_name m
>> [0.136s][info ][cds] Module m specified during dump time but not 
>> during runtime
>> [0.136s][info ][cds] Disabling optimized module handling
>> [0.136s][info ][cds] optimized module handling: disabled
>> [0.136s][info ][cds] full module graph: disabled
>
>> I reran the script you sent me few days ago and got the expected results 
>> with the latest changes. The checking of the main module name matches 
>> between dump time and runtime is performed in the VM code. If an archive 
>> (even the default CDS archive classes.jsa) is created with the -m option, 
>> running with just `java -version` will disable the optimized module handling.
>> 
>> ```
>> [0.136s][info ][cds] _archived_main_module_name m
>> [0.136s][info ][cds] Module m specified during dump time but not 
>> during runtime
>> [0.136s][info ][cds] Disabling optimized module handling
>> [0.136s][info ][cds] optimized module handling: disabled
>> [0.136s][info ][cds] full module graph: disabled
>> ```
> 
> Can you check that ArchivedModuleGraph.archivedModuleGraph is null when 
> disabled? When I tried it, it wasn't null so ModuleBootstrap has a module 
> graph containing the modules for m.

The `ArchivedModuleGraph.java` wasn't changed. So if `-m` is not specified, the 
`archivedModuleGraph` is non-null; if `-m` is specified, the 
`archivedModuleGraph` is null.
So running `java -version`, the archivedModuleGraph is non-null, but the module 
m won't be loaded from the archive. I'm seeing the following log output:

`[0.220s][debug][module ] define_module(): creation of module: m, version: 
null, location: jrt:/m, loader data: 0x7f201819dd60 for instance a 
'jdk/internal/loader/ClassLoaders$AppClassLoader'{0x00011f010500}, package 
#: 1`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1374951369

Re: RFR: JDK-8317612: ChoiceFormat and MessageFormat constructors call non-final public method [v4]

[Lance Andersen]

On Fri, 27 Oct 2023 18:40:56 GMT, Justin Lu  wrote:

>> Please review this PR which updates ChoiceFormat and MessageFormat to no 
>> longer call overridable methods in their constructors.
>> 
>> The overridable methods called in the constructors are: 
>> _ChoiceFormat::applyPattern_, _ChoiceFormat::setChoices_, and 
>> _MessageFormat::applyPattern_. The code should be updated so that both the 
>> methods and constructors call a separate private method. Some other drive-by 
>> cleanup changes were included in the change as well.
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   re-apply 'final' to the static methods in ChoiceFormat

Looks reasonable.  I assume you validated the links work in the generated docs 
as a sanity check

-

Marked as reviewed by lancea (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16064#pullrequestreview-1702392861

Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v5]

[Joe Wang]

> Add a new factory method so that a CatalogResolver can be created with a 
> resolve property on top of the Catalog object.

Joe Wang has updated the pull request incrementally with one additional commit 
since the last revision:

  Impl CSR Changes: change the parameter type from String to Enum

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16045/files
  - new: https://git.openjdk.org/jdk/pull/16045/files/a0b5d4a8..89b6b7ea

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16045&range=04
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16045&range=03-04

  Stats: 168 lines in 7 files changed: 72 ins; 46 del; 50 mod
  Patch: https://git.openjdk.org/jdk/pull/16045.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16045/head:pull/16045

PR: https://git.openjdk.org/jdk/pull/16045

Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v5]

[Lance Andersen]

On Fri, 27 Oct 2023 19:16:33 GMT, Joe Wang  wrote:

>> Add a new factory method so that a CatalogResolver can be created with a 
>> resolve property on top of the Catalog object.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Impl CSR Changes: change the parameter type from String to Enum

Marked as reviewed by lancea (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/16045#pullrequestreview-1702399673

Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v5]

[Naoto Sato]

On Fri, 27 Oct 2023 19:20:01 GMT, Joe Wang  wrote:

>> Add a new factory method so that a CatalogResolver can be created with a 
>> resolve property on top of the Catalog object.
>
> Joe Wang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Impl CSR Changes: change the parameter type from String to Enum

LGTM

src/java.xml/share/classes/javax/xml/catalog/CatalogResolver.java line 283:

> 281: return type;
> 282: }
> 283: }

Instead of looping through all the elements, `Enum.valueOf()` can be used (and 
it appropriately throws IAE if not found).

-

Marked as reviewed by naoto (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16045#pullrequestreview-1702510763
PR Review Comment: https://git.openjdk.org/jdk/pull/16045#discussion_r1375025707

Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v6]

[Joe Wang]

> Add a new factory method so that a CatalogResolver can be created with a 
> resolve property on top of the Catalog object.

Joe Wang has updated the pull request incrementally with one additional commit 
since the last revision:

  add IAE

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/16045/files
  - new: https://git.openjdk.org/jdk/pull/16045/files/89b6b7ea..f7095aa1

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=16045&range=05
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16045&range=04-05

  Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod
  Patch: https://git.openjdk.org/jdk/pull/16045.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/16045/head:pull/16045

PR: https://git.openjdk.org/jdk/pull/16045

Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v5]

[Joe Wang]

On Fri, 27 Oct 2023 20:41:59 GMT, Naoto Sato  wrote:

>> Joe Wang has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Impl CSR Changes: change the parameter type from String to Enum
>
> src/java.xml/share/classes/javax/xml/catalog/CatalogResolver.java line 283:
> 
>> 281: return type;
>> 282: }
>> 283: }
> 
> Instead of looping through all the elements, `Enum.valueOf()` can be used 
> (and it appropriately throws IAE if not found).

Thanks. Added IAE. Enum.valueOf() is nice. However, in this case, the Enum type 
is mapped to the resolve property that was defined to be case sensitive. It 
would therefore require validating the case and then convert it to upper case 
before calling valueOf. While the current impl requires loop, it uses 
toString() that's mapped to the resolve property.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16045#discussion_r1375090816

Re: RFR: 8316996: Catalog API Enhancement: add a factory method [v5]

[Naoto Sato]

On Fri, 27 Oct 2023 22:21:41 GMT, Joe Wang  wrote:

>> src/java.xml/share/classes/javax/xml/catalog/CatalogResolver.java line 283:
>> 
>>> 281: return type;
>>> 282: }
>>> 283: }
>> 
>> Instead of looping through all the elements, `Enum.valueOf()` can be used 
>> (and it appropriately throws IAE if not found).
>
> Thanks. Added IAE. Enum.valueOf() is nice. However, in this case, the Enum 
> type is mapped to the resolve property that was defined to be case sensitive. 
> It would therefore require validating the case and then convert it to upper 
> case before calling valueOf. While the current impl requires loop, it uses 
> toString() that's mapped to the resolve property.

That is correct. Thanks for checking

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16045#discussion_r1375099347

Integrated: 8316996: Catalog API Enhancement: add a factory method

[Joe Wang]

On Wed, 4 Oct 2023 22:56:28 GMT, Joe Wang  wrote:

> Add a new factory method so that a CatalogResolver can be created with a 
> resolve property on top of the Catalog object.

This pull request has now been integrated.

Changeset: 96bec358
Author:Joe Wang 
URL:   
https://git.openjdk.org/jdk/commit/96bec3584ced3ea1e75cc40bb402f571aba78b09
Stats: 325 lines in 9 files changed: 277 ins; 32 del; 16 mod

8316996: Catalog API Enhancement: add a factory method

Reviewed-by: naoto, lancea

-

PR: https://git.openjdk.org/jdk/pull/16045

Re: RFR: 8316969: Improve CDS module graph support for --module option [v3]

[Alan Bateman]

On Fri, 27 Oct 2023 19:03:36 GMT, Calvin Cheung  wrote:

> The `ArchivedModuleGraph.java` wasn't changed. So if `-m` is not specified, 
> the `archivedModuleGraph` is non-null; if `-m` is specified, the 
> `archivedModuleGraph` is null. So running `java -version`, the 
> archivedModuleGraph is non-null, but the module m won't be loaded from the 
> archive.

Right, but ArchivedModuleGraph provides the initial value for 
hasIncubatorModules, hasSplitPackages, ... This is why `java -version` 
confusingly prints a warning that an incubator module has been resolved. We've 
had archiving of the module graph (configuration) for a long time but it's only 
been for the case where the initial module is the unnamed module. I think 
archiving of the boot layer for the case where the initial module is a named 
module means we have to re-visit ArchivedModuleGraph. I think 
ArchivedModuleGraph.get has to return null when the archive doesn't match the 
mainModule. Part of it wonder if we should remove ArchivedModuleGraph, support 
archiving of the boot layer only.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16016#discussion_r1375183933