Re: RFR: 8335366: Improve String.format performance with fastpath [v12]

[Roger Riggs]

On Mon, 1 Jul 2024 19:17:50 GMT, Shaojin Wen  wrote:

>> We need a String format solution with good performance. String Template was 
>> once expected, but it has been removed. j.u.Formatter is powerful, but its 
>> performance is not good enough.
>> 
>> This PR implements a subset of j.u.Formatter capabilities. The performance 
>> is good enough that it is a fastpath for commonly used functions. When the 
>> supported functions are exceeded, it will fall back to using j.u.Formatter.
>> 
>> The performance of this implementation is good enough, the fastpath has low 
>> detection cost, There is no noticeable performance degradation when falling 
>> back to j.u.Formatter via fastpath.
>> 
>> Below is a comparison of String.format and concat-based and StringBuilder:
>> 
>> * benchmark java code
>> 
>> public class StringFormat {
>> @Benchmark
>> public String stringIntFormat() {
>> return "%s %d".formatted(s, i);
>> }
>> 
>> @Benchmark
>> public String stringIntConcat() {
>> return s + " " + i;
>> }
>> 
>> @Benchmark
>> public String stringIntStringBuilder() {
>> return new StringBuilder(s).append(" ").append(i).toString();
>> }
>> }
>> 
>> 
>> * benchmark number on macbook m1 pro
>> 
>> BenchmarkMode  Cnt   Score   Error  Units
>> StringFormat.stringIntConcat avgt   15   6.541 ? 0.056  ns/op
>> StringFormat.stringIntFormat avgt   15  17.399 ? 0.133  ns/op
>> StringFormat.stringIntStringBuilder  avgt   15   8.004 ? 0.063  ns/op
>> 
>> 
>> From the above data, we can see that the implementation of fastpath reduces 
>> the performance difference between String.format and StringBuilder from 10 
>> times to 2~3 times.
>> 
>> The implementation of fastpath supports the following four specifiers, which 
>> can appear at most twice and support a width of 1 to 9.
>> 
>> d
>> x
>> X
>> s
>> 
>> If necessary, we can add a few more.
>> 
>> 
>> Below is a comparison of performance numbers running on a MacBook M1, 
>> showing a significant performance improvement.
>> 
>> -Benchmark  Mode  CntScoreError  Units 
>> (baseline)
>> -StringFormat.complexFormat avgt   15  895.954 ? 52.541  ns/op
>> -StringFormat.decimalFormat avgt   15  277.420 ? 18.254  ns/op
>> -StringFormat.stringFormat  avgt   15   66.787 ?  2.715  ns/op
>> -StringFormat.stringIntFormat   avgt   15   81.046 ?  1.879  ns/op
>> -StringFormat.widthStringFormat avgt   15   38.897 ?  0.114  ns/op
>> -StringFormat.widthStringIntFormat  avgt   15  109.841 ?  1.028  ns/op
>> 
>> +Benchmark...
>
> Shaojin Wen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   improve StringFormat benchmark

I have serious concerns about going forward with this optimization. 
It creates duplicate and fragile code that becomes a maintenance overhead.
It reduces the performance of the non-covered cases and does nothing for the 
other cases using Formatter.

-

PR Comment: https://git.openjdk.org/jdk/pull/19956#issuecomment-2203237835

Re: RFR: 8334490: Normalize string with locale invariant `toLowerCase()`

[Roger Riggs]

On Tue, 18 Jun 2024 18:23:12 GMT, Naoto Sato  wrote:

> The test library `jdk.test.lib.Platform` uses no-arg `toLowerCase()` for 
> string comparison, which should be avoided.

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19775#pullrequestreview-2128480919

Re: RFR: 8309821: Link to hidden classes section in Class specification for Class::isHidden

[Roger Riggs]

On Wed, 19 Jun 2024 02:21:05 GMT, Joe Darcy  wrote:

> Simple doc improvement.

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19781#pullrequestreview-2128478464

Re: RFR: 8317611: Add a tool like jdeprscan to find usage of restricted methods

[Roger Riggs]

On Tue, 18 Jun 2024 16:30:37 GMT, Jorn Vernee  wrote:

> This PR adds a new JDK tool, called `jnativescan`, that can be used to find 
> code that accesses native functionality. Currently this includes `native` 
> method declarations, and methods marked with `@Restricted`.
> 
> The tool accepts a list of class path and module path entries through 
> `--class-path` and `--module-path`, and a set of root modules through 
> `--add-modules`, as well as an optional target release with `--release`.
> 
> The default mode is for the tool to report all uses of `@Restricted` methods, 
> and `native` method declaration in a tree-like structure:
> 
> 
> app.jar (ALL-UNNAMED):
>   main.Main:
> main.Main::main(String[])void references restricted methods:
>   java.lang.foreign.MemorySegment::reinterpret(long)MemorySegment
> main.Main::m()void is a native method declaration
> 
> 
> The `--print-native-access` option can be used print out all the module names 
> of modules doing native access in a comma separated list. For class path 
> code, this will print out `ALL-UNNAMED`.
> 
> Testing: 
> - `langtools_jnativescan` tests.
> - Running the tool over jextract's libclang bindings, which use the FFM API, 
> and thus has a lot of references to `@Restricted` methods.
> - tier 1-3

One more tool. or...
Could this be coalesced into a tool that does deprscan and restricted methods, 
and other "lint" type checks?
I might go so far as to suggest it be extensible and accept patterns or regular 
expressions for matching classes, methods, fields, etc.

-

PR Comment: https://git.openjdk.org/jdk/pull/19774#issuecomment-2178860810

Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v3]

[Roger Riggs]

On Wed, 12 Jun 2024 20:48:41 GMT, Chen Liang  wrote:

>> Please review this patch that fixes a critical issue that breaks some Proxy 
>> usages.
>> 
>> Since the problematic patch from before cannot be backed out, this patch 
>> aims to emulate the old behavior before. A diff between before the 
>> problematic patch and this patch is available at 
>> https://gist.github.com/7565b2091008f561eb0ada019bc5e517, generated by 
>> running `git diff 326dbb1b139dd1ec1b8605339b91697cdf49da9a -- 
>> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java`.
>
> Chen Liang has updated the pull request incrementally with three additional 
> commits since the last revision:
> 
>  - Fix bad stack map frame
>  - Forgot to write field
>  - Try to align with previous code

Once a PR is in review doing a bunch of renames hides the real change in a 
bunch of inconsequential changes and can extend the review.
The "Ce" suffix would read better as "CE".

src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 95:

> 93: 
> 94: private static final String
> 95: NAME_LOOKUP_ACCESSOR = "proxyClassLookup";

Join these if there's only 1.

-

Changes requested by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19615#pullrequestreview-2114238459
PR Review Comment: https://git.openjdk.org/jdk/pull/19615#discussion_r1637104525

Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v2]

[Roger Riggs]

On Mon, 10 Jun 2024 04:08:41 GMT, Chen Liang  wrote:

>> Please review this patch that fixes a critical issue that breaks some Proxy 
>> usages.
>> 
>> CONSTANT_Class and CONSTANT_MethodType must fail resolution for inaccessible 
>> package-private types per JVMS 5.4.3.1 and 5.4.3.5, yet such types can 
>> appear in method descriptors in the same class. The proposed way to bypass 
>> is to store the MethodType as its descriptor string in the bootstrap method 
>> arguments, and use MethodType.fromMethodDescriptorString to restore the 
>> arguments, which does not have this restriction and does not eagerly load 
>> the parameter classes. This case isn't covered by existing tests, so a new 
>> test has been added.
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Obtain classloader in security manager friendly code path

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19615#pullrequestreview-2114166690

Re: RFR: 8333854: IllegalAccessError with proxies after JDK-8332457 [v2]

[Roger Riggs]

On Mon, 10 Jun 2024 04:08:41 GMT, Chen Liang  wrote:

>> Please review this patch that fixes a critical issue that breaks some Proxy 
>> usages.
>> 
>> CONSTANT_Class and CONSTANT_MethodType must fail resolution for inaccessible 
>> package-private types per JVMS 5.4.3.1 and 5.4.3.5, yet such types can 
>> appear in method descriptors in the same class. The proposed way to bypass 
>> is to store the MethodType as its descriptor string in the bootstrap method 
>> arguments, and use MethodType.fromMethodDescriptorString to restore the 
>> arguments, which does not have this restriction and does not eagerly load 
>> the parameter classes. This case isn't covered by existing tests, so a new 
>> test has been added.
>
> Chen Liang has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Obtain classloader in security manager friendly code path

This look ok.

The odd mix of coding styles and mixing of names (CD_) between static imports 
and local declarations is annoying but is pre-existing noise. It works better 
for constants than for unqualified method calls (for example, 
`ofConstantBootstrap`, what class is that in.)
Generally, its a bad idea to assume someone reading code is using an IDE,
source constructs that are not easily searchable or navigable with text tools 
make it harder to maintain.

-

PR Review: https://git.openjdk.org/jdk/pull/19615#pullrequestreview-2111455328

Re: RFR: 8222884: ConcurrentClassDescLookup.java times out intermittently

[Roger Riggs]

On Tue, 11 Jun 2024 13:09:21 GMT, Jaikiran Pai  wrote:

> Can I please get a review of this test-only change which proposes to address 
> intermittent failures in 
> `test/jdk/java/io/Serializable/concurrentClassDescLookup/ConcurrentClassDescLookup.java`?
> 
> This addresses https://bugs.openjdk.org/browse/JDK-8222884. As noted in that 
> issue, the test in its current form has a potential race. The test main 
> thread launches 50 "successful lookup" tasks and 50 "failed lookup" tasks. 
> Each task is a `Thread`. The test passes an `Object` instance shared by 50 of 
> these tasks and each task when it starts execution in its `run()` method does 
> a `wait()` on that `Object` instance. The test main thread then after 
> launching 50 tasks, sleeps for a second (to let the 50 `Thread`s start 
> execution and arrive at the `Object.wait()`) and then after waking up the 
> main thread does a `notifyAll()` on that `Object` instance. In theory and 
> likely in practice too, it's possible that (especially) for the last batch of 
> 50 tasks (the "failing lookup" tasks), the `notifyAll()` might end up getting 
> invoked before one or more task Thread(s) have actually done a `wait()`. That 
> can then mean that one or more task `Thread`(s) will keep `wait()`ing 
> indefinitely with no on
 e `notify()`ing the `Object` instance to proceed with the task execution.
> 
> The commit in this PR removes the use of `Object.wait()/notifyAll()` and 
> instead uses a `CountdownLatch` to allow for a more deterministic barrier. 
> The test continues to pass with this change. Additionally, this change was 
> run against a CI setup, by Matthias, where it was previously failing 
> intermittently. Matthias reports that it hasn't failed after this change.

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19659#pullrequestreview-2110524082

Re: RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259 [v2]

[Roger Riggs]

On Mon, 10 Jun 2024 16:28:39 GMT, Daniel Jeliński  wrote:

>> `GetExitCodeProcess` method is not reliable for checking if a process exited 
>> already; it returns 259 (STILL_ACTIVE) if the process hasn't exited yet, but 
>> the same value is returned when the process exited with code 259. In order 
>> to check if the process exited, we need to check if its handle is in a 
>> signaled state using one of the wait methods.
>> 
>> This PR fixes the onExit, exitValue, isAlive, and waitFor(timeout) methods 
>> to correctly handle the problematic exit code.
>> 
>> I haven't fixed the ProcessImpl.toString method. I'm not sure the problem is 
>> important enough to justify an extra JNI call in the (probably typical) 
>> still-alive case.
>> 
>> Tier1-3 testing clean. I modified the existing OnExitTest to cover this case.
>
> Daniel Jeliński has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   waitForProcessExit0 should ignore interrupts

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19586#pullrequestreview-2108626204

Re: RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259 [v2]

[Roger Riggs]

On Mon, 10 Jun 2024 16:24:27 GMT, Daniel Jeliński  wrote:

>> Another reason to retain the checking of GetThreadInterruptEvent is to be 
>> belt and suspenders against the Java code changing and opening up a 
>> potential error. At the moment, the reaper thread is encapsulated and not 
>> likely to get an interrupt, but that might not always be the case and 
>> someone changing the Java code might be unaware of the native code details.  
>> $.02
>
> I did some research, and now I'm pretty confident that WaitForSingleObject is 
> the right option:
> - the thread involved here is a daemon thread, i.e. it doesn't prevent JVM 
> exit
> - the Unix version of `waitForProcessExit0` ignores interrupts and signals, 
> it only ends when the wait succeeds or fails
> - I didn't observe any problems with exiting the JVM while waiting on a 
> process handle
> - it's not clear to me what action we would take if the thread was cancelled; 
> completing a CompletableFuture with an InterruptedException doesn't feel 
> right.

ok, I'll buy that argument

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1633748785

Re: RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259

[Roger Riggs]

On Fri, 7 Jun 2024 21:13:56 GMT, Roger Riggs  wrote:

>> Interesting. Could you point me to a related bug / documentation / evidence? 
>> I tested the WaitForSingleObject version (commit 
>> 6524747a5ebc51c760b14e90309c5f18b58b20e2) on Windows 11, and I didn't 
>> observe any problems with shutting down the VM
>
> I don't have a test or counter example and don't have a detailed 
> understanding of the blocking needs of Hotspot.

Another reason to retain the checking of GetThreadInterruptEvent is to be belt 
and suspenders against the Java code changing and opening up a potential error. 
At the moment, the reaper thread is encapsulated and not likely to get an 
interrupt, but that might not always be the case and someone changing the Java 
code might be unaware of the native code details.  $.02

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1633285990

