Re: RFR: 8329729: java/util/Properties/StoreReproducibilityTest.java times out [v2]

[Alan Bateman]

On Tue, 9 Apr 2024 01:27:29 GMT, Jaikiran Pai  wrote:

>> Can I please get a review of this test-only change which proposes to address 
>> the intermittent failures in 
>> `java/util/Properties/StoreReproducibilityTest.java`? This should address 
>> https://bugs.openjdk.org/browse/JDK-8329729.
>> 
>> These failures in `StoreReproducibilityTest` have been observed in higher 
>> tiers where the test tasks are launched with various JVM options, one of 
>> them being `-Xcomp`. The goal of the `StoreReproducibilityTest` is to verify 
>> that the content which the `java.util.Properties` code generates for a 
>> properties file and reproducible across multiple different runs/launches of 
>> an Java application. To do that it launches a test application (using `java` 
>> command) several times within the test (for different scenarios). That comes 
>> up to a combined total of 25 launches, for different scenarios. Normally 
>> each such launch of the `java` application completes within a second or two. 
>> 
>> Recently, we have been updating our tests to pass along the JVM options that 
>> were used for launching the test task, to the child processes that are 
>> launched from within the tests. That now means that these trivial small java 
>> application that this test launches several times will now be passed the 
>> `-Xcomp` option too (when the test task is launched with that option). It 
>> has been observed that when `-Xcomp` is used to launch those trivial 
>> applications from within the test, each such application takes around 30 
>> seconds to a minute to complete. This then causes the test to timeout.
>> 
>> Given the context of this test case, it's not necessary to run this test 
>> when `-Xcomp` is used. The commit in this PR add a `@requires` to disable 
>> this test when `-Xcomp` is present in the test task's JVM args. 
>> 
>> I've run this change in our CI and the test continues to run (and pass) when 
>> `-Xcomp` is absent and is skipped when it is present.
>
> Jaikiran Pai 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 latest from master branch
>  - 8329729: java/util/Properties/StoreReproducibilityTest.java times out

Looks fine. If you can want, the comment could be much shorter. It just has to 
say that the test launches several processes and is too just with -Xcomp.

-

Marked as reviewed by alanb (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18681#pullrequestreview-1988155446

Re: RFR: 8323640: [TESTBUG]testMemoryFailCount in jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because OOM killed [v3]

[SendaoYan]

On Tue, 23 Jan 2024 13:04:43 GMT, SendaoYan  wrote:

>> 8323640: [TESTBUG]testMemoryFailCount in 
>> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail 
>> because OOM killed
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   8323640: [TESTBUG]testMemoryFailCount in 
> jdk/internal/platform/docker/TestDockerMemoryMetrics.java always fail because 
> OOM killed
>   
>   Signed-off-by: sendaoYan 

> /sponsor

@jerboaa Thanks for the review and the sponsor.

-

PR Comment: https://git.openjdk.org/jdk/pull/17514#issuecomment-2044000397

Re: RFR: 8326461: tools/jlink/CheckExecutable.java fails as .debuginfo files are not executable

[SendaoYan]

On Thu, 22 Feb 2024 07:23:04 GMT, SendaoYan  wrote:

> Before JDK-8325342(commit id:0bcece995840777db660811e4b20bb018e90439b), all 
> the files in build/linux-x86_64-server-release/images/jdk/bin are executable:
> 
> ![image](https://github.com/openjdk/jdk/assets/24123821/13f0eae2-7125-4d09-8793-8a5a10b785c2)
> 
> 
> After JDK-8325342, all the *.debuginfo files in 
> build/linux-x86_64-server-release/images/jdk/bin are not executable:
> 
> ![image](https://github.com/openjdk/jdk/assets/24123821/c8d190f2-3db0-439b-82b9-5121567cb1d5)
> 
> 
> This PR only modifies the testcase to adapt to the modification of the 
> corresponding build script, ignoring the check of debuginfo file executable 
> permissions, and the risk is low

> /sponsor

@shipilev @AlanBateman Thanks.

-

PR Comment: https://git.openjdk.org/jdk/pull/17958#issuecomment-2043997665

Re: RFR: 8327486: java/util/Properties/PropertiesStoreTest.java fails "Text 'xxx' could not be parsed at index 20" after 8174269 [v3]

[SendaoYan]

On Mon, 11 Mar 2024 15:15:39 GMT, Naoto Sato  wrote:

> LGTM

@naotoj Thanks for the review.

-

PR Comment: https://git.openjdk.org/jdk/pull/18155#issuecomment-2043989340

Re: RFR: 8327486: java/util/Properties/PropertiesStoreTest.java fails "Text 'xxx' could not be parsed at index 20" after 8174269 [v3]

[SendaoYan]

On Sat, 9 Mar 2024 13:18:14 GMT, SendaoYan  wrote:

>> Date.toString() uses Locale.US explicitly for printing the time zone, so 
>> replace Locale.ROOT to Locale.US in this testcase for fix the test failure.
>> 
>> This testcase fixed has been verified.
>> 
>> Only change the testcase, risk is low.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   update the Locale.US code comment
>   
>   Signed-off-by: sendaoYan 

> /sponsor

@DamonFool Thanks.

-

PR Comment: https://git.openjdk.org/jdk/pull/18155#issuecomment-2043989514

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

[Naoto Sato]

On Tue, 9 Apr 2024 00:52:43 GMT, Jaikiran Pai  wrote:

>> 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).
>
> Hello Roger,
> 
>> The code should reference the constants in Instant.java (though may need to 
>> make them package private.)
> 
> The `ChronoField` class and the `Instant` class reside in separate packages, 
> so making these two fields in `Instant`, package private will not help. I 
> will have to make them public, which I think probably isn't a good idea. 
> Unless you think we should do it? There is one other place already in the JDK 
> where we have copy/pasted these values 
> `src/java.base/share/classes/java/nio/file/attribute/FileTime.java`, so maybe 
> we can continue with this copy/paste here too?
> 
> As for the javadoc, after we decide about this field access detail, I'll 
> update it accordingly to note that the values min and max range complies with 
> the min and max epoch seconds supported by `Instant`.

Should `INSTANT_SECONDS("InstantSeconds", SECONDS, FOREVER, 
ValueRange.of(Instant.MIN.getEpochSecond(), Instant.MAX.getEpochSecond())),
` work?

-

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

Re: RFR: 8329729: java/util/Properties/StoreReproducibilityTest.java times out [v2]

[Jaikiran Pai]

> Can I please get a review of this test-only change which proposes to address 
> the intermittent failures in 
> `java/util/Properties/StoreReproducibilityTest.java`? This should address 
> https://bugs.openjdk.org/browse/JDK-8329729.
> 
> These failures in `StoreReproducibilityTest` have been observed in higher 
> tiers where the test tasks are launched with various JVM options, one of them 
> being `-Xcomp`. The goal of the `StoreReproducibilityTest` is to verify that 
> the content which the `java.util.Properties` code generates for a properties 
> file and reproducible across multiple different runs/launches of an Java 
> application. To do that it launches a test application (using `java` command) 
> several times within the test (for different scenarios). That comes up to a 
> combined total of 25 launches, for different scenarios. Normally each such 
> launch of the `java` application completes within a second or two. 
> 
> Recently, we have been updating our tests to pass along the JVM options that 
> were used for launching the test task, to the child processes that are 
> launched from within the tests. That now means that these trivial small java 
> application that this test launches several times will now be passed the 
> `-Xcomp` option too (when the test task is launched with that option). It has 
> been observed that when `-Xcomp` is used to launch those trivial applications 
> from within the test, each such application takes around 30 seconds to a 
> minute to complete. This then causes the test to timeout.
> 
> Given the context of this test case, it's not necessary to run this test when 
> `-Xcomp` is used. The commit in this PR add a `@requires` to disable this 
> test when `-Xcomp` is present in the test task's JVM args. 
> 
> I've run this change in our CI and the test continues to run (and pass) when 
> `-Xcomp` is absent and is skipped when it is present.

Jaikiran Pai 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 latest from master branch
 - 8329729: java/util/Properties/StoreReproducibilityTest.java times out

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18681/files
  - new: https://git.openjdk.org/jdk/pull/18681/files/843ef6b5..8cca30dc

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

  Stats: 5038 lines in 90 files changed: 3546 ins; 1200 del; 292 mod
  Patch: https://git.openjdk.org/jdk/pull/18681.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18681/head:pull/18681

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

RFR: 8329729: java/util/Properties/StoreReproducibilityTest.java times out

[Jaikiran Pai]

Can I please get a review of this test-only change which proposes to address 
the intermittent failures in 
`java/util/Properties/StoreReproducibilityTest.java`? This should address 
https://bugs.openjdk.org/browse/JDK-8329729.

These failures in `StoreReproducibilityTest` have been observed in higher tiers 
where the test tasks are launched with various JVM options, one of them being 
`-Xcomp`. The goal of the `StoreReproducibilityTest` is to verify that the 
content which the `java.util.Properties` code generates for a properties file 
and reproducible across multiple different runs/launches of an Java 
application. To do that it launches a test application (using `java` command) 
several times within the test (for different scenarios). That comes up to a 
combined total of 25 launches, for different scenarios. Normally each such 
launch of the `java` application completes within a second or two. 

Recently, we have been updating our tests to pass along the JVM options that 
were used for launching the test task, to the child processes that are launched 
from within the tests. That now means that these trivial small java application 
that this test launches several times will now be passed the `-Xcomp` option 
too (when the test task is launched with that option). It has been observed 
that when `-Xcomp` is used to launch those trivial applications from within the 
test, each such application takes around 30 seconds to a minute to complete. 
This then causes the test to timeout.

Given the context of this test case, it's not necessary to run this test when 
`-Xcomp` is used. The commit in this PR add a `@requires` to disable this test 
when `-Xcomp` is present in the test task's JVM args. 

I've run this change in our CI and the test continues to run (and pass) when 
`-Xcomp` is absent and is skipped when it is present.

-

Commit messages:
 - 8329729: java/util/Properties/StoreReproducibilityTest.java times out

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

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

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

[Jaikiran Pai]

On Mon, 8 Apr 2024 18:16:04 GMT, Roger Riggs  wrote:

>> 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).

Hello Roger,

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

The `ChronoField` class and the `Instant` class reside in separate packages, so 
making these two fields in `Instant`, package private will not help. I will 
have to make them public, which I think probably isn't a good idea. Unless you 
think we should do it? There is one other place already in the JDK where we 
have copy/pasted these values 
`src/java.base/share/classes/java/nio/file/attribute/FileTime.java`, so maybe 
we can continue with this copy/paste here too?

As for the javadoc, after we decide about this field access detail, I'll update 
it accordingly to note that the values min and max range complies with the min 
and max epoch seconds supported by `Instant`.

-

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

Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v7]