Re: RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259

[Roger Riggs]

On Fri, 7 Jun 2024 20:56:03 GMT, Daniel Jeliński  wrote:

>> I think waiting on JVM_GetThreadInterruptEvent() is necessary during VM 
>> shutdown to allow the blocked thread to exit cleanly.
>
> Interesting. Could you point me to a related bug / documentation / evidence? 
> I tested the WaitForSingleObject version (commit 
> 6524747a5ebc51c760b14e90309c5f18b58b20e2) on Windows 11, and I didn't observe 
> any problems with shutting down the VM

I don't have a test or counter example and don't have a detailed understanding 
of the blocking needs of Hotspot.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631701117

Re: RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259

[Roger Riggs]

On Fri, 7 Jun 2024 20:52:38 GMT, Daniel Jeliński  wrote:

>> The `WaitForSingleObject(handle, 0)` seems like an indirect way to determine 
>> if the process is alive.
>> Whereas `isProcessAlive(handle)` seems to directly answer the question.
>
> sorry, I still don't get it. Are you telling me that I should use 
> isProcessAlive to implement isProcessAlive? Because it doesn't look like a 
> WinAPI function name,

I take it back.
It was just wishful thinking that Windows had the obvious API.
And I've intermingled Java methods for native methods.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631681720

Re: RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259

[Roger Riggs]

On Fri, 7 Jun 2024 16:05:06 GMT, Daniel Jeliński  wrote:

>> src/java.base/windows/native/libjava/ProcessHandleImpl_win.c line 128:
>> 
>>> 126: JNU_ThrowByNameWithLastError(env,
>>> 127: "java/lang/RuntimeException", 
>>> "WaitForMultipleObjects");
>>> 128: } else if (!GetExitCodeProcess(handle, )) {
>> 
>> This can return STILL_ACTIVE if there is a spurious thread interrupt.
>> The interrupt is presumably present to keep the thread from being stuck in a 
>> blocking windows os call.
>> 
>> The calling code in ProcessHandleImpl is agnostic to platform and would 
>> report the process had exited.
>> 
>> Can the risk of incorrectly reporting the process exit be mitigated?
>> 
>> If the Thread is legitimately been interrupted, Thread.interrupted() would 
>> be true and the reaper could exit.
>> If false, it could retry the waitForProcessExit().
>
> I only left it here because the wait for interrupt event was already present. 
> Is being stuck in a blocking os call a bad thing? If not, I can drop the 
> interrupt event, and wait on the process handle only.

I think waiting on JVM_GetThreadInterruptEvent() is necessary during VM 
shutdown to allow the blocked thread to exit cleanly.

>> src/java.base/windows/native/libjava/ProcessImpl_md.c line 471:
>> 
>>> 469: {
>>> 470: return WaitForSingleObject((HANDLE) handle, 0) /* don't wait */
>>> 471:== WAIT_TIMEOUT;
>> 
>> Would this be better as `isProcessAlive(handle)`?
>
> I don't follow. Could you explain?

The `WaitForSingleObject(handle, 0)` seems like an indirect way to determine if 
the process is alive.
Whereas `isProcessAlive(handle)` seems to directly answer the question.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631664185
PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631662973

Re: RFR: 8333742: ProcessImpl and ProcessHandleImpl may mishandle processes that exit with code 259

[Roger Riggs]

On Thu, 6 Jun 2024 18:40:51 GMT, Daniel Jeliński  wrote:

> `GetExitCodeProcess` method is not reliable for checking if a process exited 
> already; it returns 259 (STILL_ACTIVE) if the process hasn't exited yet, but 
> the same value is returned when the process exited with code 259. In order to 
> check if the process exited, we need to check if its handle is in a signaled 
> state using one of the wait methods.
> 
> This PR fixes the onExit, exitValue, isAlive, and waitFor(timeout) methods to 
> correctly handle the problematic exit code.
> 
> I haven't fixed the ProcessImpl.toString method. I'm not sure the problem is 
> important enough to justify an extra JNI call in the (probably typical) 
> still-alive case.
> 
> Tier1-3 testing clean. I modified the existing OnExitTest to cover this case.

src/java.base/windows/native/libjava/ProcessHandleImpl_win.c line 128:

> 126: JNU_ThrowByNameWithLastError(env,
> 127: "java/lang/RuntimeException", "WaitForMultipleObjects");
> 128: } else if (!GetExitCodeProcess(handle, )) {

This can return STILL_ACTIVE if there is a spurious thread interrupt.
The interrupt is presumably present to keep the thread from being stuck in a 
blocking windows os call.

The calling code in ProcessHandleImpl is agnostic to platform and would report 
the process had exited.

Can the risk of incorrectly reporting the process exit be mitigated?

If the Thread is legitimately been interrupted, Thread.interrupted() would be 
true and the reaper could exit.
If false, it could retry the waitForProcessExit().

src/java.base/windows/native/libjava/ProcessImpl_md.c line 471:

> 469: {
> 470: return WaitForSingleObject((HANDLE) handle, 0) /* don't wait */
> 471:== WAIT_TIMEOUT;

Would this be better as `isProcessAlive(handle)`?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631346916
PR Review Comment: https://git.openjdk.org/jdk/pull/19586#discussion_r1631322282

Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor} [v6]

[Roger Riggs]

On Fri, 31 May 2024 18:39:33 GMT, jengebr  wrote:

>> Improve `java/lang/reflect/Method.java`  by eliminating needless cloning of 
>> Class[0] instances.  This cloning is intended to prevent callers from 
>> changing array contents, but many Methods have zero exceptions or zero 
>> parameters, and returning the original `Class[0]` is sufficient.
>> 
>> Results from the included JMH benchmark:
>> Before:
>> 
>> BenchmarkMode  Cnt  Score   Error  Units
>> ConstructorBenchmark.getExceptionTypes   avgt5  6.526 ± 0.183  ns/op
>> ConstructorBenchmark.getExceptionTypesEmpty  avgt5  5.803 ± 0.073  ns/op
>> ConstructorBenchmark.getParameterTypes   avgt5  6.521 ± 0.188  ns/op
>> ConstructorBenchmark.getParameterTypesEmpty  avgt5  5.747 ± 0.087  ns/op
>> MethodBenchmark.getExceptionTypesavgt5  6.525 ± 0.163  ns/op
>> MethodBenchmark.getExceptionTypesEmpty   avgt5  5.783 ± 0.130  ns/op
>> MethodBenchmark.getParameterTypesavgt5  6.518 ± 0.195  ns/op
>> MethodBenchmark.getParameterTypesEmpty   avgt5  5.742 ± 0.028  ns/op
>> 
>> 
>> After:
>> 
>> BenchmarkMode  Cnt  Score   Error  Units
>> ConstructorBenchmark.getExceptionTypes   avgt5  6.590 ± 0.124  ns/op
>> ConstructorBenchmark.getExceptionTypesEmpty  avgt5  1.351 ± 0.061  ns/op
>> ConstructorBenchmark.getParameterTypes   avgt5  6.651 ± 0.132  ns/op
>> ConstructorBenchmark.getParameterTypesEmpty  avgt5  1.353 ± 0.150  ns/op
>> MethodBenchmark.getExceptionTypesavgt5  6.701 ± 0.151  ns/op
>> MethodBenchmark.getExceptionTypesEmpty   avgt5  1.422 ± 0.025  ns/op
>> MethodBenchmark.getParameterTypesavgt5  6.629 ± 0.142  ns/op
>> MethodBenchmark.getParameterTypesEmpty   avgt5  1.273 ± 0.169  ns/op
>
> jengebr has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Fixing nits in benchmark

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2091692284

Re: RFR: 8332586: Avoid cloning empty arrays in java.lang.reflect.{Method,Constructor}

[Roger Riggs]

On Tue, 21 May 2024 13:49:18 GMT, jengebr  wrote:

> Improve `java/lang/reflect/Method.java`  by eliminating needless cloning of 
> Class[0] instances.  This cloning is intended to prevent callers from 
> changing array contents, but smany Methods have zero exceptions or zero 
> parameters, and returning the original `Class[0]` is sufficient.

LGTM;  the localized change is concise and clear as to why the clone is 
unnecessary.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19327#pullrequestreview-2089235024

Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null [v3]

[Roger Riggs]

On Fri, 24 May 2024 07:24:13 GMT, Matthias Baesken  wrote:

>> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing 
>> jtreg tests afterwards I run into this error :
>> 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime 
>> error: null pointer passed as argument 2, which is declared to never be null
>> #0 0x7fd95bec78d8 in spawnChild 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562
>> #1 0x7fd95bec78d8 in startChild 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612
>> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712
>> #3 0x7fd93797a06d ()
>> 
>> this is the memcpy call getting an unexpected null pointer :
>> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null.
>> Something similar was discussed and fixed here 
>> https://bugs.python.org/issue27570 for Python .
>> 
>> Similar issue in OpenJDK _ 
>> https://bugs.openjdk.org/browse/JDK-8332473
>> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed 
>> as argument 1, which is declared to never be null
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   handle special case that memcpy src is NULL but a len larger than 0 was 
> given

LGTM  (Freeing the buffer on fail)

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19329#pullrequestreview-2077014965

Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null [v2]

[Roger Riggs]

On Thu, 23 May 2024 07:26:14 GMT, Matthias Baesken  wrote:

>> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing 
>> jtreg tests afterwards I run into this error :
>> 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime 
>> error: null pointer passed as argument 2, which is declared to never be null
>> #0 0x7fd95bec78d8 in spawnChild 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562
>> #1 0x7fd95bec78d8 in startChild 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612
>> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec 
>> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712
>> #3 0x7fd93797a06d ()
>> 
>> this is the memcpy call getting an unexpected null pointer :
>> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null.
>> Something similar was discussed and fixed here 
>> https://bugs.python.org/issue27570 for Python .
>> 
>> Similar issue in OpenJDK _ 
>> https://bugs.openjdk.org/browse/JDK-8332473
>> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed 
>> as argument 1, which is declared to never be null
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   remarks Roger Riggs

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19329#pullrequestreview-2074031918

Re: RFR: 8332589: ubsan: unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: null pointer passed as argument 2, which is declared to never be null

[Roger Riggs]

On Tue, 21 May 2024 14:28:38 GMT, Matthias Baesken  wrote:

> When building with ubsan enabled (--enable-uban) on Linux x86_64 and doing 
> jtreg tests afterwards I run into this error :
> 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562:5: runtime error: 
> null pointer passed as argument 2, which is declared to never be null
> #0 0x7fd95bec78d8 in spawnChild 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:562
> #1 0x7fd95bec78d8 in startChild 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:612
> #2 0x7fd95bec78d8 in Java_java_lang_ProcessImpl_forkAndExec 
> /jdk/src/java.base/unix/native/libjava/ProcessImpl_md.c:712
> #3 0x7fd93797a06d ()
> 
> this is the memcpy call getting an unexpected null pointer :
> memcpy(buf+offset, c->pdir, sp.dirlen); gets a second parameter null.
> Something similar was discussed and fixed here 
> https://bugs.python.org/issue27570 for Python .
> 
> Similar issue in OpenJDK _ 
> https://bugs.openjdk.org/browse/JDK-8332473
> 8332473: ubsan: growableArray.hpp:290:10: runtime error: null pointer passed 
> as argument 1, which is declared to never be null

src/java.base/unix/native/libjava/ProcessImpl_md.c line 565:

> 563: memcpy(buf+offset, c->pdir, sp.dirlen);
> 564: }
> 565: offset += sp.dirlen;

I'd be inclined to check sp.dirlen > 0 in the `if` and move the offset += 
inside too. Like:
Suggestion:

if (sp.dirlen > 0 && c->pdir != NULL) {
memcpy(buf+offset, c->pdir, sp.dirlen);
offset += sp.dirlen;
}

The behavior is correct either way.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19329#discussion_r1610170951

Integrated: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

[Roger Riggs]

On Wed, 1 May 2024 18:43:21 GMT, Roger Riggs  wrote:

> The issue reported a ClassCastException "cannot assign instance of 
> java.util.CollSer to field of type java.util.Map"
> while deserializing an object referring to an immutable Map that contained a 
> reference to a class that was not available.
> Immutable Collections such as Map utilize a serialization proxy in their 
> serialized form.
> During deserialization the serialization proxy (a private implementation 
> class) was attempted to be set in a field resulting in the 
> ClassCastException. The ClassCastException and bug hid the ClassCastException 
> that should have been thrown.
> 
> When reading record fields or fields of a class, the results of 
> deserialization of individual fields are recorded as dependencies of the 
> object being constructed.
> The apparent bug is that the summary of those dependencies is not checked 
> between reading the fields and invoking the constructor to create the record 
> or assigning the fields to an object being constructed.

This pull request has now been integrated.

Changeset: 8291c94b
Author:Roger Riggs 
URL:   
https://git.openjdk.org/jdk/commit/8291c94bcdbb01beddc94f290f2749841404cc0c
Stats: 199 lines in 2 files changed: 195 ins; 0 del; 4 mod

8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

Reviewed-by: smarks

-

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

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException [v2]

[Roger Riggs]

> The issue reported a ClassCastException "cannot assign instance of 
> java.util.CollSer to field of type java.util.Map"
> while deserializing an object referring to an immutable Map that contained a 
> reference to a class that was not available.
> Immutable Collections such as Map utilize a serialization proxy in their 
> serialized form.
> During deserialization the serialization proxy (a private implementation 
> class) was attempted to be set in a field resulting in the 
> ClassCastException. The ClassCastException and bug hid the ClassCastException 
> that should have been thrown.
> 
> When reading record fields or fields of a class, the results of 
> deserialization of individual fields are recorded as dependencies of the 
> object being constructed.
> The apparent bug is that the summary of those dependencies is not checked 
> between reading the fields and invoking the constructor to create the record 
> or assigning the fields to an object being constructed.

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Add description of exception behavior when reading components in readRecord()

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19043/files
  - new: https://git.openjdk.org/jdk/pull/19043/files/cb9cad62..d2f1db96

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=19043=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=19043=00-01

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

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

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

[Roger Riggs]

On Wed, 1 May 2024 18:43:21 GMT, Roger Riggs  wrote:

> The issue reported a ClassCastException "cannot assign instance of 
> java.util.CollSer to field of type java.util.Map"
> while deserializing an object referring to an immutable Map that contained a 
> reference to a class that was not available.
> Immutable Collections such as Map utilize a serialization proxy in their 
> serialized form.
> During deserialization the serialization proxy (a private implementation 
> class) was attempted to be set in a field resulting in the 
> ClassCastException. The ClassCastException and bug hid the ClassCastException 
> that should have been thrown.
> 
> When reading record fields or fields of a class, the results of 
> deserialization of individual fields are recorded as dependencies of the 
> object being constructed.
> The apparent bug is that the summary of those dependencies is not checked 
> between reading the fields and invoking the constructor to create the record 
> or assigning the fields to an object being constructed.

Thanks for the review.
I have some ideas to refactor readOrdinaryObject to split out the various 
subcases and be able to simplify each one.
That's a separate (future) PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/19043#issuecomment-2118070421

Re: RFR: 8331224: ClassCastException in ObjectInputStream hides ClassNotFoundException

[Roger Riggs]

On Thu, 16 May 2024 21:18:04 GMT, Stuart Marks  wrote:

>> The issue reported a ClassCastException "cannot assign instance of 
>> java.util.CollSer to field of type java.util.Map"
>> while deserializing an object referring to an immutable Map that contained a 
>> reference to a class that was not available.
>> Immutable Collections such as Map utilize a serialization proxy in their 
>> serialized form.
>> During deserialization the serialization proxy (a private implementation 
>> class) was attempted to be set in a field resulting in the 
>> ClassCastException. The ClassCastException and bug hid the 
>> ClassCastException that should have been thrown.
>> 
>> When reading record fields or fields of a class, the results of 
>> deserialization of individual fields are recorded as dependencies of the 
>> object being constructed.
>> The apparent bug is that the summary of those dependencies is not checked 
>> between reading the fields and invoking the constructor to create the record 
>> or assigning the fields to an object being constructed.
>
> src/java.base/share/classes/java/io/ObjectInputStream.java line 2376:
> 
>> 2374: if (handles.lookupException(passHandle) != null) {
>> 2375: return null; // slot marked with exception, don't 
>> create record
>> 2376: }
> 
> It's proper to avoid creating a record instance in this case. However, 
> returning null is new behavior; this initially seemed a bit odd. The calling 
> method `readOrdinaryObject()` does allow a null return if the class couldn't 
> be resolved, and the body of `readOrdinaryObject()` does allow `obj` to be 
> null throughout. So, returning `null` from here is correct, though it took me 
> a while to puzzle out. :-)
> 
> I'd suggest adding some docs to `readRecord()` to indicate that it returns 
> the record instance if it could be created successfully, null if the record 
> class couldn't be resolved, or throws an error or other exception if one 
> occurred when instantiation was attempted.