[Dean Long]

On Mon, 8 Apr 2024 19:11:19 GMT, Scott Gibbons  wrote:

>> This code makes an intrinsic stub for `Unsafe::setMemory` for x86_64.  See 
>> [this PR](https://github.com/openjdk/jdk/pull/16760) for discussion around 
>> this change.
>> 
>> Overall, making this an intrinsic improves overall performance of 
>> `Unsafe::setMemory` by up to 4x for all buffer sizes.
>> 
>> Tested with tier-1 (and full CI).  I've added a table of the before and 
>> after numbers for the JMH I ran (`MemorySegmentZeroUnsafe`).
>> 
>> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt)
>
> Scott Gibbons has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Add movq to locate_operand

Thanks, I see that my ideas have pretty much already been discussed in 
https://github.com/openjdk/jdk/pull/16760.  I might have missed it, but has the 
possibility of always setting the aligned interior region with 8 byte stores 
been discussed?  A literal reading of the javadoc seems to disallow it, but it 
seems like it should be allowed based on memory coherence.  Only the unaligned 
head and tail would need special treatment.

-

PR Comment: https://git.openjdk.org/jdk/pull/18555#issuecomment-2043732829

Re: RFR: 8325659: Normalize Random usage by incubator vector tests

[Paul Sandoz]

On Mon, 8 Apr 2024 10:50:00 GMT, Evgeny Nikitin  wrote:

> Improve RNG usage in said tests:
> 
> 1. The RNG is not created by our Utils class, as suggested in our JTReg tests;
> 2. The seed, accordingly, is not a fixed value now, but truly random;
> 3. The tests that had been creating their own Random instances, are now using 
> RAND static member from the AbstractVectorTest;
> 4. The generated tests sources have been re-generated.
> 
> The most important change is #2, it could add variability and help cover more 
> JIT Compiler and Runtime scenarios.

Looks good, thank you for cleaning this up.

-

Marked as reviewed by psandoz (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/18675#pullrequestreview-1987442519

RFR: 8325659: Normalize Random usage by incubator vector tests

[Evgeny Nikitin]

Improve RNG usage in said tests:

1. The RNG is not created by our Utils class, as suggested in our JTReg tests;
2. The seed, accordingly, is not a fixed value now, but truly random;
3. The tests that had been creating their own Random instances, are now using 
RAND static member from the AbstractVectorTest;
4. The generated tests sources have been re-generated.

The most important change is #2, it could add variability and help cover more 
JIT Compiler and Runtime scenarios.

-

Commit messages:
 - Adjust generated tests
 - Use class member RNG vs newly created one
 - 8325659: Normalize Random usage by incubator vector tests

Changes: https://git.openjdk.org/jdk/pull/18675/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk=18675=00
  Issue: https://bugs.openjdk.org/browse/JDK-8325659
  Stats: 469 lines in 69 files changed: 188 ins; 0 del; 281 mod
  Patch: https://git.openjdk.org/jdk/pull/18675.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18675/head:pull/18675

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

Re: [External] : Re: JEP 473: Stream Gatherers (Second Preview)

[Viktor Klang]

>"could" not "would"?

If the operation is strictly many-to-one, then it will always emit a single 
element, this means that it might need to track as a part of its state whether 
it has emitted something in its integrator and if not then emit it in the 
finisher.

But I, guess that one could reason about it from multiple levels of zoom: a 
Stream can have end-to-end a specific N:M-ratio, a Gatherer could have a 
specific N:M-ratio, and an Integrator itself could have a specific N:M-ratio.

Think of something like a `sort()`-Gatherer, it would not be able to emit 
anything until it has consumed all upstream elements, and it would only know 
that all upstream elements have been consumed once its finisher is invoked, and 
as a response to that it could then iterate over the entire sorted set of 
elements and emit them in the new order downstream.

>Other than at  initialization or finish, is it possible to have an "empty" 
>input?

No, not an empty as in "absense of". You'd have to emit in the finisher then.



Cheers,
√


Viktor Klang
Software Architect, Java Platform Group
Oracle

From: Ernie Rael 
Sent: Sunday, 7 April 2024 18:31
To: Viktor Klang ; core-libs-dev@openjdk.org 

Subject: [External] : Re: JEP 473: Stream Gatherers (Second Preview)

On 24/04/07 9:11 AM, Viktor Klang wrote:
Hi Ernie,

"Many" in this case refers to "N", which is "0 ... N",
OK, I was wondering about "many" including "0".
so I'd say while it is techincally correct as-is, perhaps more precise would be 
to say "1-to-0..1" gatherer, since for every element in, there is 0 or 1 
element out.
I see.

Many-to-one would be 0..N -> 1, which means that an empty input would
"could" not "would"?
yield a single output.

Out of curiosity, is either correct technically?


Other than at  initialization or finish, is it possible to have an "empty" 
input?


-ernie

Cheers,
√


Viktor Klang
Software Architect, Java Platform Group
Oracle

From: core-libs-dev 
 on 
behalf of Ernie Rael 
Sent: Sunday, 7 April 2024 18:06
To: core-libs-dev@openjdk.org 

Subject: JEP 473: Stream Gatherers (Second Preview)


This is about what might be a minor doc issue.

In https://openjdk.org/jeps/473 it says

As another example, Stream::filter takes a predicate that determines whether an 
input element should be passed downstream; this is simply a stateless 
one-to-many gatherer.
Shouldn't this be "many-to-one"?

-ernie

Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v17]