As you discovered, though it returns null at this point, the return from 
readObject checks for the exception and throws ClassNotFoundException.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19043#discussion_r1605347591

Re: Deterministic naming of subclasses of `java/lang/reflect/Proxy`

[Roger Riggs]

Hi Aman,

You may also run into hidden classes (JEP 371: Hidden Classes) that 
allow classes to be defined, at runtime, without names.
It has been proposed to use them for generated proxies but that hasn't 
been implemented yet.
There are benefits to having nameless classes, because they can't be 
referenced by name, only as a capability, they can be better encapsulated.


fyi, Roger Riggs


On 5/16/24 8:11 AM, Aman Sharma wrote:


Hi,


Thanks for your response, Liang!


> I think you meant CVE-2021-42392 instead of 2022.


Sorry of the error. I indeed meant CVE-2021-42392 
<https://nvd.nist.gov/vuln/detail/cve-2021-42392>.



> Leyden mainly avoids this unstable generation by performing a 
training run to collect classes loaded



Would love to know the details of Project Leyden and how they worked 
so far to focus on this goal. In our case, the training run is the 
test suite.



> GeneratedConstructorAccessor is already retired by JEP 416 [2] in 
Java 18



I did see them not appearing in my allowlist when I ran my study 
subject (Apache PDFBox) with Java 21. Thanks for letting me know about 
this JEP. I see they are re-implemented with method handles.



> How are you checking the classes?


To detect runtime generated code, we have javaagent that is hooked 
statically to the test suite execution. It gives us all classes that 
that is loaded post the JVM and the javaagent are loaded. So we only 
check the classes loaded for the purpose of running the application. 
This is also why we did not choose -agentlib as it would give classes 
for the setting up JVM and javaagent and we the user of our tool must 
the classes they load.



Next, we have a `ClassFileTransformer` hook in the agent where we 
produce the checksum using the bytecode. And we compare the checksum 
with the one existing in the allowlist. The checksum computation 
algorithm is same for both steps. Let me describe how I compute the 
checksum.



 1. I get the CONSTANT_Class_info
<https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-4.html#jvms-4.4.1>
entry corresponding to `this_class` and rewrite the
CONSTANT_Utf8_info
<https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-4.html#jvms-4.4.7>
corresponding to a fix String constant, say "foo".
 2. Since, the name of the class is used to refer to its types members
(fields/method), I get all CONSTANT_Fieldref_info
<https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-4.html#jvms-4.4.2>
and if its `class_index` corresponds to the old `this_class`, we
rewrite the UTF8 value of class_index to the same constant "foo".
 3. Next, since the naming of the fields, in Proxy classes, are also
suffixed by numbers, for example, `private static Method m4`, we
rewrite the UTF8 value of name in the CONSTANT_NameAndType_info

<https://docs.oracle.com/javase/specs/jvms/se11/html/jvms-4.html#jvms-4.4.6>.
 4. These fields can also have a random order so we simply sort the
entire byte code using `Arrays.sort(byte[])` to eliminate any
differences due to ordering of fields/methods.
 5. Simply sorting the byte array still had minute differences. I
could not understand why they existed even though values in
constant pool of the bytecode in allowlist and at runtime were
exactly the same after rewriting. The differences existed in the
bytes of the Code attribute of methods. I concluded that the bytes
stored some position information. To avoid this, I created a
subarray where I considered the bytes corresponding to
`CONSTANT_Utf8_info.bytes` only. Computing a checksum for it
resulted in the same checksums for both classfiles.


Let's understand the whole approach with an example of Proxy class.

`
public  final  class  $Proxy42  extends  Proxy  implements  
org.apache.logging.log4j.core.config.plugins.Plugin  {
`

The will go in the allowlist as "Proxy_Plugin: ".

When the same class is intercepted at runtime, say "$Proxy10", we look 
for "Proxy_Plugin" in the allowlist and since the checksum algorithm 
is same in both cases, we get a match and let the class load.


This approach has seemed to work well for Proxy classes, Generated 
Constructor Accessor (which is removed as you said). I also looked at 
the species generated by method handles. I did not notice any 
modification in them. Their name generation seemed okay to me. If some 
new Species are generated, it is of course detected since it is not in 
the allowlist.


I have not looked into LambdaMetafactory because I did not encounter 
it as a problem so far, but I am aware its name generation is also 
unstable. I have run my approach only a few projects only. And for 
hidden classes, I assume the the agent won't be able to intercept them 
so detecting them would be really hard.



Regards,
Aman Sharma

PhD Student
KTH Royal Institute of Technology
School of Electrical Eng

Re: RFR: 8331646: Add specific regression leap year tests [v5]

[Roger Riggs]

On Fri, 10 May 2024 09:22:39 GMT, serhiysachkov  wrote:

>> Calendar.add() tests that describe its behavior.
>
> serhiysachkov has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   JDK-8331646 fixing typo

A bit late but the descriptions of the problem should not be focused on leap 
years. 
Every add/subtract that involves indefinite month lengths has the same behavior 
when the result is clamped to the length of the month.

-

PR Comment: https://git.openjdk.org/jdk/pull/19082#issuecomment-2104799803

Re: RFR: JDK-8331646: Add specific regression leap year tests (Task - P4)

[Roger Riggs]

On Fri, 3 May 2024 10:31:14 GMT, serhiysachkov  wrote:

> Calendar.add() tests that describe its behavior.

The bug report and/or the PR description should describe the change in more 
detail.
What conditions of the Calendar spec are being tested.  Thanks

-

PR Comment: https://git.openjdk.org/jdk/pull/19082#issuecomment-2096482166

Re: RFR: 8330276: Console methods with explicit Locale [v2]

[Roger Riggs]

On Mon, 29 Apr 2024 23:22:21 GMT, Naoto Sato  wrote:

>> Proposing new overloaded methods in `java.io.Console` class that explicitly 
>> take a `Locale` argument. Existing methods rely on the default locale, so 
>> the users won't be able to format their prompts/outputs in a certain locale 
>> explicitly.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed review comments

lgtm

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18923#pullrequestreview-2038907502

Re: In support of Instant.minus(Instant)

[Roger Riggs]

Hi,

I would also reinforce Stephen's early observation that the pattern for 
"until" methods in java.time includes those of the XXXDate classes, with 
a single Temporal parameter.  Period and Duration are similar values 
holding relative TemporalAmounts.


    public Period until(ChronoLocalDate endDateExclusive)

In addition to Instant, the LocalTime class might also benefit from adding:

public Duration until(LocalTime endExclusive)`

The API design of java.time included an emphasis on consistent naming 
across the packages.


Regards, Roger


On 5/2/24 4:01 PM, Naoto Sato wrote:
`Temporal` interface is clear that its `minus` methods return objects 
of the same `Temporal` type, and `until` calculates the amount of time 
until another `Temporal` type. Introducing `Instant.minus` that 
returns `Duration` would be confusing to me.


Naoto

On 5/2/24 10:41 AM, Éamonn McManus wrote:
I'd say too that this makes intuitive sense based on algebra. If we 
have:

/instant1/ + /duration/ = /instant2/
then we can subtract /duration/ from both sides:
/instant1 = instant2 - duration/
or we can subtract /instant1/ from both sides:
/duration = instant2 - instant1/

There's no manipulation we can do that would cause us to try to add 
instants together, and it's a bit surprising for the API to allow the 
first subtraction but not the second.
I also think that if I see instant2.minus(instant1) it's immediately 
obvious to me what that means, while instant1.until(instant2) seems 
both less discoverable and less obvious.


On Thu, 2 May 2024 at 10:29, Louis Wasserman > wrote:


    That doesn't follow for me at all.

    The structure formed by Instants and Durations is an affine space
, with
    instants the points and durations the vectors.  (An affine space is
    a vector space without a distinguished origin, which of course
    Instants don't have.)  It is 100% standard to use the minus sign for
    the operation "point - point = vector," even when "point + point" is
    not defined, and to use all the other standard idioms for
    subtraction; the Wikipedia article uses "subtraction" and
    "difference" ubiquitously.

    Personally, I'd be willing to live with a different name for the
    operation, but consider "users keep getting it wrong" a strong
    enough argument all by itself for a version with the swapped
    argument order; it's not obvious to me that another API with the
    same argument order adds enough value over Duration.between to
    bother with.

    On Thu, May 2, 2024 at 10:04 AM Stephen Colebourne
    mailto:scolebou...@joda.org>> wrote:

    On Thu, 2 May 2024 at 15:58, Kurt Alfred Kluever mailto:k...@google.com>> wrote:
 > instant − instant = duration // what we're discussing
 > instant + duration = instant // satisfied by
    instant.plus(duration)
 > instant - duration = instant // satisfied by
    instant.minus(duration)
 > duration + duration = duration // satisfied by
    duration.plus(duration)
 > duration - duration = duration // satisfied by
    duration.minus(duration)
 > duration × real number = duration // satisfied by
    duration.multipliedBy(long)
 > duration ÷ real number = duration // satisfied by
    duration.dividedBy(long)
 >
 > All but the first operation have very clear translations from
    conceptual model to code. I'm hoping we can achieve the same
    clarity for instant - instant by using the obvious name:
    instant.minus(instant)

    But you can't have
  instant + instant = ???
    It doesn't make sense.

    This is at the heart of why minus isn't right in this case.
    Stephen



    --     Louis Wasserman (he/they)

Re: RFR: 8331202: Support for Duration until another Instant [v3]

[Roger Riggs]

On Wed, 1 May 2024 17:21:20 GMT, Naoto Sato  wrote:

>> A new method on Instant to return the duration `until` another Instant is 
>> suggested per the following discussion thread:
>> 
>> https://mail.openjdk.org/pipermail/core-libs-dev/2024-April/122131.html
>> 
>> A CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressing CSR reviews

@kluever Your comment isn't visible yet, there's a checkbox to accept OpenJDK 
terms.
It would be good to know what you've commented.

My reservation about `minus(Instant)` is that all of the current `minus(...)` 
methods return an `Instant` as do the `plus(...)` methods.
Adding a `minus` method that returns `Duration` adds a bit of fuzziness to the 
plus and minus method uses.
A similar concern could be raised about adding `Duration until(Instant)` as 
there is already a `until(Temporarl, TemporalUnit)` method. But there is 
smaller opportunity to mix them up.

-

PR Comment: https://git.openjdk.org/jdk/pull/19007#issuecomment-2090693079

Re: RFR: 8331202: Support for Duration until another Instant [v3]

[Roger Riggs]

On Wed, 1 May 2024 17:21:20 GMT, Naoto Sato  wrote:

>> A new method on Instant to return the duration `until` another Instant is 
>> suggested per the following discussion thread:
>> 
>> https://mail.openjdk.org/pipermail/core-libs-dev/2024-April/122131.html
>> 
>> A CSR has also been drafted.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressing CSR reviews

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/19007#pullrequestreview-2034423684

RFR: 8331224: ClassCastException in ObjectStreamClass during deserialization - cannot assign instance of java.util.CollSer to field of type java.util.Map

[Roger Riggs]

The issue reported a ClassCastException "cannot assign instance of 
java.util.CollSer to field of type java.util.Map"
while deserializing an object referring to an immutable Map that contained a 
reference to a class that was not available.
Immutable Collections such as Map utilize a serialization proxy in their 
serialized form.
During deserialization the serialization proxy (a private implementation class) 
was attempted to be set in a field resulting in the ClassCastException. The 
ClassCastException and bug hid the ClassCastException that should have been 
thrown.

When reading record fields or fields of a class, the results of deserialization 
of individual fields are recorded as dependencies of the object being 
constructed.
The apparent bug is that the summary of those dependencies is not checked 
between reading the fields and invoking the constructor to create the record or 
assigning the fields to an object being constructed.

-

Commit messages:
 - 8331224: ClassCastException in ObjectStreamClass during deserialization

Changes: https://git.openjdk.org/jdk/pull/19043/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=19043=00
  Issue: https://bugs.openjdk.org/browse/JDK-8331224
  Stats: 192 lines in 2 files changed: 189 ins; 0 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/19043.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19043/head:pull/19043

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

Re: RFR: 8330276: Console methods with explicit Locale [v2]

[Roger Riggs]

On Mon, 29 Apr 2024 23:22:21 GMT, Naoto Sato  wrote:

>> Proposing new overloaded methods in `java.io.Console` class that explicitly 
>> take a `Locale` argument. Existing methods rely on the default locale, so 
>> the users won't be able to format their prompts/outputs in a certain locale 
>> explicitly.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed review comments

src/java.base/share/classes/java/io/ProxyingConsole.java line 89:

> 87: @Override
> 88: public Console format(String format, Object ... args) {
> 89: return format(Locale.getDefault(Locale.Category.FORMAT), format, 
> args);

Given the number of calls to Locale.getDefault(Locale.Category.FORMAT) it might 
be worthwhile to cache that in the Proxying Console or in the JdkConsoleImpl.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18923#discussion_r1586523255

Re: In support of Instant.minus(Instant)

[Roger Riggs]

A constructive API enhancement.
Created JDK-8331202  
Support for duration between Instants


Regards, Roger

On 4/25/24 4:53 PM, Stephen Colebourne wrote:

java.time.* already has the `until(ChronoLocalDate)` method on
LocalDate. It would be reasonable to add a similar method to various
other classes. This potentially gives you

  Duration dur = start.until(end)

I'm wary of adding the opposite (given until() is already there). I'm
particularly wary of using minus() as the verb for the opposite as
minus() means something different in other parts of the API (minus()
is used to subtract a TemporalAmounrt, not a Temporal).

Stephen


On Thu, 25 Apr 2024 at 19:57, Kurt Alfred Kluever  wrote:

Hi core-libs-dev,

The java.time API already supports subtracting two Instants (end - start) via 
Duration.between(Temporal, Temporal), but we've found the parameter ordering (which 
requires a bit of "mental gymnastics") and API location to be a bit unnatural.

Parameter Ordering

We very often see folks write code like this: Duration elapsed = 
Duration.ofMillis(end.toEpochMilli() - start.toEpochMilli());

This closely matches the mental model of what they're trying to accomplish, but it is 
longer (and more complex) than it needs to be, it drops sub-millisecond precision, and it 
requires decomposing the java.time types (which we strongly discourage). If you want to 
"simplify" the above statement, you must remember to swap the argument order: 
Duration elapsed = Duration.between(start, end);

Many of us find representing (end - start) as between(start, end) to be 
confusing.

API Location

We do not believe Duration is the most obvious place to find this method; if 
you want a way to subtract two Instant values, an instance method on Instant is 
a more obvious place to look. Pushing what could be an instance method to a 
static utility method feels unnatural.

JDK-8276624 (https://bugs.openjdk.org/browse/JDK-8276624) proposes to add 
Temporal.minus(Temporal) as a default method (which would effectively 
accomplish the same thing), but we do not recommend implementing that proposal 
as specified. A default method on Temporal would require runtime exceptions to 
be thrown from other Temporal types like LocalDate or Year. It would also allow 
oddities like instant.minus(year) to compile (but presumably throw at runtime). 
Conceptually, this API would not make sense for certain types (e.g., LocalDate 
— the difference between two LocalDates is a Period, not a Duration).

Instead, we recommend adding a new instance method: instant.minus(instant) 
(which returns a Duration), and possibly also adding localDate.minus(localDate) 
(which returns a Period). However note that we've seen a lot of confusion using 
the Period API (but that's a separate discussion).

While we generally don't like having 2 public APIs that accomplish the same 
thing, in this case we feel the discoverability and simplicity of the new 
API(s) outweighs the cost of an additional public API.

Please consider this a +1 from our team to add instant.minus(instant).

Regards,

-Kurt Alfred Kluever (k...@google.com)
(on behalf of Google's Java and Kotlin Ecosystem Team, aka the Guava team)

Integrated: 8329805: Deprecate for removal ObjectOutputStream.PutField.write

[Roger Riggs]

On Tue, 16 Apr 2024 19:44:36 GMT, Roger Riggs  wrote:

> The method `java.io.ObjectOutputStream.PutField.write` has been deprecated 
> since 1.4 and should be deprecated for removal. The Deprecation annotation is 
> updated to indicate the intention to remov the method.

This pull request has now been integrated.

Changeset: 4dfaa9b5
Author:    Roger Riggs 
URL:   
https://git.openjdk.org/jdk/commit/4dfaa9b5bd4f9733e5a67d7c5b55eaa5ad4e27e4
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8329805: Deprecate for removal ObjectOutputStream.PutField.write

Reviewed-by: naoto, iris

-

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

Re: RFR: 8330624: Inconsistent use of semicolon in the code snippet of java.io.Serializable javadoc

[Roger Riggs]

On Sat, 20 Apr 2024 11:49:30 GMT, Korov  wrote:

> Fix the description of java.io.Serializable.

LGTM; trivial fix of example javadoc code

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18874#pullrequestreview-2022703632

Re: RFR: 8330802: Desugar switch in Locale::createLocale [v2]

[Roger Riggs]

On Mon, 22 Apr 2024 14:11:41 GMT, Claes Redestad  wrote:

>> This switch expression in `Locale::createLocale` is causing a somewhat large 
>> startup regression on my local system. Desugaring to if statements seem like 
>> the right thing to do while we investigate ways to further reduce overheads 
>> in `SwitchBootstraps`.
>> 
>> These numbers are against a baseline which include #18865 and #18845, which 
>> already improved the situation:
>> 
>> 
>> Name Cnt  Base Error   Test 
>> Error Unit  Change
>> Perfstartup-Noop  2040,500 ±   1,942 31,000 ±   
>> 2,673ms/op   1,31x (p = 0,000*)
>>   :.cycles   143254849,000 ± 3398321,355  102205427,650 ± 
>> 2192784,853   cycles   0,71x (p = 0,000*)
>>   :.instructions 307138448,850 ± 2095834,550  219415574,800 ±  
>> 376992,067 instructions   0,71x (p = 0,000*)
>>   :.taskclock   39,500 ±   1,942 22,500 ±   
>> 3,858   ms   0,57x (p = 0,000*)
>>   * = significant
>> 
>> 
>> Comparing to a baseline without those recent improvements the overhead was 
>> almost the double: 
>> 
>> Name Cnt  Base Error   Test 
>> Error Unit  Change
>> Perfstartup-Noop  2050,000 ±   0,000 31,000 ±   
>> 2,673ms/op   1,61x (p = 0,000*)
>>   :.cycles   187047932,000 ± 3330400,381  102205427,650 ± 
>> 2192784,853   cycles   0,55x (p = 0,000*)
>>   :.instructions 408219060,350 ± 4031173,140  219415574,800 ±  
>> 376992,067 instructions   0,54x (p = 0,000*)
>>   :.taskclock   53,500 ±   4,249 22,500 ±   
>> 3,858   ms   0,42x (p = 0,000*)
>>   * = significant
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove InternalError in favor of CCE

LGTM, the CCE is the equivalent of should-not-reach-here.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18882#pullrequestreview-2015014213

Re: RFR: 8329760: Add indexOf(Predicate filter) to java..util.List interface [v9]

[Roger Riggs]

On Fri, 19 Apr 2024 12:59:24 GMT, Evemose  wrote:

>> **Subject**
>> Addition of Predicate-based `indexOf` and `lastIndexOf` methods to 
>> `java.util.List`
>> 
>> **Motivation**
>> The motivation behind this proposal is to enhance the functionality of the 
>> `List` interface by providing a more flexible way to find the index of an 
>> element. Currently, the `indexOf` and `lastIndexOf` methods only accept an 
>> object as a parameter. This limits the flexibility of these methods as they 
>> can only find the index of exact object matches.
>> 
>> The proposed methods would accept a `Predicate` as a parameter, allowing 
>> users to define a condition that the desired element must meet. This would 
>> provide a more flexible and powerful way to find the index of an element in 
>> a list.
>> 
>> Here is a brief overview of the changes made in this pull request:
>> 
>> 1. Added the `indexOf(Predicate filter)` method to the `List` 
>> interface.
>> 2. Added the `lastIndexOf(Predicate filter)` method to the `List` 
>> interface.
>> 3. Implemented these methods in all non-abstract classes that implement the 
>> `List` interface.
>> 
>> The changes have been thoroughly tested to ensure they work as expected and 
>> do not introduce any regressions. The test cases cover a variety of 
>> scenarios to ensure the robustness of the implementation.
>> 
>> For example, consider the following test case:
>> 
>> List list = new ArrayList<>();
>> list.add("Object one");
>> list.add("NotObject two");
>> list.add("NotObject three");
>> 
>> int index1 = list.indexOf(s -> s.contains("ct t"));
>> System.out.println(index1); // Expected output: 1
>> int index2 = list.lastIndexOf(s -> s.startsWith("NotObject"));
>> System.out.println(index2); // Expected output: 2
>> 
>> 
>> Currently, to achieve the same result, we would have to use a more verbose 
>> approach:
>> 
>> int index1 = IntStream.range(0, list.size())
>>  .filter(i -> list.get(i).contains("ct t"))
>>  .findFirst()
>>  .orElse(-1);
>> System.out.println(index1); // Output: 1
>> int index2 = IntStream.range(0, list.size())
>>  .filter(i -> list.get(i).startsWith("NotObject"))
>>  .reduce((first, second) -> second)
>>  .orElse(-1);
>> System.out.println(index2); // Output: 2
>> 
>> 
>> I believe these additions would greatly enhance the functionality and 
>> flexibility of the `List` interface, making it more powerful and 
>> user-friendly. I look forward to your feedback and am open to making any 
>> necessary changes bas...
>
> Evemose has updated the pull request incrementally with one additional commit 
> since the last revision:
> 
>   Update Vector.java

Please revert the formatting as soon as possible; its noise as far as reviewers 
are concerned. 
You want reviewers to focus on the suitability of the API and clarity of the 
specification first, then later on the implementation.  Thanks

-

PR Comment: https://git.openjdk.org/jdk/pull/18639#issuecomment-2066673266

RFR: 8329805: Deprecate for removal ObjectOutputStream.PutField.write

[Roger Riggs]

The method `java.io.ObjectOutputStream.PutField.write` has been deprecated 
since 1.4 and should be deprecated for removal. The Deprecation annotation is 
updated to indicate the intention to remov the method.

-

Commit messages:
 - 8329805: Deprecate for removal ObjectOutputStream.PutField.write

Changes: https://git.openjdk.org/jdk/pull/18802/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18802=00
  Issue: https://bugs.openjdk.org/browse/JDK-8329805
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.org/jdk/pull/18802.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18802/head:pull/18802

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

Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v3]

[Roger Riggs]

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

Marked as reviewed by rriggs (Reviewer).

The CSR is enough, no-one else except the filer will even notice.

-

PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-2004434418
PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2059771876

Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]

[Roger Riggs]

On Tue, 9 Apr 2024 08:34:39 GMT, Jaikiran Pai  wrote:

>> Should `INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER, 
>> ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())),
>> ` work?
>
> Hello Naoto, that's a very good point. I was too caught up with the constant 
> values that it didn't occur to me that the `Instant.MIN` and `Instant.MAX` 
> public fields could be used for this. I have followed your suggestion and 
> updated the PR. I have also updated the javadoc to link to `Instant.MIN` and 
> `Instant.MAX` as the supported epoch second range.
> 
> The test continues to pass with this change and fails (as expected) without 
> the source change.

Good, that was going to be my backup suggestion; trying to avoid a method call 
even for the init. :)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1557657347

Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant [v2]

[Roger Riggs]

On Tue, 9 Apr 2024 08:37:29 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:
> 
>   Naoto's suggestion - use Instant.MIN and Instant.MAX instead of hardcoded 
> values

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18674#pullrequestreview-1989084996

Re: RFR: 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of Instant

[Roger Riggs]

On Mon, 8 Apr 2024 18:00:02 GMT, Naoto Sato  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.
>
> src/java.base/share/classes/java/time/temporal/ChronoField.java line 590:
> 
>> 588:  * This is necessary to ensure interoperation between calendars.
>> 589:  */
>> 590: // ValueRange matches the min and max epoch second supported by 
>> java.time.Instant
> 
> Would we want to include this in the javadoc?

The code should reference the constants in Instant.java (though may need to 
make them package private.)

The javadoc can/should reference Instant.MIN and Instant.MAX (as the test does).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18674#discussion_r1556243941

Integrated: 8329623: NegativeArraySizeException encoding large String to UTF-8

[Roger Riggs]

On Fri, 5 Apr 2024 17:44:38 GMT, Roger Riggs  wrote:

> When encoding a vary large string in String.getBytes(StandardCharset.UTF_8) 
> computation of the buffer size may exceed the range of a positive 32-bit 
> Integer.
> If the estimated size for the result byte array is too large, pre-compute the 
> exact buffer size. 
> If that exceeds the range, then throw OutOfMemoryError.

This pull request has now been integrated.

Changeset: 212a2536
Author:Roger Riggs 
URL:   
https://git.openjdk.org/jdk/commit/212a253697b1a5e722bb90ae1140c91175fc028b
Stats: 78 lines in 2 files changed: 76 ins; 0 del; 2 mod

8329623: NegativeArraySizeException encoding large String to UTF-8

Reviewed-by: naoto, rgiulietti

-

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

Re: RFR: 8329623: NegativeArraySizeException encoding large String to UTF-8

[Roger Riggs]

On Mon, 8 Apr 2024 13:39:34 GMT, Roger Riggs  wrote:

>> test/jdk/java/lang/String/CompactString/MaxSizeUTF16String.java line 143:
>> 
>>> 141: // Strings of size min+1...min+2, throw OOME
>>> 142: // The resulting byte array would exceed implementation limits
>>> 143: for (int count = min + 1; count < max; count++) {
>> 
>> The case `min + 1` cannot lead to a `NegativeArraySizeException` in the 
>> current code, since `3 * (min + 1) <= MAX_VALUE`. In theory, it should 
>> succeed by returning the encoded `byte[]`, although It throws `OOME` for 
>> exceeding VM limits. That is, this case does not trigger the invocation of 
>> `computeSizeUTF8_UTF16()` in the proposed fix.
>> 
>> Only `min + 2` throws `NegativeArraySizeException` in the current code, and 
>> thus the invocation of `computeSizeUTF8_UTF16()` in the proposed fix.
>
> Indeed, different OOMEs are thrown in the two cases triggered by different 
> limits, min +2 is due to integer overflow, while min +1  is due a VM limit on 
> the size of byte[Integer.MAX_VALUE]. Different VM implementations may have 
> different limits on the max size of a byte array.

There might be some merit in lowering the threshold at which an exact size 
computation is triggered.
The oversized allocation "wastes" quite a bit of memory and causes extra GC 
work and usually triggers a second copy of the final size.
However, some guess or heuristic would be needed to choose the threshold at 
which extra cpu work is needed to compute the exact size vs some metric as to 
the "cost" of wasted memory (and saving on the copy).
Most guesses would be somewhat arbitrary; bigger than 1Mb, 1GB, etc? 
Choosing that number would be out of scope for the issue raised by this bug.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18663#discussion_r1555875973

Re: RFR: 8329623: NegativeArraySizeException encoding large String to UTF-8

[Roger Riggs]

On Mon, 8 Apr 2024 08:54:21 GMT, Raffaello Giulietti  
wrote:

>> When encoding a vary large string in String.getBytes(StandardCharset.UTF_8) 
>> computation of the buffer size may exceed the range of a positive 32-bit 
>> Integer.
>> If the estimated size for the result byte array is too large, pre-compute 
>> the exact buffer size. 
>> If that exceeds the range, then throw OutOfMemoryError.
>
> test/jdk/java/lang/String/CompactString/MaxSizeUTF16String.java line 143:
> 
>> 141: // Strings of size min+1...min+2, throw OOME
>> 142: // The resulting byte array would exceed implementation limits
>> 143: for (int count = min + 1; count < max; count++) {
> 
> The case `min + 1` cannot lead to a `NegativeArraySizeException` in the 
> current code, since `3 * (min + 1) <= MAX_VALUE`. In theory, it should 
> succeed by returning the encoded `byte[]`, although It throws `OOME` for 
> exceeding VM limits. That is, this case does not trigger the invocation of 
> `computeSizeUTF8_UTF16()` in the proposed fix.
> 
> Only `min + 2` throws `NegativeArraySizeException` in the current code, and 
> thus the invocation of `computeSizeUTF8_UTF16()` in the proposed fix.

Indeed, different OOMEs are thrown in the two cases triggered by different 
limits, min +2 is due to integer overflow, while min +1  is due a VM limit on 
the size of byte[Integer.MAX_VALUE]. Different VM implementations may have 
different limits. on the max size of a byte array.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18663#discussion_r1555866610

Re: RFR: 8329623: NegativeArraySizeException encoding large String to UTF-8

[Roger Riggs]

On Fri, 5 Apr 2024 20:17:39 GMT, Naoto Sato  wrote:

> LGTM. The test case could be more thorough if it tests strings with 
> supplementary codepoints, as the new method computes them exclusively.

I considered that, but the worst case is the x3 expansion. 
A 2 character high/low surrogate pair would result in 4 bytes of UTF-8, less 
than the 6 bytes needed for a 2 16 bit characters. The test doesn't run quickly 
already due to the large chunks of memory used.

-

PR Comment: https://git.openjdk.org/jdk/pull/18663#issuecomment-2040681452

RFR: 8329623: NegativeArraySizeException encoding large String to UTF-8

[Roger Riggs]

When encoding a vary large string in String.getBytes(StandardCharset.UTF_8) 
computation of the buffer size may exceed the range of a positive 32-bit 
Integer.
If the estimated size for the result byte array is too large, pre-compute the 
exact buffer size. 
If that exceeds the range, then throw OutOfMemoryError.

-

Commit messages:
 - 8329623: NegativeArraySizeException encoding large String to UTF-8

Changes: https://git.openjdk.org/jdk/pull/18663/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18663=00
  Issue: https://bugs.openjdk.org/browse/JDK-8329623
  Stats: 78 lines in 2 files changed: 76 ins; 0 del; 2 mod
  Patch: https://git.openjdk.org/jdk/pull/18663.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18663/head:pull/18663

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

Re: RFR: JDK-8329089: Empty immutable list throws the wrong exception type for remove(first | last) operations

[Roger Riggs]

On Tue, 2 Apr 2024 10:37:02 GMT, Per Minborg  wrote:

> This PR proposes to make empty immutable lists always throw UOE on 
> `removeFirst` and `removeLast`.

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18581#pullrequestreview-1980142511

Re: RFR: JDK-8329089: Empty immutable list throws the wrong exception type for remove(first | last) operations

[Roger Riggs]

On Tue, 2 Apr 2024 10:37:02 GMT, Per Minborg  wrote:

> This PR proposes to make empty immutable lists always throw UOE on 
> `removeFirst` and `removeLast`.

test/jdk/java/util/Collection/MOAT.java line 573:

> 571: c::removeLast);
> 572: }
> 573: 

Would this test better if inserted in `testImmutableCollection(final 
Collection c, T t)`, Line 477'ish.
Or in `testImmutableList(final List c)`, line 519.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18581#discussion_r1549940454

Re: RFR: 8328700: Unused import and variable should be deleted in regex package

[Roger Riggs]

On Fri, 22 Mar 2024 17:58:25 GMT, Raffaello Giulietti  
wrote:

> Make use of an unused local variable probably intended to replace later casts.

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18460#pullrequestreview-195551

Re: CFV: New Core Libraries Group Member: Per-Ake Minborg

[Roger Riggs]

Vote: Yes

On 3/19/24 11:19 AM, Daniel Fuchs wrote:
I hereby nominate Per-Ake Minborg (pminborg) [1] to Membership in the 
Core Libraries Group [4]. 

Re: RFR: 8328524: [x86] StringRepeat.java failure on linux-x86: Could not reserve enough space for 2097152KB object heap

[Roger Riggs]

On Tue, 19 Mar 2024 14:59:19 GMT, Goetz Lindenmaier  wrote:

> …rve enough space for 2097152KB object heap
> 
> I would like to fix this as the two related issues mentioned in the JBS bug.
> We see it currently in most GHA runs.

lgtm

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18380#pullrequestreview-1946547791

Re: RFR: 8328261: public lookup fails with IllegalAccessException when used while module system is being initialized

[Roger Riggs]

On Mon, 18 Mar 2024 17:40:26 GMT, Mandy Chung  wrote:

> A simple fix.   This is caused by a bug in `VerifyAccess::isClassAccessible` 
> that checks if a class is exported from `java.base` before the exports are 
> fully setup.It should check if the module system is fully initialized 
> before checking the module exports instead.

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18356#pullrequestreview-1943799289

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic

[Roger Riggs]

On Tue, 12 Mar 2024 10:44:54 GMT, Yudi Zheng  wrote:

> Moving array construction within BigInteger.implMultiplyToLen intrinsic 
> candidate to its caller simplifies the intrinsic implementation in JIT 
> compiler.

src/java.base/share/classes/java/math/BigInteger.java line 1836:

> 1834: 
> 1835: if (z == null || z.length < (xlen+ ylen))
> 1836:  z = new int[xlen+ylen];

Spaces before and after "+" please.  Tnx

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1528984661

Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v3]

[Roger Riggs]

On Wed, 13 Mar 2024 20:10:25 GMT, Roger Riggs  wrote:

>> The intermittent failure of ObjectStreamClassCaching is due to an incorrect 
>> assumption about GC behavior and a race condition.
>> 
>> Removed test based on incorrect assumptions about simultaneous clearing of 
>> WeakReferences.
>> Added a run of the test using ZGC, (previously omitted)
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Use the correct ZGenerational collector

CI Tier1-3 successful

-

PR Comment: https://git.openjdk.org/jdk/pull/18284#issuecomment-2004334272

Integrated: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1

[Roger Riggs]

On Wed, 13 Mar 2024 18:01:21 GMT, Roger Riggs  wrote:

> The intermittent failure of ObjectStreamClassCaching is due to an incorrect 
> assumption about GC behavior and a race condition.
> 
> Removed test based on incorrect assumptions about simultaneous clearing of 
> WeakReferences.
> Added a run of the test using ZGC, (previously omitted)

This pull request has now been integrated.

Changeset: 85fc47c8
Author:Roger Riggs 
URL:   
https://git.openjdk.org/jdk/commit/85fc47c81af81a595dc88e61454d8ba2d860f301
Stats: 36 lines in 1 file changed: 4 ins; 28 del; 4 mod

8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1

Reviewed-by: iris, stefank

-

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

Re: RFR: 8326941: Remove StringUTF16::isBigEndian

[Roger Riggs]

On Mon, 18 Mar 2024 14:09:09 GMT, Per Minborg  wrote:

> This PR proposes to remove the native method `StringUTF16::isBigEndian` and 
> its corresponding C implementation and instead rely on the 
> `jdk.internal.misc.Unsafe::isBigEndian` method.

Nice Cleanup.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18348#pullrequestreview-1943235796

Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v3]

[Roger Riggs]

> The intermittent failure of ObjectStreamClassCaching is due to an incorrect 
> assumption about GC behavior and a race condition.
> 
> Removed test based on incorrect assumptions about simultaneous clearing of 
> WeakReferences.
> Added a run of the test using ZGC, (previously omitted)

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Use the correct ZGenerational collector

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18284/files
  - new: https://git.openjdk.org/jdk/pull/18284/files/b94fe6d5..55960c41

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18284=02
 - incr: https://webrevs.openjdk.org/?repo=jdk=18284=01-02

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

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

Re: RFR: 8325621: Improve jspawnhelper version checks [v4]

[Roger Riggs]

On Wed, 13 Mar 2024 17:11:25 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 10 commits:
> 
>  - Merge branch 'master' into JDK-8325621-2
>  - Update style and improve error messages
>  - Print to stdout instead of stderr
>  - Compare version using VERSION_STRING
>  - Code cleanup
>  - Remove string.h include
>  - Update test
>  - Pass version as integer instead of string
>  - Read in version using int instead of string
>  - 8325621: Improve jspawnhelper version checks

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18204#pullrequestreview-1935071268

Re: RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 [v2]

[Roger Riggs]

> The intermittent failure of ObjectStreamClassCaching is due to an incorrect 
> assumption about GC behavior and a race condition.
> 
> Removed test based on incorrect assumptions about simultaneous clearing of 
> WeakReferences.
> Added a run of the test using ZGC, (previously omitted)

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Remove obsolete note; no longer disabled

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18284/files
  - new: https://git.openjdk.org/jdk/pull/18284/files/de516a24..b94fe6d5

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18284=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=18284=00-01

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

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

RFR: 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1

[Roger Riggs]

The intermittent failure of ObjectStreamClassCaching is due to an incorrect 
assumption about GC behavior and a race condition.

Removed test based on incorrect assumptions about simultaneous clearing of 
WeakReferences.
Added a run of the test using ZGC, (previously omitted)

-

Commit messages:
 - 8327180: Failed: java/io/ObjectStreamClass/ObjectStreamClassCaching.java#G1 
failed with CONF=release -XX:ReservedCodeCacheSize=8m

Changes: https://git.openjdk.org/jdk/pull/18284/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18284=00
  Issue: https://bugs.openjdk.org/browse/JDK-8327180
  Stats: 36 lines in 1 file changed: 5 ins; 28 del; 3 mod
  Patch: https://git.openjdk.org/jdk/pull/18284.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18284/head:pull/18284

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

Re: RFR: 8327998: Enable java/lang/ProcessBuilder/JspawnhelperProtocol.java on Mac

[Roger Riggs]

On Tue, 12 Mar 2024 21:52:31 GMT, Elif Aslan  wrote:

> This change enables to run JspawnhelperProtocol.java on MacOS.
> 
> In addition to GHA , the test has been run on macos and linux.
> 
> 
> Test report is stored in 
> build/macosx-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR
>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>  1 1 0 0
> ==
> TEST SUCCESS
> 
> Finished building target 'test' in configuration 
> 'macosx-x86_64-server-fastdebug'
> 
> 
> 
> 
> 
> Test report is stored in 
> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
> 
> ==
> Test summary
> ==
>TEST  TOTAL  PASS  FAIL ERROR
>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>  1 1 0 0
> ==
> TEST SUCCESS
> 
> Stopping javac server
> [ec2-user@ip-172-16-0-10 jdk]$ make test CONF=linux-x86_64-server-fastdebug 
> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java

LGTM
Just a reminder to keep that the PR description is copied into every email sent 
so it should be concise.
Extended descriptions and additional details should be added in a separate 
comment.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18253#pullrequestreview-1934219262

Re: RFR: 8325621: Improve jspawnhelper version checks [v2]

[Roger Riggs]

On Mon, 11 Mar 2024 19:12:33 GMT, Chad Rakoczy  wrote:

>> Fix for [8325621](https://bugs.openjdk.org/browse/JDK-8325621)
>> 
>> Updates jspawnhelper to check that JDK version and jspawnhelper version are 
>> the same. Updates test to include check for version. Also tested manually by 
>> replacing jspawnhelper with incorrect version to confirm that check works.
>
> Chad Rakoczy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Code cleanup

make/modules/java.base/Launcher.gmk line 85:

> 83:   -DVERSION_INTERIM=$(VERSION_INTERIM) \
> 84:   -DVERSION_UPDATE=$(VERSION_UPDATE) \
> 85:   -DVERSION_PATCH=$(VERSION_PATCH), \

Using all 4 is way overkill for the problem at hand.  Just the FEATURE_VERSION 
is sufficient.
We all know better than to make incompatible changes in minor versions let 
alone update or patch version.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18204#discussion_r1520364358

Re: RFR: 8327705: Remove mention of "applet" from java.text package description [v2]

[Roger Riggs]

On Mon, 11 Mar 2024 16:32:31 GMT, Naoto Sato  wrote:

>> Removing left over "applet" mention in the package-info doc.
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressing review comments

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18173#pullrequestreview-1928607470

Re: RFR: 8327705: Remove mention of "applet" from java.text package description

[Roger Riggs]

On Fri, 8 Mar 2024 19:54:31 GMT, Naoto Sato  wrote:

> Removing left over "applet" mention in the package-info doc.

bye bye

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18173#pullrequestreview-1925798262

Re: RFR: 8325567: jspawnhelper without args fails with segfault [v8]

[Roger Riggs]

On Fri, 8 Mar 2024 17:24:24 GMT, Elif Aslan  wrote:

>> This change is intended to address the segmentation fault issue that occurs 
>> when jspawnhelper is called without arguments,.
>> There is a new test added  to verify the behavior in such cases.
>> 
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test 
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>> 
>> 
>> 
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>>  1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address comments

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1925740150

Re: RFR: 8327167: Clarify the handling of Leap year by Calendar

[Roger Riggs]

On Thu, 7 Mar 2024 19:28:13 GMT, Naoto Sato  wrote:

> A simple doc update to include a leap day example in the `Calendar` class.

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18158#pullrequestreview-1925260836

Re: RFR: 8325567: jspawnhelper without args fails with segfault [v7]

[Roger Riggs]

On Thu, 7 Mar 2024 17:13:12 GMT, Elif Aslan  wrote:

>> This change is intended to address the segmentation fault issue that occurs 
>> when jspawnhelper is called without arguments,.
>> There is a new test added  to verify the behavior in such cases.
>> 
>> `[ec2-user@ip-172-16-0-10 jdk]$ make CONF=linux-x86_64-server-fastdebug test 
>> TEST=test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java`
>> 
>> 
>> 
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java
>>  1 1 0 0
>> ==
>> TEST SUCCESS
>
> Elif Aslan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add args[0] back

test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 29:

> 27:  * @test
> 28:  * @bug 8325567
> 29:  * @requires (os.family == "linux") | (os.family == "aix")

Unless I'm mistaken, jspawn helper is used on Mac as well.

test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 36:

> 34: import java.nio.file.Paths;
> 35: import java.util.Arrays;
> 36: import java.util.concurrent.TimeUnit;

Unused import.

test/jdk/java/lang/ProcessBuilder/JspawnhelperWarnings.java line 56:

> 54: public static void main(String[] args) throws Exception {
> 55: for (int nArgs = 0; nArgs < 10; nArgs++) {
> 56: tryWithNArgs(nArgs);

Running with more than 3 arguments is unnecessary. Yes, its quick but just 
burns cpu.

When running with 2 arguments, the failure mode is not due to the number of 
arguments but is because argument 1 is formatted incorrectly;  should be 
`"%d:%d:%d"`. Though I supposed this falls into the "incorrect use category".

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516736692
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516738195
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1516733166

Re: RFR: 8327434: Test java/util/PluggableLocale/TimeZoneNameProviderTest.java timed out

[Roger Riggs]

On Wed, 6 Mar 2024 21:32:11 GMT, Naoto Sato  wrote:

> Fixing timeout failures in the test case. Time zone names that are missing 
> (chiefly for minor locales) are now calculated at runtime after the fix to 
> JDK-8174269. This extra calculation time was multiplied in the test case as 
> it iterated over all locales x timezones, which caused timeouts in some 
> configurations. Changed the test case to not iterate all locales, but iterate 
> over relevant ones.

Looks good; will save a lot of of CI time and still cover the cases.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18143#pullrequestreview-1921008397

Re: RFR: JDK-8325255 jdk.internal.util.ReferencedKeySet::add using wrong test [v4]

[Roger Riggs]

On Tue, 5 Mar 2024 20:23:56 GMT, Jim Laskey  wrote:

>> Currently, add is returning intern(e) == null which will always be false. 
>> The correct test is intern(e) == e , that is, true when element is newly 
>> added.
>
> Jim Laskey has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Fix ReferencedKeySet::add

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17732#pullrequestreview-1918184517

Re: RFR: 8325567: jspawnhelper without args fails with segfault

[Roger Riggs]

On Mon, 4 Mar 2024 23:54:38 GMT, Vladimir Petko  wrote:

>> test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 111:
>> 
>>> 109: if (p.exitValue() != 1)
>>> 110: System.exit(ERROR + 2);
>>> 111: System.exit(0);
>> 
>> Its bad form to System.exit from not at the top level. Is that necessary.
>
> This is in line with other tests in this file , e.g. see `parentCode()`. 
> 
> I agree that code would be cleaner if the test is refactored to return error 
> code from the test methods and system.exit() in the main instead, but this 
> might be a separate pr done before or after this one, e.g. something like [1].
> 
> [1] 
> https://github.com/vpa1977/jdk/commit/7cbbaf498da8b71476e61e1c0f3fa7882c7aa7b6

ok, the same pattern is used consistently.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1513262005

Re: RFR: 8325567: jspawnhelper without args fails with segfault

[Roger Riggs]

On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan  wrote:

> This change is intended to address the segmentation fault issue that occurs 
> when jspawnhelper is called without arguments, and it includes an updated 
> test to verify the behavior in such cases.
> 
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> code block
> 
>> Test results: failed: 1
>> Report written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test 
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in 
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>  
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >>   1 0 1 0 
>> >> <<
>> ==
>> TEST FAILURE
> 
> 
> 
> after added the code block back 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> 
> updated test passes
> 
> 
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
>  \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
> 
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
>  \\
> -ea \\
> -esa \\
> 
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
>  \\
> com.sun.javatest.regtest.agent.MainWrapper 
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...

I'm curious why this test is requires `vm.debug`?  That means it generally 
won't get run on release builds.

-

PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1979329508

Re: RFR: 8325567: jspawnhelper without args fails with segfault

[Roger Riggs]

On Mon, 4 Mar 2024 21:19:26 GMT, Elif Aslan  wrote:

> This change is intended to address the segmentation fault issue that occurs 
> when jspawnhelper is called without arguments, and it includes an updated 
> test to verify the behavior in such cases.
> 
> Existing tests passes since it does not check behavior without args.
> After test update the test fails without 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> code block
> 
>> Test results: failed: 1
>> Report written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/html/report.html
>> Results written to 
>> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>> Error: Some tests failed or other problems occurred.
>> Finished running test 
>> 'jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java'
>> Test report is stored in 
>> build/linux-x86_64-server-fastdebug/test-results/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java
>>  
>> ==
>> Test summary
>> ==
>>TEST  TOTAL  PASS  FAIL ERROR 
>>   
>>jtreg:test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java
>> >>   1 0 1 0 
>> >> <<
>> ==
>> TEST FAILURE
> 
> 
> 
> after added the code block back 
> 
>if (argc != 2) {
> shutItDown();
> }
> 
> 
> updated test passes
> 
> 
> lang/ProcessBuilder/JspawnhelperProtocol.d:/home/ec2-user/moe/ws/openjdk/jdk/test/jdk/java/lang/ProcessBuilder:/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/classes/0/test/lib
>  \\
> -Xmx768m \\
> -XX:MaxRAMPercentage=1.04167 \\
> -Dtest.boot.jdk=/home/ec2-user/moe/soft/jdk/jdk-21 \\
> 
> -Djava.io.tmpdir=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/tmp
>  \\
> -ea \\
> -esa \\
> 
> -Djava.library.path=/home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/images/test/jdk/jtreg/native
>  \\
> com.sun.javatest.regtest.agent.MainWrapper 
> /home/ec2-user/moe/ws/openjdk/jdk/build/linux-x86_64-server-fastdebug/test-support/jtreg_test_jdk_java_lang_ProcessBuilder_JspawnhelperProtocol_java/java/lang/ProcessBuilder/JspawnhelperPr...

Changes requested by rriggs (Reviewer).

Need more time to review.

test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 111:

> 109: if (p.exitValue() != 1)
> 110: System.exit(ERROR + 2);
> 111: System.exit(0);

Its bad form to System.exit from not at the top level. Is that necessary.

-

PR Review: https://git.openjdk.org/jdk/pull/18112#pullrequestreview-1915545846
PR Comment: https://git.openjdk.org/jdk/pull/18112#issuecomment-1977643226
PR Review Comment: https://git.openjdk.org/jdk/pull/18112#discussion_r1511921441

Re: RFR: JDK-8326908: DecimalFormat::toPattern throws OutOfMemoryError when pattern is empty string

[Roger Riggs]

On Sat, 2 Mar 2024 00:34:32 GMT, Justin Lu  wrote:

> Please review this PR and corresponding CSR which prevents an 
> OutOfMemoryError by restricting the initial maximum fraction digits for an 
> empty pattern DecimalFormat.
> 
> For an empty String pattern DecimalFormat, the maximum fraction digits is 
> initialized to `Integer.MAX_VALUE`. When toPattern() is invoked, 
> StringBuilder internally doubles capacity attempting to append 
> Integer.MAX_VALUE digits until OOME occurs. CSR covers potential behavioral 
> compatibility changes.

src/java.base/share/classes/java/text/DecimalFormat.java line 3717:

> 3715: // As maxFracDigits are fully displayed unlike maxIntDigits
> 3716: // Prevent OOME by setting to a much more reasonable value.
> 3717: setMaximumFractionDigits(DOUBLE_FRACTION_DIGITS);

Setting a reasonable default makes sense.  
In other control paths, the max fraction digits come from the inputs or are 
explicitly set.

It might be a reasonable related change to use StringBuilder.repeat() instead 
of a loop at LIne 3312-3319, where the pattern char(s) are being appended to 
the result.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18094#discussion_r1511404338

Re: RFR: 8327225: Revert DataInputStream.readUTF to static final

[Roger Riggs]

On Mon, 4 Mar 2024 13:55:15 GMT, Claes Redestad  wrote:

> [JDK-8325340](https://bugs.openjdk.org/browse/JDK-8325340) accidentally 
> removed `final` from the `static final DataInputStream.readUTF` method. This 
> has a minor compatibility impact (allows hiding the method in a subclass, 
> while before that would throw an exception at compile time) and since it was 
> not the intent of the prior change to alter any behavioral semantics here I 
> want to revert that change.

Good catch; it would have been caught by the signature tests but later.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18107#pullrequestreview-1914625661

Re: RFR: 8309622: Re-examine the cache mechanism in BaseLocale [v11]

[Roger Riggs]

On Fri, 1 Mar 2024 21:29:17 GMT, Naoto Sato  wrote:

>> This is stemming from the PR: https://github.com/openjdk/jdk/pull/14211 
>> where aggressive GC can cause NPE in `BaseLocale$Key` class. I refactored 
>> the in-house cache with WeakHashMap, and removed the Key class as it is no 
>> longer needed (thus the original NPE will no longer be possible). Also with 
>> the new JMH test case, it gains some performance improvement:
>> 
>> (w/o fix)
>> 
>> Benchmark   Mode  Cnt  Score Error  Units
>> LocaleCache.testForLanguageTag  avgt   20   5781.275 ± 569.580  ns/op
>> LocaleCache.testLocaleOfavgt   20  62564.079 ± 406.697  ns/op
>> 
>> (w/ fix)
>> Benchmark   Mode  Cnt  Score Error  Units
>> LocaleCache.testForLanguageTag  avgt   20   4801.175 ± 371.830  ns/op
>> LocaleCache.testLocaleOfavgt   20  60394.652 ± 352.471  ns/op
>
> Naoto Sato has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   copyright year revert

Much simpler and looks good.  Thanks

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14404#pullrequestreview-1912051206

Re: RFR: 8326714: Make file-local functions static in src/java.base/unix/native/libjava/childproc.c

[Roger Riggs]

On Tue, 27 Feb 2024 03:11:37 GMT, Jiangli Zhou  wrote:

> Please help review this trivial change. This was branched from 
> https://github.com/openjdk/jdk/pull/18013, based on discussion with 
> @plummercj in https://github.com/openjdk/jdk/pull/18013 comments. Thanks

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18019#pullrequestreview-1903727632

Re: RFR: 8325340: Add ASCII fast-path to Data-/ObjectInputStream.readUTF [v7]

[Roger Riggs]

On Fri, 16 Feb 2024 15:31:07 GMT, Claes Redestad  wrote:

>> Adding a fast-path for ASCII-only modified UTF-8 strings deserialied via 
>> Data- and ObjectInputStream
>> 
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update test/micro/org/openjdk/bench/java/io/DataInputStreamTest.java
>
>Co-authored-by: Raffaello Giulietti 
>  - Update test/micro/org/openjdk/bench/java/io/ObjectInputStreamTest.java
>
>Co-authored-by: Raffaello Giulietti 

Looks good.

src/java.base/share/classes/java/io/DataInputStream.java line 622:

> 620: "malformed input around byte " + count);
> 621: chararr[chararr_count++]=(char)(((c & 0x1F) << 6) |
> 622:  (char2 & 0x3F));

Seems like the extra space isn't necessary for alignment.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17734#pullrequestreview-1901248533
PR Review Comment: https://git.openjdk.org/jdk/pull/17734#discussion_r1502841700

Re: RFR: 8326653: Remove jdk.internal.reflect.UTF8 [v2]

[Roger Riggs]

On Mon, 26 Feb 2024 11:34:18 GMT, Claes Redestad  wrote:

>> jdk.internal.reflect.UTF8 is used for encoding String to encoded UTF-8 when 
>> generating some classes. 
>> 
>> Since JDK 9 we have a fast-path (which avoids creating encoders) for 
>> UTF-8-encoding strings which is bootstrapped very early, so it seems safe to 
>> rewire this and remove the UTF8 helper class whose stated raison d'être is 
>> to avoid bootstrapping issues.
>> 
>> This cleanup also removes a latent bug since the custom encoder isn't able 
>> to deal with classfile constants containing surrogate pairs.
>> 
>> For a quick comparison I copied the UTF8 code to the `StringEncode` 
>> microbenchmark and set up a benchmark testing the same inputs as 
>> `encodeAllMixed`:
>> 
>> 
>> Benchmark(charsetName)  Mode  Cnt   
>> Score  Error  Units
>> StringEncode.encodeAllMixed  UTF-8  avgt   10   
>> 12894,551 ±  164,816  ns/op
>> StringEncode.encodeUTF8InternalAllMixed  UTF-8  avgt   10  
>> 236614,548 ± 1445,975  ns/op
>> 
>> 
>> I.e. `String.getBytes(UTF_8.instance)` is about 18x faster on mixed inputs. 
>> The benchmark is available in 595b464 but as a quick sanity check not 
>> intended for integration. 
>> 
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Remove temporary microbenchmark

Marked as reviewed by rriggs (Reviewer).

src/java.base/share/classes/jdk/internal/reflect/ClassFileAssembler.java line 
28:

> 26: package jdk.internal.reflect;
> 27: 
> 28: import sun.nio.cs.UTF_8;

I see about equal numbers of uses of `StandardCharsets.UTF_8` and 
`sun.nio.cs.UTF_8.INSTANCE` in java.base but would lean toward the one in 
`StandardCharsets` unless there's a bootstrap order dependency.

-

PR Review: https://git.openjdk.org/jdk/pull/18006#pullrequestreview-1901076118
PR Review Comment: https://git.openjdk.org/jdk/pull/18006#discussion_r1502733517

Re: CFV: New Core Libraries Group Member: Viktor Klang

[Roger Riggs]

Vote: Yes

On 2/19/24 6:40 PM, Stuart Marks wrote:
I hereby nominate Viktor Klang [6] to Membership in the Core Libraries 
Group [4]. 

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Roger Riggs]

On Tue, 13 Feb 2024 21:23:29 GMT, Dan Lutker  wrote:

>> At the time `sleep` seemed ubiquitous and was native so it ran quickly. 
>> (Much quicker than running java).
>> Running another program would be fine.  For example ,selecting an executable 
>> based on the OS type and giving the expected string in the output would be 
>> preferred.
>> The isBusyBox check is also a bad hack, but lighter weight since its not 
>> executing a program.
>> As the number of minor variations of platforms appear a more general 
>> approach would be useful.
>> 
>> I'd code it all in the test itself, doing a lookup based on the operating 
>> system name with the respective executable and expected string in the info 
>> arguments[0].
>> (With a fallback to sleep, so the table does not have to list every os).
>
> Any reason not to use 
> [exesanity_SimpleNativeLauncher](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/native_sanity/simplenativelauncher/exesanity_SimpleNativeLauncher.c)
>  or 
> [exeBasicSleep](https://github.com/openjdk/jdk/blob/628cd8a489fd54db18204c3bbaf4339d7ab5e9d6/test/jdk/java/lang/ProcessBuilder/exeBasicSleep.c)
>  in all cases?

I'd be ok with exeBasicSleep; (currently only used on Windows and if its been 
built by the make files).
Note the discovery mechanism used in Basic.java `initSleepPath()` to locate and 
identify the path where its built or to fallback to the OS sleep.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488640847

Re: CFV: New Core Libraries Group Member: Raffaello Giulietti

[Roger Riggs]

Vote: Yes

On 2/13/24 3:25 PM, Brian Burkhalter wrote:
I hereby nominate Raffaello Giulietti to Membership in the Core 
Libraries Group.

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary [v2]

[Roger Riggs]

On Tue, 13 Feb 2024 18:24:10 GMT, Dan Lutker  wrote:

>> To be fair, this looks similar to what `isMusl()` does: searching for 
>> strings in outputs. But I do wonder if `--help` would include "sleep" for 
>> some innocuous description somewhere, and this code would accidentally 
>> match. @lutkerd, what's the output for `coreutils --help` for a single 
>> executable binary?
>
>> what's the output for `coreutils --help` for a single executable binary?
> 
> 
> # coreutils --help
> Usage: coreutils --coreutils-prog=PROGRAM_NAME [PARAMETERS]... 
> Execute the PROGRAM_NAME built-in program with the given PARAMETERS.
> 
>   --help display this help and exit
>   --version  output version information and exit
> 
> Built-in programs:
>  [ arch b2sum base32 base64 basename basenc cat chcon chgrp chmod chown 
> chroot cksum comm cp csplit cut date dd df dir dircolors dirname du echo env 
> expand expr factor false fmt fold ginstall groups head hostid id join link ln 
> logname ls md5sum mkdir mkfifo mknod mktemp mv nice nl nohup nproc numfmt od 
> paste pathchk pinky pr printenv printf ptx pwd readlink realpath rm rmdir 
> runcon seq sha1sum sha224sum sha256sum sha384sum sha512sum shred shuf sleep 
> sort split stat stdbuf stty sum sync tac tail tee test timeout touch tr true 
> truncate tsort tty uname unexpand uniq unlink users vdir wc who whoami yes
> 
> Use: 'coreutils --coreutils-prog=PROGRAM_NAME --help' for individual program 
> help.
> 
> GNU coreutils online help: 
> Report any translation bugs to 
> Full documentation 
> or available locally via: info '(coreutils) Multi-call invocation'
> 
> 
> This test is about parameter checking, so I am inclined to agree with @ecki 
> and not check the process name or change the exe.

At the time `sleep` seemed ubiquitous and was native so it ran quickly. (Much 
quicker than running java).
Running another program would be fine.  For example ,selecting an executable 
based on the OS type and giving the expected string in the output would be 
preferred.
The isBusyBox check is also a bad hack, but lighter weight since its not 
executing a program.
As the number of minor variations of platforms appear a more general approach 
would be useful.

I'd code it all in the test itself, doing a lookup based on the operating 
system name with the respective executable and expected string in the info 
arguments[0].
(With a fallback to sleep, so the table does not have to list every os).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488468243

Re: RFR: 8325576: java/lang/ProcessHandle/InfoTest.java fails on systems with coreutils with --enable-single-binary

[Roger Riggs]

On Fri, 9 Feb 2024 23:40:00 GMT, Dan Lutker  wrote:

> Ran the test on AmazonLinux 2 which has multiple binaries from coreutils 
> package and no coreutils executable as well as AmazonLinux 2023 that uses 
> `--enable-single-binary`

test/lib/jdk/test/lib/Platform.java line 145:

> 143: }
> 144: 
> 145: public static boolean isOSX() {

This seems like a real hack. Is there really no better way to identity how 
sleep is implemented.
I'd probably favor an alternative that accepts multiple values for the expected 
value.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17798#discussion_r1488338803

[jdk22] Withdrawn: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

[Roger Riggs]

On Tue, 13 Feb 2024 15:57:31 GMT, Roger Riggs  wrote:

> 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

This pull request has been closed without being integrated.

-

PR: https://git.openjdk.org/jdk22/pull/110

Re: [jdk22] RFR: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

[Roger Riggs]

On Tue, 13 Feb 2024 15:57:31 GMT, Roger Riggs  wrote:

> 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

The backport was intended to be for jdk22u

-

PR Comment: https://git.openjdk.org/jdk22/pull/110#issuecomment-1941927141

[jdk22] RFR: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

[Roger Riggs]

8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

-

Commit messages:
 - Backport 13d9e8ff38536287b82c54bb63bd2d20f65615dc

Changes: https://git.openjdk.org/jdk22/pull/110/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk22=110=00
  Issue: https://bugs.openjdk.org/browse/JDK-8325590
  Stats: 39 lines in 2 files changed: 32 ins; 2 del; 5 mod
  Patch: https://git.openjdk.org/jdk22/pull/110.diff
  Fetch: git fetch https://git.openjdk.org/jdk22.git pull/110/head:pull/110

PR: https://git.openjdk.org/jdk22/pull/110

Integrated: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

[Roger Riggs]

On Mon, 12 Feb 2024 21:29:02 GMT, Roger Riggs  wrote:

> Correct the result string coder of a string encoded using a CharsetDecoder 
> with multi-byte encoded input.
> Added tests for UTF16 strings and a regression test.

This pull request has now been integrated.

Changeset: 13d9e8ff
Author:    Roger Riggs 
URL:   
https://git.openjdk.org/jdk/commit/13d9e8ff38536287b82c54bb63bd2d20f65615dc
Stats: 39 lines in 2 files changed: 32 ins; 2 del; 5 mod

8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

Reviewed-by: alanb, redestad

-

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

Re: RFR: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906 [v2]

[Roger Riggs]

> Correct the result string coder of a string encoded using a CharsetDecoder 
> with multi-byte encoded input.
> Added tests for UTF16 strings and a regression test.

Roger Riggs has updated the pull request incrementally with one additional 
commit since the last revision:

  Test Files.readString with multiple charsets
  Cleanup regression test to match style of other tests

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17817/files
  - new: https://git.openjdk.org/jdk/pull/17817/files/45ed4e19..39403b00

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17817=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=17817=00-01

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

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

Re: RFR: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906 [v2]

[Roger Riggs]

On Tue, 13 Feb 2024 07:21:01 GMT, Alan Bateman  wrote:

>> Roger Riggs has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Test Files.readString with multiple charsets
>>   Cleanup regression test to match style of other tests
>
> test/jdk/java/nio/file/Files/ReadWriteString.java line 322:
> 
>> 320: }
>> 321: assertEquals(actual, original, "Round trip string mismatch with 
>> multi-byte encoding");
>> 322: }
> 
> The update to newStringNoRepl1 looks fine.
> 
> The added test is very different to the tests in this source file. We really 
> need to expand the test to exercise a lot more charsets and input cases.  
> It's okay to have a targeted test for now but needs to be renamed to be 
> consistent with the other tests. Also the other tests use testFiles as the 
> file paths rather than putting files in /tmp.

The UTF-16 charset is added to the existing `readString` test.
Additional charsets can be added, most will exercise the same code path in 
newStringNoRepl1 that uses a CharsetDecoder for all charsets other than UTF-8, 
ASCII, or ISO-8859-1.

The additional individual test is taken from the bug report and is not strictly 
necessary.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17817#discussion_r1487787125

RFR: 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

[Roger Riggs]

Correct the result string coder of a string encoded using a CharsetDecoder with 
multi-byte encoded input.
Added tests for UTF16 strings and a regression test.

-

Commit messages:
 - 8325590: Regression in round-tripping UTF-16 strings after JDK-8311906

Changes: https://git.openjdk.org/jdk/pull/17817/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk=17817=00
  Issue: https://bugs.openjdk.org/browse/JDK-8325590
  Stats: 24 lines in 2 files changed: 18 ins; 1 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/17817.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17817/head:pull/17817

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

Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v7]

[Roger Riggs]

On Thu, 8 Feb 2024 21:29:19 GMT, Justin Lu  wrote:

>> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319344) 
>> which adds MessageFormat pattern support for the following subformats: 
>> ListFormat, CompactNumberFormat, and DateTimeFormatter. This change is 
>> intended to provide pattern support for the more recently added JDK Format 
>> subclasses, as well as improving java.time formatting within i18n. The draft 
>> javadoc can be viewed here: 
>> https://cr.openjdk.org/~jlu/docs/api/java.base/java/text/MessageFormat.html. 
>> Please see the CSR for more in-depth behavioral changes, as well as 
>> limitations.
>> 
>> The `FormatTypes`:  dtf_date, dtf_time, dtf_datetime, pre-defined 
>> DateTimeFormatter(s), and list are added.
>> The `FormatStyles`: compact_short, compact_long, or, and unit are added.
>> 
>> For example, previously,
>> 
>> 
>> Object[] args = {LocalDate.of(2023, 11, 16), LocalDate.of(2023, 11, 27)};
>> MessageFormat.format("It was {0,date,full}, now it is {1,date,full}", args);
>> 
>> 
>> would throw `Exception java.lang.IllegalArgumentException: Cannot format 
>> given Object as a Date`
>> 
>> Now, a user can call
>> 
>> 
>> MessageFormat.format("It was {0,dtf_date,full}, now it is 
>> {1,dtf_date,full}", args);
>> 
>> 
>> which returns "It was Thursday, November 16, 2023, now it is Friday, 
>> November 17, 2023"
>
> Justin Lu has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   implement Naoto's review

Good cleanup and filling some gaps in the pattern cases.

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17663#pullrequestreview-1873083702

Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v4]

[Roger Riggs]

On Wed, 7 Feb 2024 00:48:30 GMT, Naoto Sato  wrote:

>> One idea would be to delegate to a (package-private) method in the formatXXX 
>> class. 
>> That would localize to the respective class the details.
>> (An abstract protected method might be preferred, but its not worth creating 
>> extra public API surface area for this).
>
> Would it work with custom implementations for say NumberFormat via the SPI?

Just moving the if...then... else code to a package-private static method in 
the respective XXFormat class would work the same.
I had not thought of an addition to the spi to delegate to/through the provider.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1481637904

Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v4]

[Roger Riggs]

On Tue, 6 Feb 2024 22:31:32 GMT, Justin Lu  wrote:

>> src/java.base/share/classes/java/text/MessageFormat.java line 681:
>> 
>>> 679: if (fmt instanceof NumberFormat) {
>>> 680: // Add any instances returned from the NumberFormat 
>>> factory methods
>>> 681: if (fmt.equals(NumberFormat.getInstance(locale))) {
>> 
>> This looks like wack-a-mole code, no good design; likely to be hard to 
>> maintain.
>> (I don't have a better idea at the moment though).
>
> I agree, it was the existing design that caused for example, 
> CompactNumberFormat to not automatically be supported by MessageFormat. A 
> simple alternative would be storing the potential pre-defined NumberFormats 
> in some data structure that we could just iterate through to feel less “whack 
> a mole” like, but that array would still suffer from the same maintenance 
> issues. I’ll try to think of something better.

One idea would be to delegate to a (package-private) method in the formatXXX 
class. 
That would localize to the respective class the details.
(An abstract protected method might be preferred, but its not worth creating 
extra public API surface area for this).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480644067

Re: RFR: JDK-8325255 jdk.internal.util.ReferencedKeySet::add using wrong test [v2]

[Roger Riggs]

On Tue, 6 Feb 2024 13:56:09 GMT, Jim Laskey  wrote:

>> Currently, add is returning intern(e) == null which will always be false. 
>> The correct test is intern(e) == e , that is, true when element is newly 
>> added.
>
> Jim Laskey 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 two additional commits since 
> the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into 8325255
>  - Correct test

LGTM

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17732#pullrequestreview-1865626163

Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v4]

[Roger Riggs]

On Fri, 2 Feb 2024 22:24:49 GMT, Naoto Sato  wrote:

>> We discussed this separately, but will go over again here in case others are 
>> interested. Since DTF does not implement `equals()`, even if we implement 
>> `equals()` for the `ClassicFormat`, we would basically still need to 
>> implement our own `equals()` for a DTF to effectively compare the 
>> ClassicFormats.
>> 
>> I had also mentioned that we could reference check the pre-defined DTFs, but 
>> this won't work either actually. This is because we cannot reference check 
>> the DTFs, but rather the Format adapted DTFs, which mean they are now new 
>> ClassicFormats every time since we don't store them.
>
> Thanks. I think it is fine.

The comment "does not implement equals and thus cannot be represented as a 
pattern" doesn't explain why it can be represented as a pattern. Can that be 
explained better?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480011093

Re: RFR: JDK-8318761: MessageFormat pattern support for CompactNumberFormat, ListFormat, and DateTimeFormatter [v4]

[Roger Riggs]

On Mon, 5 Feb 2024 23:51:00 GMT, Justin Lu  wrote:

>> Please review this PR and [CSR](https://bugs.openjdk.org/browse/JDK-8319344) 
>> which adds MessageFormat pattern support for the following subformats: 
>> ListFormat, CompactNumberFormat, and DateTimeFormatter. This change is 
>> intended to provide pattern support for the more recently added JDK Format 
>> subclasses, as well as improving java.time formatting within i18n. The draft 
>> javadoc can be viewed here: 
>> https://cr.openjdk.org/~jlu/docs/api/java.base/java/text/MessageFormat.html. 
>> Please see the CSR for more in-depth behavioral changes, as well as 
>> limitations.
>> 
>> The `FormatTypes`:  dtf_date, dtf_time, dtf_datetime, pre-defined 
>> DateTimeFormatter(s), and list are added.
>> The `FormatStyles`: compact_short, compact_long, or, and unit are added.
>> 
>> For example, previously,
>> 
>> 
>> Object[] args = {LocalDate.of(2023, 11, 16), LocalDate.of(2023, 11, 27)};
>> MessageFormat.format("It was {0,date,full}, now it is {1,date,full}", args);
>> 
>> 
>> would throw `Exception java.lang.IllegalArgumentException: Cannot format 
>> given Object as a Date`
>> 
>> Now, a user can call
>> 
>> 
>> MessageFormat.format("It was {0,dtf_date,full}, now it is 
>> {1,dtf_date,full}", args);
>> 
>> 
>> which returns "It was Thursday, November 16, 2023, now it is Friday, 
>> November 17, 2023"
>
> Justin Lu has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Apply spacing suggestions
>
>Co-authored-by: Andrey Turbanov 
>  - add expected message to exception checking

src/java.base/share/classes/java/text/MessageFormat.java line 90:

> 88:  * dtf_time
> 89:  * dtf_datetime
> 90:  * pre-defined DateTimeFormatter(s)

Its not clear why these new options are inserted before the existing formats.
(And also the order of the table below).

src/java.base/share/classes/java/text/MessageFormat.java line 219:

> 217:  *   {@code 
> pre-defined DateTimeFormatter(s)}
> 218:  *   (none)
> 219:  *   The {@code pre-defined DateTimeFormatter(s)} are used as a 
> {@code FormatType} | {@link DateTimeFormatter#BASIC_ISO_DATE BASIC_ISO_DATE}, 
> {@link DateTimeFormatter#ISO_LOCAL_DATE ISO_LOCAL_DATE}, {@link 
> DateTimeFormatter#ISO_OFFSET_DATE ISO_OFFSET_DATE}, {@link 
> DateTimeFormatter#ISO_DATE ISO_DATE}, {@link DateTimeFormatter#ISO_LOCAL_TIME 
> ISO_LOCAL_TIME}, {@link DateTimeFormatter#ISO_OFFSET_TIME ISO_OFFSET_TIME}, 
> {@link DateTimeFormatter#ISO_TIME ISO_TIME}, {@link 
> DateTimeFormatter#ISO_LOCAL_DATE_TIME ISO_LOCAL_DATE_TIME}, {@link 
> DateTimeFormatter#ISO_OFFSET_DATE_TIME ISO_OFFSET_DATE_TIME}, {@link 
> DateTimeFormatter#ISO_ZONED_DATE_TIME ISO_ZONED_DATE_TIME}, {@link 
> DateTimeFormatter#ISO_DATE_TIME ISO_DATE_TIME}, {@link 
> DateTimeFormatter#ISO_ORDINAL_DATE ISO_ORDINAL_DATE}, {@link 
> DateTimeFormatter#ISO_WEEK_DATE ISO_WEEK_DATE}, {@link 
> DateTimeFormatter#ISO_INSTANT ISO_INSTANT}, {@link 
> DateTimeFormatter#RFC_1123_DATE_TIME RFC_1123_DATE_TIME}

The "|" is out of place; its not a common delimiter.  Perhaps ":" colon instead.
This source line is too long (80-100 is typical). Breaking the line should not 
break the table formatting.

src/java.base/share/classes/java/text/MessageFormat.java line 335:

> 333:  * {@code result} returns the following:
> 334:  * 
> 335:  * At 12:30 PM on Jul 3, 2053, there was a disturbance in the Force on 
> planet 7.

An explicit date `new Date(7,3,2053)` would give predictable results between 
the code and the result string.

src/java.base/share/classes/java/text/MessageFormat.java line 681:

> 679: if (fmt instanceof NumberFormat) {
> 680: // Add any instances returned from the NumberFormat factory 
> methods
> 681: if (fmt.equals(NumberFormat.getInstance(locale))) {

This looks like wack-a-mole code, no good design; likely to be hard to maintain.
(I don't have a better idea at the moment though).

src/java.base/share/classes/java/text/MessageFormat.java line 1881:

> 1879: for (FormatStyle style : values()) {
> 1880: // Also check trimmed case-insensitive for historical 
> reasons
> 1881: if 
> (text.trim().toLowerCase(Locale.ROOT).equals(style.text)) {

`String.compareIgnoreCase()` might be clearer and not need to allocate for 
a converted string.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1479979607
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1479989358
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r147184
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480020800
PR Review Comment: https://git.openjdk.org/jdk/pull/17663#discussion_r1480039800

Re: RFR: JDK-8310913 Move ReferencedKeyMap to jdk.internal so it may be shared [v10]

[Roger Riggs]

On Mon, 5 Feb 2024 12:34:02 GMT, Jim Laskey  wrote:

>> src/java.base/share/classes/jdk/internal/util/ReferencedKeySet.java line 151:
>> 
>>> 149: @Override
>>> 150: public boolean add(T e) {
>>> 151: return intern(e) == null;
>> 
>> This line is wrong, as `intern(…)` will never return `null`.
>> 
>> 
>> 
>> This is the closest to the correct implementation,
>> Suggestion:
>> 
>> return !contains(e) & intern(e) == e;
>> 
>> 
>> but may incorrectly return `true` in case of the following data race, 
>> assuming `t1e == t2e`:
>> 
>> 1. **Thread 1** calls `contains(t1e)` and gets `false`.
>> 2. **Thread 2** calls `intern(t2e)` and successfully adds `t2e`.
>> 3. **Thread 1** calls `intern(t1e)` and gets back `t2e`, which is `==` to 
>> `t1e`.
>> 4. **Thread 1** returns `true`, even though it was **Thread 2** which 
>> modified the `ReferencedKeySet`.
>
> Good catch. Your solution might be correct but I think `!contains(e)` is 
> redundant since that is how intern starts out.
> 
> 
>static  T intern(ReferencedKeyMap> setMap, T key, 
> UnaryOperator interner) {
> T value = existingKey(setMap, key);
> if (value != null) {
> return value;
> }
> key = interner.apply(key);
> return internKey(setMap, key);
> }
> 
> 
> Agree? So changing to `return intern(e) == e;` should be sufficient.
> 
> The other aspect of this is that `internKey` uses `putIfAbsent` which should 
> prevent the race (assuming `ConcurrentHashMap`).
> 
> 
> /**
>  * Attempt to add key to map.
>  *
>  * @param setMap{@link ReferencedKeyMap} where interning takes place
>  * @param key   key to add
>  *
>  * @param  type of key
>  *
>  * @return the old key instance if found otherwise the new key instance
>  */
> private static  T internKey(ReferencedKeyMap> 
> setMap, T key) {
> ReferenceKey entryKey = setMap.entryKey(key);
> T interned;
> do {
> setMap.removeStaleReferences();
> ReferenceKey existing = setMap.map.putIfAbsent(entryKey, 
> entryKey);
> if (existing == null) {
> return key;
> } else {
> // If {@code putIfAbsent} returns non-null then was actually a
> // {@code replace} and older key was used. In that case the 
> new
> // key was not used and the reference marked stale.
> interned = existing.get();
> if (interned != null) {
> entryKey.unused();
> }
> }
> } while (interned == null);
> return interned;
> }
> 
> 
> Agree? Anyone else want to pipe in?

ok, `intern(e) == e` is sufficient.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/14684#discussion_r1478541091

Re: RFR: 8325169: Reduce String::indexOf overheads [v3]

[Roger Riggs]

On Sat, 3 Feb 2024 14:36:14 GMT, Claes Redestad  wrote:

>> This patch streamlines and specializes various `String::indexOf` methods. 
>> Mainly avoids the need for clamping and doing checks that are redundant in 
>> almost all cases, moving the checks to the API boundary where they are 
>> needed. 
>> 
>> This improves performance both at peak and during startup/warmup. Since 
>> indexOf is heavily used in bootstrapping code it makes sense to slightly 
>> dial back abstraction and delegation, which in this case also brought some 
>> benefit to peak performance.
>> 
>> Testing: tier1-3
>
> Claes Redestad has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - Update comments regarding bounds checks
>  - Clamp fromIndex to be in range

Marked as reviewed by rriggs (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/17685#pullrequestreview-1863061296