[Martin Doerr]

On Mon, 8 Apr 2024 18:44:51 GMT, Suchismith Roy  wrote:

>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   test change

test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
 line 36:

> 34: String libraryName = "awt";
> 35: File awtSharedObjectPath = new File("/test/lib/libawt.so");
> 36: File awtArchivePath = new File("/test/lib/libawt.a");

How does this work? Did you create a "/test" directory? I don't have it on my 
machine.

test/jdk/java/lang/RuntimeTests/loadLibrary/aix/LoadAIXLibraryFromArchiveObject.java
 line 37:

> 35: File awtSharedObjectPath = new File("/test/lib/libawt.so");
> 36: File awtArchivePath = new File("/test/lib/libawt.a");
> 37: awtSharedObjectPath.renameTo(awtArchivePath);

This should work for this test. But, what if an AWT test gets executed after 
your test? I think copy is safer than renaming.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1556361989
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r1556360137

Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v7]

[Scott Gibbons]

> This code makes an intrinsic stub for `Unsafe::setMemory` for x86_64.  See 
> [this PR](https://github.com/openjdk/jdk/pull/16760) for discussion around 
> this change.
> 
> Overall, making this an intrinsic improves overall performance of 
> `Unsafe::setMemory` by up to 4x for all buffer sizes.
> 
> Tested with tier-1 (and full CI).  I've added a table of the before and after 
> numbers for the JMH I ran (`MemorySegmentZeroUnsafe`).
> 
> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt)

Scott Gibbons has updated the pull request incrementally with one additional 
commit since the last revision:

  Add movq to locate_operand

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18555/files
  - new: https://git.openjdk.org/jdk/pull/18555/files/fd6f04f7..f81aaa9f

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18555=06
 - incr: https://webrevs.openjdk.org/?repo=jdk=18555=05-06

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

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

Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v17]

[Suchismith Roy]

> Allow support for both .a and .so files in AIX.
> If .so file is not found, allow fallback to .a extension.
> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)

Suchismith Roy has updated the pull request incrementally with one additional 
commit since the last revision:

  test change

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17945/files
  - new: https://git.openjdk.org/jdk/pull/17945/files/42820306..f0459fc3

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17945=16
 - incr: https://webrevs.openjdk.org/?repo=jdk=17945=15-16

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

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

Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v16]

[Suchismith Roy]

> Allow support for both .a and .so files in AIX.
> If .so file is not found, allow fallback to .a extension.
> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)

Suchismith Roy has updated the pull request incrementally with one additional 
commit since the last revision:

  test change

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17945/files
  - new: https://git.openjdk.org/jdk/pull/17945/files/66dc630a..42820306

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17945=15
 - incr: https://webrevs.openjdk.org/?repo=jdk=17945=14-15

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

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

Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v15]

[Suchismith Roy]

> Allow support for both .a and .so files in AIX.
> If .so file is not found, allow fallback to .a extension.
> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)

Suchismith Roy has updated the pull request incrementally with one additional 
commit since the last revision:

  test change

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17945/files
  - new: https://git.openjdk.org/jdk/pull/17945/files/1746f554..66dc630a

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17945=14
 - incr: https://webrevs.openjdk.org/?repo=jdk=17945=13-14

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

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

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

Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v14]

[Suchismith Roy]

> Allow support for both .a and .so files in AIX.
> If .so file is not found, allow fallback to .a extension.
> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)

Suchismith Roy has updated the pull request incrementally with one additional 
commit since the last revision:

  test change

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/17945/files
  - new: https://git.openjdk.org/jdk/pull/17945/files/89b8da39..1746f554

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=17945=13
 - incr: https://webrevs.openjdk.org/?repo=jdk=17945=12-13

  Stats: 11 lines in 2 files changed: 1 ins; 1 del; 9 mod
  Patch: https://git.openjdk.org/jdk/pull/17945.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/17945/head:pull/17945

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

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

[Naoto Sato]

On Mon, 8 Apr 2024 09:52:39 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.

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?

-

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

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: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v13]

[Suchismith Roy]

On Fri, 5 Apr 2024 18:14:36 GMT, Suchismith Roy  wrote:

>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - update tests
>  - update tests
>  - update tests

Ok. How can i get the path of an existing .so file and renamed it to .a ? 
Example we have libawt.so. In order to rename it i need to find its path. How 
can i do that using Java code ? 
Do i need to parse the java library path and search in it ?

-

PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2043232272

Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v9]

[Vicente Romero]

On Mon, 8 Apr 2024 16:21:19 GMT, Jan Lahoda  wrote:

>> This is a patch for javac, that adds the Derived Record Creation 
>> expressions. The current draft specification for the feature is:
>> https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html
>> 
>> The current CSR is here:
>> https://bugs.openjdk.org/browse/JDK-8328637
>> 
>> The patch is mostly straightforward, with two notable changes:
>>  - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the 
>> specification introduces this term, and it seems consistent with 
>> `ElementKind.BINDING_VARIABLE` that was introduced some time ago.
>>  - there are a bit broader changes in `Flow`, to facilitate the introduction 
>> of variables without an explicit declaration for definite assignment and 
>> effectively final computation.
>
> Jan Lahoda has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Adding tests as suggested.

src/jdk.compiler/share/classes/com/sun/tools/javac/comp/TransPatterns.java line 
101:

> 99: import com.sun.tools.javac.tree.JCTree.JCPattern;
> 100: import com.sun.tools.javac.tree.JCTree.JCPatternCaseLabel;
> 101: import com.sun.tools.javac.tree.JCTree.JCDerivedInstance;

changes to this file can be removed now

test/langtools/tools/javac/patterns/withers/Model.java line 2:

> 1: /*
> 2:  * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.

probably this test should be in another location not under `patterns`, same for 
other tests added inside `patterns` folder

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1556130188
PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1556136586

Re: JEP 473: Stream Gatherers (Second Preview)

[Ernie Rael]

Filter isn't many-to-... At all, at no point it collapses a 
subsequence of elements, it only works on one element at a time 
I was conflating the behavior of a specific intermediate operation with 
a black box view of an operation. Looking at an operation as a black box 
is useless (maybe mildly interesting at most). The description of filter 
as "one-to-many" threw me off balance (this is the first time I read the 
gatherer spec in detail, and I've only looked at streams on an as need 
basis, it took this long having brief exposures till I felt ready); but 
as was said, the precise description of filter is "1-to-0..1".


I'm still curious about the idea of an "empty" input?. Seems like "many" 
on the left side is never "0", in particular something like: 
"1..N-to-SomeRange"


Thanks to all,
-ernie

PS. Looking at flatMap (and mapMulti) was a good exercise.

On 24/04/07 11:24 AM, Holo The Sage Wolf wrote:
Think about `flatMap` (which filter is a special case of), this is 
one-to-many function that can also return 1 or 0 elements for each 
input. So it makes sense to include 0 as "many".


Filter isn't many-to-... At all, at no point it collapses a 
subsequence of elements, it only works on one element at a time


On Sun, 7 Apr 2024, 19:31 Ernie Rael,  wrote:

On 24/04/07 9:11 AM, Viktor Klang wrote:

Hi Ernie,

"Many" in this case refers to "N", which is "0 ... N",

OK, I was wondering about "many" including "0".

so I'd say while it is techincally correct as-is, perhaps more
precise would be to say "1-to-0..1" gatherer, since for every
element in, there is 0 or 1 element out.

I see.


Many-to-one would be 0..N -> 1, which means that an empty input
would

"could" not "would"?

yield a single output.


Out of curiosity, is either correct technically?


Other than at  initialization or finish, is it possible to have an
"empty" input?


-ernie



Cheers,
√

*
*
*Viktor Klang*
Software Architect, Java Platform Group
Oracle

*From:* core-libs-dev 
 on behalf of Ernie Rael
 
*Sent:* Sunday, 7 April 2024 18:06
*To:* core-libs-dev@openjdk.org 

*Subject:* JEP 473: Stream Gatherers (Second Preview)

This is about what might be a minor doc issue.

In https://openjdk.org/jeps/473 it says


As another example, |Stream::filter| takes a predicate that
determines whether an input element should be passed downstream;
this is simply a stateless one-to-many gatherer.

Shouldn't this be "many-to-one"?

-ernie

Integrated: 8329787: Fix typo in CLDRConverter

[Naoto Sato]

On Fri, 5 Apr 2024 17:22:36 GMT, Naoto Sato  wrote:

> Fix a file/class name in the `CLDRConverter` build tool, with some clean-up 
> for a switch statement.

This pull request has now been integrated.

Changeset: dd930c57
Author:Naoto Sato 
URL:   
https://git.openjdk.org/jdk/commit/dd930c573b2822e7f55e9d1b9a945a023c3fdee6
Stats: 393 lines in 4 files changed: 186 ins; 203 del; 4 mod

8329787: Fix typo in CLDRConverter

Reviewed-by: jlu, iris, lancea, bpb

-

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

Re: RFR: 8329787: Fix typo in CLDRConverter

[Naoto Sato]

On Fri, 5 Apr 2024 17:22:36 GMT, Naoto Sato  wrote:

> Fix a file/class name in the `CLDRConverter` build tool, with some clean-up 
> for a switch statement.

Thanks for the reviews!

-

PR Comment: https://git.openjdk.org/jdk/pull/18662#issuecomment-2043177032

Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v9]

[Jan Lahoda]

> This is a patch for javac, that adds the Derived Record Creation expressions. 
> The current draft specification for the feature is:
> https://cr.openjdk.org/~gbierman/jep468/jep468-20240326/specs/derived-record-creation-jls.html
> 
> The current CSR is here:
> https://bugs.openjdk.org/browse/JDK-8328637
> 
> The patch is mostly straightforward, with two notable changes:
>  - there is a new `ElementKind.COMPONENT_LOCAL_VARIABLE`, as the 
> specification introduces this term, and it seems consistent with 
> `ElementKind.BINDING_VARIABLE` that was introduced some time ago.
>  - there are a bit broader changes in `Flow`, to facilitate the introduction 
> of variables without an explicit declaration for definite assignment and 
> effectively final computation.

Jan Lahoda has updated the pull request incrementally with one additional 
commit since the last revision:

  Adding tests as suggested.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18509/files
  - new: https://git.openjdk.org/jdk/pull/18509/files/14651358..e0930688

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18509=08
 - incr: https://webrevs.openjdk.org/?repo=jdk=18509=07-08

  Stats: 261 lines in 3 files changed: 257 ins; 0 del; 4 mod
  Patch: https://git.openjdk.org/jdk/pull/18509.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18509/head:pull/18509

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

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

[Raffaello Giulietti]

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.

Marked as reviewed by rgiulietti (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/18663#pullrequestreview-1986623356

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

[Raffaello Giulietti]

On Mon, 8 Apr 2024 13:46:03 GMT, Roger Riggs  wrote:

>> 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.

What I mean is that the VM limit might change and suddenly accept `MAX_VALUE` 
as an allowed array size (very unlikely, I guess). The test would then fail on 
`min + 1` because it expects a OOME which will not be thrown.

But that is very remote.

-

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

Re: RFR: 8324651: Compiler Implementation for Derived Record Creation (Preview) [v7]

[Maurizio Cimadamore]

On Fri, 5 Apr 2024 18:28:58 GMT, Jan Lahoda  wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/comp/Lower.java line 4443:
>> 
>>> 4441: }
>>> 4442: 
>>> 4443: @Override
>> 
>> As usual, I suggest to add some brief comment with the shape of the 
>> generated code.
>
> Done here:
> https://github.com/openjdk/jdk/pull/18509/commits/146513580f96abd5feb7886bd1191805cc93403b#diff-bc0df6dce7f74078bfca1e90bec75d7bb3d8b338933be725da78f0a8a7a0c78dR4445
> 
> Please let me know if that needs some tweaks.
> 
> Thanks for the comment!

Looks great!

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18509#discussion_r1555902226

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: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v13]

[Joachim Kern]

On Fri, 5 Apr 2024 18:14:36 GMT, Suchismith Roy  wrote:

>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - update tests
>  - update tests
>  - update tests

> The fix itself looks good except minor nits.
> 
> I have looked at the test together with Joachim. The test is not valid. It 
> only passes because 
> [b8ae4a8](https://github.com/openjdk/jdk/commit/b8ae4a8c0985d1763ac48ba78943d8b992d7be77)
>  has a bug. The lib `/usr/lib/libperfstat.a(shr_64.o)` has already been 
> loaded by hotspot and 
> [JDK-8320890](https://bugs.openjdk.org/browse/JDK-8320890) prevents the 
> loading attempt we are trying to test. This is a real problem because it 
> breaks the ability to load several members of the same library. @JoKern65: 
> Please file a bug for this problem.
> 
> Also note that `dlopen ("/usr/lib/libperfstat.a", RTLD_NOW);` fails with 
> "Exec format error". The library really requires a member specification.
> 
> If you need a test for this PR, I suggest copying an existing ".so" library 
> to e.g. "libdummyarchive.a" and use that. I think that it is possible to 
> implement the copy step in Java and to write it to the test's temporary 
> directory.

I Have created a PR (https://github.com/openjdk/jdk/pull/18676) to fix the 
mentioned problem. With this correction in place I can verify that your 
testcase does not properly work as is.

-

PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2042624487

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

[Jaikiran Pai]

On Mon, 8 Apr 2024 09:52:39 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.

Given that this is a observable change through a public API, I think a csr will 
be needed for this. I'll draft one shortly.

-

PR Comment: https://git.openjdk.org/jdk/pull/18674#issuecomment-2042488119

Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v13]

[Martin Doerr]

On Fri, 5 Apr 2024 18:14:36 GMT, Suchismith Roy  wrote:

>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - update tests
>  - update tests
>  - update tests

The fix itself looks good except minor nits.

I have looked at the test together with Joachim. The test is not valid. It only 
passes because 
https://github.com/openjdk/jdk/commit/b8ae4a8c0985d1763ac48ba78943d8b992d7be77 
has a bug. The lib `/usr/lib/libperfstat.a(shr_64.o)` has already been loaded 
by hotspot and [JDK-8320890](https://bugs.openjdk.org/browse/JDK-8320890) 
prevents the loading attempt we are trying to test. This is a real problem 
because it breaks the ability to load several members of the same library.
@JoKern65: Please file a bug for this problem.

If you need a test for this PR, I suggest copying an existing ".so" library to 
e.g. "libdummyarchive.a" and use that. I think that it is possible to implement 
the copy step in Java and to write it to the test's temporary directory.

src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 43:

> 41: static boolean loadLibraryOnlyIfPresent() {
> 42: return false;
> 43: }

Please insert an empty line.

-

Changes requested by mdoerr (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/17945#pullrequestreview-1986030389
PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r191905

Re: RFR: JDK-8319516 AIX System::loadLibrary needs support to load a shared library from an archive object [v13]

[Martin Doerr]

On Fri, 5 Apr 2024 18:14:36 GMT, Suchismith Roy  wrote:

>> Allow support for both .a and .so files in AIX.
>> If .so file is not found, allow fallback to .a extension.
>> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516)
>
> Suchismith Roy has updated the pull request incrementally with three 
> additional commits since the last revision:
> 
>  - update tests
>  - update tests
>  - update tests

src/java.base/aix/classes/jdk/internal/loader/ClassLoaderHelper.java line 40:

> 38: * This method returns false so that loading of shared library 
> continues if
> 39: * libname.so is not present.
> 40: */

The '*' should be in the same column. Please compare to ClassLoaderHelper.java 
for other OSs.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/17945#discussion_r191088

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

[Jaikiran Pai]

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.

-

Commit messages:
 - 8212895: ChronoField.INSTANT_SECONDS's range doesn't match the range of 
Instant

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

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

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

[Raffaello Giulietti]

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.

Code fix looks good, but see the comment in the test.

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.

-

PR Review: https://git.openjdk.org/jdk/pull/18663#pullrequestreview-1985830221
PR Review Comment: https://git.openjdk.org/jdk/pull/18663#discussion_r1555466467