Re: RFR: 8336315: tools/jpackage/windows/WinChildProcessTest.java Failed: Check is calculator process is alive [v2]

[Vanitha B P]

> tools/jpackage/windows/WinChildProcessTest.java was failing intermittently, 
> fixed the issue and changes are tested.

Vanitha B P has updated the pull request incrementally with one additional 
commit since the last revision:

  Addressed the review comments

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20312/files
  - new: https://git.openjdk.org/jdk/pull/20312/files/5a2dcd3d..d3cae3b9

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

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

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

Re: RFR: 8336856: Optimize String Concat [v14]

[Shaojin Wen]

> The current implementation of StringConcat is to mix the coder and length 
> into a long. This operation will have some overhead for int/long/boolean 
> types. We can separate the calculation of the coder from the calculation of 
> the length, which can improve the performance in the scenario of concat 
> int/long/boolean.
> 
> This idea comes from the suggestion of @l4es in the discussion of PR 
> https://github.com/openjdk/jdk/pull/20253#issuecomment-2240412866

Shaojin Wen has updated the pull request incrementally with one additional 
commit since the last revision:

  Descending mixer, same order as prepend

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20273/files
  - new: https://git.openjdk.org/jdk/pull/20273/files/a0cdef4d..e430a417

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=13
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=12-13

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

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

Re: RFR: 8334342: Add MergeStore JMH benchmarks [v7]

[Emanuel Peter]

On Wed, 24 Jul 2024 21:59:52 GMT, Shaojin Wen  wrote:

>> Shaojin Wen 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 16 additional 
>> commits since the last revision:
>> 
>>  - Merge remote-tracking branch 'upstream/master' into merge_store_bench
>>  - move to vm.compiler
>>  - Merge remote-tracking branch 'upstream/master' into merge_store_bench
>>  - Merge remote-tracking branch 'upstream/master' into merge_store_bench
>>  - bug fix for `putChars4C`
>>  - bug fix for `putChars4C` and `putChars4S`
>>  - use VarHandler CHAR_L & CHAR_B
>>  - copyright
>>  - bug fix for putIntBU
>>  - add cases for `getChar` & `putChar`
>>  - ... and 6 more: https://git.openjdk.org/jdk/compare/19a79110...d00654ff
>
> Here are the performance numbers running on the new MacBook M1 Pro,
> 
> * Test scenarios with significant performance improvements
> 
> BenchmarkMode  Cnt  Score-OldScore-NewUnits
> MergeStoreBench.putChars4BU  avgt   15  10266.1233830.198 *   ns/op
> MergeStoreBench.putChars4LU  avgt   15  10266.2383827.784 *   ns/op
> MergeStoreBench.setIntLU avgt   15   5103.5622573.624 *   ns/op
> MergeStoreBench.setLongLUavgt   15  10304.0122921.575 *   ns/op
> MergeStoreBench.setLongRLU   avgt   15  10263.9753241.057 *   ns/op
> 
> 
> * 
> 
> BenchmarkMode  Cnt  Score-OldScore-NewUnits
> MergeStoreBench.getCharB avgt   15   5341.7875340.200 ns/op
> MergeStoreBench.getCharBUavgt   15   5477.3635482.163 ns/op
> MergeStoreBench.getCharBVavgt   15   5163.0995074.165 ns/op
> MergeStoreBench.getCharC avgt   15   5068.7085051.763 ns/op
> MergeStoreBench.getCharL avgt   15   5379.8215374.464 ns/op
> MergeStoreBench.getCharLUavgt   15   5477.2685487.532 ns/op
> MergeStoreBench.getCharLVavgt   15   5079.0455071.263 ns/op
> MergeStoreBench.getIntB  avgt   15   6276.5486277.984 ns/op
> MergeStoreBench.getIntBU avgt   15   5229.8135232.984 ns/op
> MergeStoreBench.getIntBV avgt   15   1207.8681206.264 ns/op
> MergeStoreBench.getIntL  avgt   15   6182.1506172.779 ns/op
> MergeStoreBench.getIntLU avgt   15   5164.2605157.317 ns/op
> MergeStoreBench.getIntLV avgt   15   2555.4432558.110 ns/op
> MergeStoreBench.getIntRB avgt   15   6879.1886889.916 ns/op
> MergeStoreBench.getIntRBUavgt   15   5771.8575769.950 ns/op
> MergeStoreBench.getIntRL avgt   15   6625.7546625.605 ns/op
> MergeStoreBench.getIntRLUavgt   15   5746.5545746.742 ns/op
> MergeStoreBench.getIntRU avgt   15   2547.4492544.586 ns/op
> MergeStoreBench.getIntU  avgt   15   2543.5522541.119 ns/op
> MergeStoreBench.getLongB avgt   15  12099.002   12098.129 ns/op
> MergeStoreBench.getLongBUavgt   15   9771.8939760.621 ns/op
> MergeStoreBench.getLongBVavgt   15   2593.8352593.635 ns/op
> MergeStoreBench.getLongL avgt   15  12045.235   12031.065 ns/op
> MergeStoreBench.getLongLUavgt   15   9659.5859653.938 ns/op
> MergeStoreBench.getLongLVavgt   15   2561.0892557.521 ns/op
> MergeStoreBench.getLongRBavgt   15  12095.060   12092.061...

@wenshao generally we like to have at least 2 reviews before integration ;)

-

PR Comment: https://git.openjdk.org/jdk/pull/19734#issuecomment-2249791741

Re: RFR: 8336856: Optimize String Concat [v15]

[Shaojin Wen]

> The current implementation of StringConcat is to mix the coder and length 
> into a long. This operation will have some overhead for int/long/boolean 
> types. We can separate the calculation of the coder from the calculation of 
> the length, which can improve the performance in the scenario of concat 
> int/long/boolean.
> 
> This idea comes from the suggestion of @l4es in the discussion of PR 
> https://github.com/openjdk/jdk/pull/20253#issuecomment-2240412866

Shaojin Wen has updated the pull request incrementally with one additional 
commit since the last revision:

  add benchmark

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20273/files
  - new: https://git.openjdk.org/jdk/pull/20273/files/e430a417..00fcd554

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=14
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=13-14

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

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

Integrated: 8334342: Add MergeStore JMH benchmarks

[Shaojin Wen]

On Sun, 16 Jun 2024 07:17:16 GMT, Shaojin Wen  wrote:

> [8318446](https://github.com/openjdk/jdk/pull/16245)  brings MergeStore. We 
> need a JMH Benchmark to evaluate the performance of various batch operations 
> and the effect of MergeStore.

This pull request has now been integrated.

Changeset: 8081f870
Author:Shaojin Wen 
Committer: Tobias Hartmann 
URL:   
https://git.openjdk.org/jdk/commit/8081f8702a89c4895302377be62da22fc34f2c74
Stats: 1132 lines in 1 file changed: 1132 ins; 0 del; 0 mod

8334342: Add MergeStore JMH benchmarks

Reviewed-by: epeter, thartmann

-

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

Re: RFR: 8334342: Add MergeStore JMH benchmarks [v7]

[Tobias Hartmann]

On Wed, 24 Jul 2024 16:00:10 GMT, Shaojin Wen  wrote:

>> [8318446](https://github.com/openjdk/jdk/pull/16245)  brings MergeStore. We 
>> need a JMH Benchmark to evaluate the performance of various batch operations 
>> and the effect of MergeStore.
>
> Shaojin Wen 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 16 additional 
> commits since the last revision:
> 
>  - Merge remote-tracking branch 'upstream/master' into merge_store_bench
>  - move to vm.compiler
>  - Merge remote-tracking branch 'upstream/master' into merge_store_bench
>  - Merge remote-tracking branch 'upstream/master' into merge_store_bench
>  - bug fix for `putChars4C`
>  - bug fix for `putChars4C` and `putChars4S`
>  - use VarHandler CHAR_L & CHAR_B
>  - copyright
>  - bug fix for putIntBU
>  - add cases for `getChar` & `putChar`
>  - ... and 6 more: https://git.openjdk.org/jdk/compare/3610715a...d00654ff

Looks good to me too.

-

Marked as reviewed by thartmann (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19734#pullrequestreview-2198766610

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[SendaoYan]

> Hi all,
> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails on 
> rpm build mock environment. The `df -h` command return fail `df: cannot read 
> table of mounted file systems: No such file or directory` on the rpm build 
> mock environment also. I think it's a environmental issue, and the 
> environmental issue should not cause the test fails, it should skip the test.
> 
> Only change the testcase, the change has been verified locally, no risk.

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

  1. Just catch the IOException here when getting the FileStore and skip the 
test instead of checking for specific exception messages. 2. Throw a 
org.testng.SkipException instead print and return

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19905/files
  - new: https://git.openjdk.org/jdk/pull/19905/files/8931debe..9c7946f8

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

  Stats: 17 lines in 1 file changed: 1 ins; 11 del; 5 mod
  Patch: https://git.openjdk.org/jdk/pull/19905.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19905/head:pull/19905

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

Re: RFR: 8337168: Optimize LocalDateTime.toString

[Chen Liang]

On Thu, 25 Jul 2024 05:06:17 GMT, Shaojin Wen  wrote:

> The current LocalDateTime.toString implementation concatenates the toString 
> results of date and time, which can be passed to StringBuilder to reduce 
> additional object allocation and improve performance.

Simple and straightforward improvement.

-

Marked as reviewed by liach (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20321#pullrequestreview-2198838834

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v2]

[SendaoYan]

On Thu, 25 Jul 2024 04:53:33 GMT, Jaikiran Pai  wrote:

>> SendaoYan has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   add a word throw
>
> test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java line 216:
> 
>> 214: } catch (IOException e) {
>> 215: if (e.getMessage().contains("Mount point not found")) {
>> 216: // We would like to skip the test with a cause with
> 
> Hello @sendaoYan, it feels very specific and odd to be checking only for this 
> exception message. This test method's goal appears to be to create a 
> read-only directory into which it wants to write out the proxy classes and 
> verify that it won't be able to do that. For that it first verifies that the 
> underlying `FileStore` supports posix file attributes. If it's not able to 
> ascertain that the underlying `FileStore` has posix support, then it skips 
> the test.
> 
> So I think we should just catch the `IOException` here when getting the 
> FileStore and skip the test instead of checking for specific exception 
> messages. While we are at it, we should throw a `org.testng.SkipException` 
> (this is a testng test) from this method wherever we are currently skipping 
> the test execution by writing out a System.out warning message and returning.

Thanks for your advice, the checking for specific exception messages has been 
replaced to just catch the IOException when getting the FileStore, and all the 
`System.out warning message and returning` has been replaced to 
`org.testng.SkipException`.
Additional, the `org.testng.SkipException` seems do not work normally in 
[jtreg](https://bugs.openjdk.org/browse/CODETOOLS-7903708) for now.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19905#discussion_r1691168794

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava

[Alan Bateman]

On Wed, 24 Jul 2024 19:11:42 GMT, Brian Burkhalter  wrote:

> This proposed change would move the native objects required for NIO file 
> interaction from the libnio native library to the libjava native library on 
> Linux, macOS, and Windows.

I think this will require thinking about how to organize the native code in 
native/libjava as it hard to maintain if everything is in the same directory. 
We may have to create subdirectories, as we do in native/libnio.

Also think to work through some naming on IOUtil vs. NIOUtil as it won't be 
obvious to maintainers which class to use.

-

PR Comment: https://git.openjdk.org/jdk/pull/20317#issuecomment-2249958450

Re: RFR: 8336856: Optimize String Concat [v15]

[Claes Redestad]

On Thu, 25 Jul 2024 08:58:14 GMT, Shaojin Wen  wrote:

>> The current implementation of StringConcat is to mix the coder and length 
>> into a long. This operation will have some overhead for int/long/boolean 
>> types. We can separate the calculation of the coder from the calculation of 
>> the length, which can improve the performance in the scenario of concat 
>> int/long/boolean.
>> 
>> This idea comes from the suggestion of @l4es in the discussion of PR 
>> https://github.com/openjdk/jdk/pull/20253#issuecomment-2240412866
>
> Shaojin Wen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add benchmark

test/micro/org/openjdk/bench/java/lang/StringConcatGenerate.java line 47:

> 45: @Measurement(iterations = 5, time = 1000, timeUnit = 
> TimeUnit.MILLISECONDS)
> 46: @Fork(value = 3, jvmArgsAppend = 
> "-Djava.lang.invoke.StringConcat.highArityThreshold=0")
> 47: public class StringConcatGenerate extends StringConcat {

Adding a subclass with an overridden `@Fork` to pass a different 
`jvmArgsAppend` is a reasonable trick, but could be moved to a nested class 
within `StringConcat` to keep it in the same context. I'm not sure if this 
micro brings a persistent added value, though: for experimentation we can just 
run `StringConcat` with different 
`-Djava.lang.invoke.StringConcat.highArityThreshold` settings, while for 
continuous regression testing we're more interested in validating the default 
settings. Supplying the new `main` method doesn't add anything here, either, 
since a standalone execution wouldn't pick up the `jvmArgsAppend` value.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1691205621

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[Alan Bateman]

On Thu, 25 Jul 2024 09:50:10 GMT, SendaoYan  wrote:

>> Hi all,
>> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails 
>> on rpm build mock environment. The `df -h` command return fail `df: cannot 
>> read table of mounted file systems: No such file or directory` on the rpm 
>> build mock environment also. I think it's a environmental issue, and the 
>> environmental issue should not cause the test fails, it should skip the test.
>> 
>> Only change the testcase, the change has been verified locally, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   1. Just catch the IOException here when getting the FileStore and skip the 
> test instead of checking for specific exception messages. 2. Throw a 
> org.testng.SkipException instead print and return

Would it possible to provide a summary on how Mock works and how we end up with 
the current directory in a location that doesn't have a mount point?

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2249991703

Re: RFR: 8337167: StringSize deduplication

[Chen Liang]

On Thu, 25 Jul 2024 05:30:34 GMT, Shaojin Wen  wrote:

> Integer.stringSize and Long.stringSize are used not only in java.lang, but 
> also in other places. We put stringSize in DecimalDigits to reduce duplicate 
> code and the use of JLA.

src/java.base/share/classes/java/lang/Integer.java line 472:

> 470:  * code after loop unrolling.
> 471:  */
> 472: static int stringSize(int x) {

Same question, if we can just inline this

src/java.base/share/classes/java/lang/Long.java line 502:

> 500:  * code after loop unrolling.
> 501:  */
> 502: static int stringSize(long x) {

Can we just remove this method?

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20322#discussion_r1691241572
PR Review Comment: https://git.openjdk.org/jdk/pull/20322#discussion_r1691240301

RFR: 8337060: Test java/foreign/TestConcurrentClose.java failed: IllegalStateException: SegmentAccessor::doAccess method not being compiled

[Jorn Vernee]

We are seeing some failures of this test in our CI, particularly on Windows 
machines, which are known to starve out certain threads.

To make sure the compiler threads get a chance to work, I've added `-Xbatch` to 
the flags with which the test runs (this was missed in the original PR). I've 
also removed the timeout from the `awaitCompilation` method. The intent is to 
instead rely on the overall test's timeout, which can also be adjusted with a 
timeout factor for slower machines, rather than putting an absolute time limit 
on the compilation.

Finally, Alan asked me to increase the timeout for the executor shutdown, since 
there have also been some timeouts there. The current timeout is borrowed from 
`test/jdk/java/foreign/TestHandshake`, but that test uses a lot less threads, 
so it can probably get await with waiting just 20 seconds.

Testing: I ran this 200s time in Oracle CI, half of the runs were with 
`-Duse.JTREG_TEST_THREAD_FACTORY=Virtual -XX:-VerifyContinuations` (which is 
one of the failing configurations).

-

Commit messages:
 - use -Xbatch + rely on test timeout

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

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

Re: RFR: 8337060: Test java/foreign/TestConcurrentClose.java failed: IllegalStateException: SegmentAccessor::doAccess method not being compiled

[Maurizio Cimadamore]

On Thu, 25 Jul 2024 10:58:42 GMT, Jorn Vernee  wrote:

> We are seeing some failures of this test in our CI, particularly on Windows 
> machines, which are known to starve out certain threads.
> 
> To make sure the compiler threads get a chance to work, I've added `-Xbatch` 
> to the flags with which the test runs (this was missed in the original PR). 
> I've also removed the timeout from the `awaitCompilation` method. The intent 
> is to instead rely on the overall test's timeout, which can also be adjusted 
> with a timeout factor for slower machines, rather than putting an absolute 
> time limit on the compilation.
> 
> Finally, Alan asked me to increase the timeout for the executor shutdown, 
> since there have also been some timeouts there. The current timeout is 
> borrowed from `test/jdk/java/foreign/TestHandshake`, but that test uses a lot 
> less threads, so it can probably get await with waiting just 20 seconds.
> 
> Testing: I ran this 200s time in Oracle CI, half of the runs were with 
> `-Duse.JTREG_TEST_THREAD_FACTORY=Virtual -XX:-VerifyContinuations` (which is 
> one of the failing configurations).

Looks good.

-

Marked as reviewed by mcimadamore (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20325#pullrequestreview-2199057092

Re: Should the documentation state peekFirst() as equivalent to Stack's peek()?

[Chen Liang]

Hi Turkhan, this mail belongs to core-libs-dev list. I have forwarded your mail 
to the right list.

Indeed, we should claim that peek() is equivalent to peekFirst(); the 
information in stack section should be a typo, as peek() being the same as 
peekFirst() is claimed by the deque section and the peek() specification.

I have created a ticket on the Java Bug System to track this issue: 
[JDK-8337205] Typo in Stack vs Deque Method table in Deque specification - Java 
Bug System (openjdk.org)

Feel free to open a pull request to jdk to fix this bug.

Best, Chen Liang

From: jdk-dev  on behalf of Turkhan Badalov 

Sent: Thursday, July 25, 2024 4:58 AM
To: jdk-...@openjdk.org 
Subject: Should the documentation state peekFirst() as equivalent to Stack's 
peek()?

Here is the table "Comparison of Stack and Deque methods" that lists equivalent 
Deque methods compared to Stack methods: 
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/Deque.html

At the moment, getFirst() is said to be equivalent to peek(). Since peek() 
doesn't throw an exception when the queue/stack is empty, peekFirst() could be 
a better equivalent because getFirst() throws.

In fact, the documentation of the peek() method itself says that this method is 
equivalent to peekFirst(): 
https://docs.oracle.com/en/java/javase/22/docs/api/java.base/java/util/Deque.html#peek().

If this should be posted somewhere else, please let me know. I am still new to 
using mailing lists. Cheers.

Re: RFR: 8337167: StringSize deduplication

[Chen Liang]

On Thu, 25 Jul 2024 05:30:34 GMT, Shaojin Wen  wrote:

> Integer.stringSize and Long.stringSize are used not only in java.lang, but 
> also in other places. We put stringSize in DecimalDigits to reduce duplicate 
> code and the use of JLA.

src/java.base/share/classes/jdk/internal/access/JavaLangAccess.java line 462:

> 460: 
> 461: int stringSize(long i);
> 462: 

👍 Removing extra dependencies on `JavaLangAccess`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20322#discussion_r1691333555

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[SendaoYan]

On Thu, 25 Jul 2024 10:23:29 GMT, Alan Bateman  wrote:

> Would it possible to provide a summary on how Mock works and how we end up 
> with the current directory in a location that doesn't have a mount point?

The rpmbuild mock enviroment is like a sandbox, which created by `chroot` shell 
command, in the rpmbuild mock enviroment, `df -h` report `cannot read table of 
mounted file systems`, and java Files.getFileStore also throw `IOException`. We 
want to build and test the jdk in this `sandbox`, and the default jtreg work 
directory is `JTWork` in current directory, so this testcase will report fails.

![image](https://github.com/user-attachments/assets/693928c6-9080-4cfe-b90b-cc0d3133478a)

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2250225244

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[SendaoYan]

On Thu, 25 Jul 2024 09:50:10 GMT, SendaoYan  wrote:

>> Hi all,
>> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails 
>> on rpm build mock environment. The `df -h` command return fail `df: cannot 
>> read table of mounted file systems: No such file or directory` on the rpm 
>> build mock environment also. I think it's a environmental issue, and the 
>> environmental issue should not cause the test fails, it should skip the test.
>> 
>> The rpmbuild mock enviroment is like a sandbox, which created by `chroot` 
>> shell command, in the rpmbuild mock enviroment, `df -h` report `cannot read 
>> table of mounted file systems`, and java Files.getFileStore also throw 
>> `IOException`. We want to build and test the jdk in this `sandbox`, and the 
>> default jtreg work directory is `JTWork` in current directory, so this 
>> testcase will report fails.
>> 
>> Only change the testcase, the change has been verified locally, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   1. Just catch the IOException here when getting the FileStore and skip the 
> test instead of checking for specific exception messages. 2. Throw a 
> org.testng.SkipException instead print and return

GHA report a failure:
linux x86 `compiler/interpreter/Test6833129.java` fails `SIGSEGV  in 
oopDesc::size_given_klass`, this issue has been recorded by 
[JDK-8334760](https://bugs.openjdk.org/browse/JDK-8334760), it's unrelated to 
this PR.

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2250234321

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[Alan Bateman]

On Thu, 25 Jul 2024 12:39:35 GMT, SendaoYan  wrote:

> The rpmbuild mock enviroment is like a sandbox, which created by `chroot` 
> shell command, in the rpmbuild mock enviroment, `df -h` report `cannot read 
> table of mounted file systems`, and java Files.getFileStore also throw 
> `IOException`. We want to build and test the jdk in this `sandbox`, and the 
> default jtreg work directory is `JTWork` in current directory, so this 
> testcase will report fails.

Okay, but doesn't mean that lots of other tests will fail too, esp. tests in 
jdk_nio test group.

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2250241338

Re: RFR: 8337060: Test java/foreign/TestConcurrentClose.java failed: IllegalStateException: SegmentAccessor::doAccess method not being compiled

[Alan Bateman]

On Thu, 25 Jul 2024 10:58:42 GMT, Jorn Vernee  wrote:

> We are seeing some failures of this test in our CI, particularly on Windows 
> machines, which are known to starve out certain threads.
> 
> To make sure the compiler threads get a chance to work, I've added `-Xbatch` 
> to the flags with which the test runs (this was missed in the original PR). 
> I've also removed the timeout from the `awaitCompilation` method. The intent 
> is to instead rely on the overall test's timeout, which can also be adjusted 
> with a timeout factor for slower machines, rather than putting an absolute 
> time limit on the compilation.
> 
> Finally, Alan asked me to increase the timeout for the executor shutdown, 
> since there have also been some timeouts there. The current timeout is 
> borrowed from `test/jdk/java/foreign/TestHandshake`, but that test uses a lot 
> less threads, so it can probably get await with waiting just 20 seconds.
> 
> Testing: I ran this 200s time in Oracle CI, half of the runs were with 
> `-Duse.JTREG_TEST_THREAD_FACTORY=Virtual -XX:-VerifyContinuations` (which is 
> one of the failing configurations).

Marked as reviewed by alanb (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20325#pullrequestreview-2199207970

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[SendaoYan]

On Thu, 25 Jul 2024 12:48:20 GMT, Alan Bateman  wrote:

> Okay, but doesn't mean that lots of other tests will fail too, esp. tests in 
> jdk_nio test group.

Currently we observer only this test fails of tier1.

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2250252865

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[Alan Bateman]

On Thu, 25 Jul 2024 12:54:16 GMT, SendaoYan  wrote:

> > Okay, but doesn't mean that lots of other tests will fail too, esp. tests 
> > in jdk_nio test group.
> 
> Currently we observer only this test fails of tier1.

Is this because you only run tier1 or do you mean this is the only test that 
fails?

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2250315338

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[SendaoYan]

On Thu, 25 Jul 2024 13:23:37 GMT, Alan Bateman  wrote:

> Is this because you only run tier1 or do you mean this is the only test that 
> fails?

We only run tier1 on rpmbuild mock enviroment.

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2250369276

Re: RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v6]

[Andrew Haley]

On Wed, 24 Jul 2024 19:09:06 GMT, Vladimir Ivanov  wrote:

>>> > Also also, Klass::is_subtype_of() is used for C1 runtime.
>>> 
>>> Can you elaborate, please?
>> 
>> Sorry, that was rather vague. In C1-compiled code, the Java method 
>> `Class::isInstance(Object)`calls `Klass::is_subtype_of()`. 
>> 
>> In general, I find it difficult to decide how much work, if any, should be 
>> done to improve C1 performance. Clearly, if C1 exists only to help with 
>> startup time in a tiered compilation system, the answer is "not much".
>
> Thanks, now I see that `Class::isInstance(Object)` is backed by 
> `Runtime1::is_instance_of()` which uses `oopDesc::is_a()` to do the job.
> 
> If it turns out to be performance critical, the intrinsic implementation 
> should be rewritten to exercise existing subtype checking support in C1. As 
> it is implemented now, it's already quite inefficient.

I did write an intrinsic for that, but it made this patch even larger. I have a 
small patch for C1, for some other time.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1691491950

Re: RFR: 8337167: StringSize deduplication [v2]

[Shaojin Wen]

> Integer.stringSize and Long.stringSize are used not only in java.lang, but 
> also in other places. We put stringSize in DecimalDigits to reduce duplicate 
> code and the use of JLA.

Shaojin Wen has updated the pull request incrementally with one additional 
commit since the last revision:

  remove unused Integer.stringSize & Long.stringSize

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20322/files
  - new: https://git.openjdk.org/jdk/pull/20322/files/752279f0..a3045712

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

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

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

Re: RFR: 8337167: StringSize deduplication [v2]

[Chen Liang]

On Thu, 25 Jul 2024 14:29:05 GMT, Shaojin Wen  wrote:

>> Integer.stringSize and Long.stringSize are used not only in java.lang, but 
>> also in other places. We put stringSize in DecimalDigits to reduce duplicate 
>> code and the use of JLA.
>
> Shaojin Wen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove unused Integer.stringSize & Long.stringSize

Marked as reviewed by liach (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20322#pullrequestreview-2199522887

Re: RFR: 8337167: StringSize deduplication [v2]

[Roger Riggs]

On Thu, 25 Jul 2024 14:29:05 GMT, Shaojin Wen  wrote:

>> Integer.stringSize and Long.stringSize are used not only in java.lang, but 
>> also in other places. We put stringSize in DecimalDigits to reduce duplicate 
>> code and the use of JLA.
>
> Shaojin Wen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove unused Integer.stringSize & Long.stringSize

What is the performance difference between the long and int versions?
If it is negligible, keep only the long version.

-

PR Comment: https://git.openjdk.org/jdk/pull/20322#issuecomment-2250535851

Re: RFR: 8336856: Optimize String Concat [v16]

[Shaojin Wen]

> The current implementation of StringConcat is to mix the coder and length 
> into a long. This operation will have some overhead for int/long/boolean 
> types. We can separate the calculation of the coder from the calculation of 
> the length, which can improve the performance in the scenario of concat 
> int/long/boolean.
> 
> This idea comes from the suggestion of @l4es in the discussion of PR 
> https://github.com/openjdk/jdk/pull/20253#issuecomment-2240412866

Shaojin Wen has updated the pull request incrementally with two additional 
commits since the last revision:

 - remove benchmark
 - common simpleConcat

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20273/files
  - new: https://git.openjdk.org/jdk/pull/20273/files/00fcd554..f1898af9

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=15
 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=20273&range=14-15

  Stats: 175 lines in 3 files changed: 20 ins; 149 del; 6 mod
  Patch: https://git.openjdk.org/jdk/pull/20273.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20273/head:pull/20273

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

Re: RFR: 8337167: StringSize deduplication [v2]

[Chen Liang]

On Thu, 25 Jul 2024 14:48:29 GMT, Roger Riggs  wrote:

>> Shaojin Wen has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   remove unused Integer.stringSize & Long.stringSize
>
> What is the performance difference between the long and int versions?
> If it is negligible, keep only the long version.

@RogerRiggs They will be used in MethodHandle chains or explicit bytecode; 
having `long` and `int` specific versions simplify the bytecode or MethodHandle 
we need to prepare for String concatenation.

-

PR Comment: https://git.openjdk.org/jdk/pull/20322#issuecomment-2250575109

Re: RFR: 8337167: StringSize deduplication [v2]

[Shaojin Wen]

On Thu, 25 Jul 2024 14:48:29 GMT, Roger Riggs  wrote:

>> Shaojin Wen has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   remove unused Integer.stringSize & Long.stringSize
>
> What is the performance difference between the long and int versions?
> If it is negligible, keep only the long version.

@RogerRiggs The risk of removing stringSize(int) is not low, and the 
performance may be different on different platforms. If we want to remove it, 
it should be a separate PR, with extensive testing and extensive discussion 
before doing it.

-

PR Comment: https://git.openjdk.org/jdk/pull/20322#issuecomment-2250622673

Re: RFR: 8337168: Optimize LocalDateTime.toString

[duke]

On Thu, 25 Jul 2024 05:06:17 GMT, Shaojin Wen  wrote:

> The current LocalDateTime.toString implementation concatenates the toString 
> results of date and time, which can be passed to StringBuilder to reduce 
> additional object allocation and improve performance.

@wenshao 
Your change (at version c03ed26ffdf2cebcc9074c2aeae27fbb1f49c025) is now ready 
to be sponsored by a Committer.

-

PR Comment: https://git.openjdk.org/jdk/pull/20321#issuecomment-2250640401

[jdk23] RFR: 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out

[Jaikiran Pai]

Can I please get a review of this test-only backport of the fix that we did in 
master branch a few weeks back?

This isn't a clean backport, there were trivial merge issues in the 
`NativeMethodPrefixApp` test class, which I have resolved manually. The merged 
content matches with what we have in master branch. The original fix was done 
in https://github.com/openjdk/jdk/pull/20154 and we have seen this test fail 
since it was integrated.

Backporting this to jdk23 branch will provide test stability in that branch 
where currently this test occasionally times out.

-

Commit messages:
 - 8334167: Test java/lang/instrument/NativeMethodPrefixApp.java timed out

Changes: https://git.openjdk.org/jdk/pull/20333/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20333&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8334167
  Stats: 144 lines in 3 files changed: 82 ins; 40 del; 22 mod
  Patch: https://git.openjdk.org/jdk/pull/20333.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20333/head:pull/20333

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

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava

[Brian Burkhalter]

On Wed, 24 Jul 2024 19:11:42 GMT, Brian Burkhalter  wrote:

> This proposed change would move the native objects required for NIO file 
> interaction from the libnio native library to the libjava native library on 
> Linux, macOS, and Windows.

Yes, I was wondering about changing the organization of the native code files. 
It's not great to have them all in `/libjava`.

-

PR Comment: https://git.openjdk.org/jdk/pull/20317#issuecomment-2250654680

Re: RFR: 8337168: Optimize LocalDateTime.toString

[Chen Liang]

On Thu, 25 Jul 2024 06:19:46 GMT, Shaojin Wen  wrote:

>> The current LocalDateTime.toString implementation concatenates the toString 
>> results of date and time, which can be passed to StringBuilder to reduce 
>> additional object allocation and improve performance.
>
> Below are the performance numbers running on a MacBook M1 Max, which 
> increased speed by +179.88%
> 
> 
> -# master 05d88de05e9b7814ecd5517aacd17f0feafdff3c
> -Benchmark Mode  Cnt  Score   Error   Units
> -ToStringBench.localDateTimeToString  thrpt   15  4.896 ? 1.414  ops/ms
> 
> +# current c03ed26ffdf2cebcc9074c2aeae27fbb1f49c025
> +Benchmark Mode  Cnt   Score   Error   Units
> +ToStringBench.localDateTimeToString  thrpt   15  13.703 ? 1.960  ops/ms 
> +179.88%

@wenshao Please give other reviewers (especially those maintaining the locale 
area) some time to take a look: https://openjdk.org/guide/#life-of-a-pr 
recommends to wait for 24 hours for reviewers before integrating a pull request.

Also, ran tier 1-3 tests for this patch and looks good.

-

PR Comment: https://git.openjdk.org/jdk/pull/20321#issuecomment-2250698964

Re: RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v7]

[Andrew Haley]

> This patch expands the use of a hash table for secondary superclasses
> to the interpreter, C1, and runtime. It also adds a C2 implementation
> of hashed lookup in cases where the superclass isn't known at compile
> time.
> 
> HotSpot shared runtime
> --
> 
> Building hashed secondary tables is now unconditional. It takes very
> little time, and now that the shared runtime always has the tables, it
> might as well take advantage of them. The shared code is easier to
> follow now, I think.
> 
> There might be a performance issue with x86-64 in that we build
> HotSpot for a default x86-64 target that does not support popcount.
> This means that HotSpot C++ runtime on x86 always uses a software
> emulation for popcount, even though the vast majority of machines made
> for the past 20 years can do popcount in a single instruction. It
> wouldn't be terribly hard to do something about that.
> 
> Having said that, the software popcount is really not bad.
> 
> x86
> ---
> 
> x86 is rather tricky, because we still support
> `-XX:-UseSecondarySupersTable` and `-XX:+UseSecondarySupersCache`, as
> well as 32- and 64-bit ports. There's some further complication in
> that only `RCX` can be used as a shift count, so there's some register
> shuffling to do. All of this makes the logic in macroAssembler_x86.cpp
> rather gnarly, with multiple levels of conditionals at compile time
> and runtime.
> 
> AArch64
> ---
> 
> AArch64 is considerably more straightforward. We always have a
> popcount instruction and (thankfully) no 32-bit code to worry about.
> 
> Generally
> -
> 
> I would dearly love simply to rip out the "old" secondary supers cache
> support, but I've left it in just in case someone has a performance
> regression.
> 
> The versions of `MacroAssembler::lookup_secondary_supers_table` that
> work with variable superclasses don't take a fixed set of temp
> registers, and neither do they call out to to a slow path subroutine.
> Instead, the slow patch is expanded inline.
> 
> I don't think this is necessarily bad. Apart from the very rare cases
> where C2 can't determine the superclass to search for at compile time,
> this code is only used for generating stubs, and it seemed to me
> ridiculous to have stubs calling other stubs.
> 
> I've followed the guidance from @iwanowww not to obsess too much about
> the performance of C1-compiled secondary supers lookups, and to prefer
> simplicity over absolute performance. Nonetheless, this is a
> complicated patch that touches many areas.

Andrew Haley has updated the pull request incrementally with four additional 
commits since the last revision:

 - Cleanup
 - temp
 - Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk into 
JDK-8331658-work
 - Minor cleanup

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19989/files
  - new: https://git.openjdk.org/jdk/pull/19989/files/48e80a13..011a3880

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

  Stats: 51 lines in 7 files changed: 21 ins; 18 del; 12 mod
  Patch: https://git.openjdk.org/jdk/pull/19989.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19989/head:pull/19989

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

Re: RFR: 8336315: tools/jpackage/windows/WinChildProcessTest.java Failed: Check is calculator process is alive [v2]

[Alexey Semenyuk]

On Thu, 25 Jul 2024 08:40:44 GMT, Vanitha B P  wrote:

>> tools/jpackage/windows/WinChildProcessTest.java was failing intermittently, 
>> fixed the issue and changes are tested.
>
> Vanitha B P has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed the review comments

Marked as reviewed by asemenyuk (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20312#pullrequestreview-2199685519

Re: RFR: 8336856: Optimize String Concat [v16]

[Shaojin Wen]

On Thu, 25 Jul 2024 14:52:11 GMT, Shaojin Wen  wrote:

>> The current implementation of StringConcat is to mix the coder and length 
>> into a long. This operation will have some overhead for int/long/boolean 
>> types. We can separate the calculation of the coder from the calculation of 
>> the length, which can improve the performance in the scenario of concat 
>> int/long/boolean.
>> 
>> This idea comes from the suggestion of @l4es in the discussion of PR 
>> https://github.com/openjdk/jdk/pull/20253#issuecomment-2240412866
>
> Shaojin Wen has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - remove benchmark
>  - common simpleConcat

Startup performance is better when simpleConcat works, so I use highArity and 
bytecode-spinning only when simpleConcat doesn't work.

Should we add some simpleConcat implementations, such as:


String simpleConcat(Object arg0, int arg1);
String simpleConcat(Object arg0, long arg1)
String simpleConcat(Object arg0, boolean arg1)
String simpleConcat(int arg0, Object arg1)
String simpleConcat(long arg0, Object arg1)
String simpleConcat(boolean arg0, Object arg1)


This will help with startup performance

-

PR Comment: https://git.openjdk.org/jdk/pull/20273#issuecomment-2250787254

Re: RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v8]

[Andrew Haley]

> This patch expands the use of a hash table for secondary superclasses
> to the interpreter, C1, and runtime. It also adds a C2 implementation
> of hashed lookup in cases where the superclass isn't known at compile
> time.
> 
> HotSpot shared runtime
> --
> 
> Building hashed secondary tables is now unconditional. It takes very
> little time, and now that the shared runtime always has the tables, it
> might as well take advantage of them. The shared code is easier to
> follow now, I think.
> 
> There might be a performance issue with x86-64 in that we build
> HotSpot for a default x86-64 target that does not support popcount.
> This means that HotSpot C++ runtime on x86 always uses a software
> emulation for popcount, even though the vast majority of machines made
> for the past 20 years can do popcount in a single instruction. It
> wouldn't be terribly hard to do something about that.
> 
> Having said that, the software popcount is really not bad.
> 
> x86
> ---
> 
> x86 is rather tricky, because we still support
> `-XX:-UseSecondarySupersTable` and `-XX:+UseSecondarySupersCache`, as
> well as 32- and 64-bit ports. There's some further complication in
> that only `RCX` can be used as a shift count, so there's some register
> shuffling to do. All of this makes the logic in macroAssembler_x86.cpp
> rather gnarly, with multiple levels of conditionals at compile time
> and runtime.
> 
> AArch64
> ---
> 
> AArch64 is considerably more straightforward. We always have a
> popcount instruction and (thankfully) no 32-bit code to worry about.
> 
> Generally
> -
> 
> I would dearly love simply to rip out the "old" secondary supers cache
> support, but I've left it in just in case someone has a performance
> regression.
> 
> The versions of `MacroAssembler::lookup_secondary_supers_table` that
> work with variable superclasses don't take a fixed set of temp
> registers, and neither do they call out to to a slow path subroutine.
> Instead, the slow patch is expanded inline.
> 
> I don't think this is necessarily bad. Apart from the very rare cases
> where C2 can't determine the superclass to search for at compile time,
> this code is only used for generating stubs, and it seemed to me
> ridiculous to have stubs calling other stubs.
> 
> I've followed the guidance from @iwanowww not to obsess too much about
> the performance of C1-compiled secondary supers lookups, and to prefer
> simplicity over absolute performance. Nonetheless, this is a
> complicated patch that touches many areas.

Andrew Haley has updated the pull request with a new target base due to a merge 
or a rebase. The pull request now contains 40 commits:

 - Merge branch 'clean' into JDK-8331658-work
 - Cleanup
 - temp
 - Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk into 
JDK-8331658-work
 - Review comments
 - Review comments
 - Review comments
 - Review comments
 - Review feedback
 - Review feedback
 - ... and 30 more: https://git.openjdk.org/jdk/compare/34ee06f5...248f44dc

-

Changes: https://git.openjdk.org/jdk/pull/19989/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=19989&range=07
  Stats: 991 lines in 19 files changed: 754 ins; 116 del; 121 mod
  Patch: https://git.openjdk.org/jdk/pull/19989.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/19989/head:pull/19989

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

Re: RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v8]

[Andrew Haley]

On Thu, 25 Jul 2024 16:05:49 GMT, Andrew Haley  wrote:

>> This patch expands the use of a hash table for secondary superclasses
>> to the interpreter, C1, and runtime. It also adds a C2 implementation
>> of hashed lookup in cases where the superclass isn't known at compile
>> time.
>> 
>> HotSpot shared runtime
>> --
>> 
>> Building hashed secondary tables is now unconditional. It takes very
>> little time, and now that the shared runtime always has the tables, it
>> might as well take advantage of them. The shared code is easier to
>> follow now, I think.
>> 
>> There might be a performance issue with x86-64 in that we build
>> HotSpot for a default x86-64 target that does not support popcount.
>> This means that HotSpot C++ runtime on x86 always uses a software
>> emulation for popcount, even though the vast majority of machines made
>> for the past 20 years can do popcount in a single instruction. It
>> wouldn't be terribly hard to do something about that.
>> 
>> Having said that, the software popcount is really not bad.
>> 
>> x86
>> ---
>> 
>> x86 is rather tricky, because we still support
>> `-XX:-UseSecondarySupersTable` and `-XX:+UseSecondarySupersCache`, as
>> well as 32- and 64-bit ports. There's some further complication in
>> that only `RCX` can be used as a shift count, so there's some register
>> shuffling to do. All of this makes the logic in macroAssembler_x86.cpp
>> rather gnarly, with multiple levels of conditionals at compile time
>> and runtime.
>> 
>> AArch64
>> ---
>> 
>> AArch64 is considerably more straightforward. We always have a
>> popcount instruction and (thankfully) no 32-bit code to worry about.
>> 
>> Generally
>> -
>> 
>> I would dearly love simply to rip out the "old" secondary supers cache
>> support, but I've left it in just in case someone has a performance
>> regression.
>> 
>> The versions of `MacroAssembler::lookup_secondary_supers_table` that
>> work with variable superclasses don't take a fixed set of temp
>> registers, and neither do they call out to to a slow path subroutine.
>> Instead, the slow patch is expanded inline.
>> 
>> I don't think this is necessarily bad. Apart from the very rare cases
>> where C2 can't determine the superclass to search for at compile time,
>> this code is only used for generating stubs, and it seemed to me
>> ridiculous to have stubs calling other stubs.
>> 
>> I've followed the guidance from @iwanowww not to obsess too much about
>> the performance of C1-compiled secondary supers lookups, and to prefer
>> simplicity over absolute performance. Nonetheless, this i...
>
> Andrew Haley has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 40 commits:
> 
>  - Merge branch 'clean' into JDK-8331658-work
>  - Cleanup
>  - temp
>  - Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk into 
> JDK-8331658-work
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review feedback
>  - Review feedback
>  - ... and 30 more: https://git.openjdk.org/jdk/compare/34ee06f5...248f44dc

OK! I think that's everything.

Are we ready for a second pair of eyes now?

-

PR Comment: https://git.openjdk.org/jdk/pull/19989#issuecomment-2250880303

Re: RFR: 8337143: (fc, fs) Move filesystem-related native objects from libnio to libjava

[Brian Burkhalter]

On Wed, 24 Jul 2024 19:11:42 GMT, Brian Burkhalter  wrote:

> This proposed change would move the native objects required for NIO file 
> interaction from the libnio native library to the libjava native library on 
> Linux, macOS, and Windows.

Perhaps `/native/libjava/nio/ch` and 
`/native/libjava/nio/fs`?

-

PR Comment: https://git.openjdk.org/jdk/pull/20317#issuecomment-2250883875

Re: RFR: 8337060: Test java/foreign/TestConcurrentClose.java failed: IllegalStateException: SegmentAccessor::doAccess method not being compiled

[Jorn Vernee]

On Thu, 25 Jul 2024 10:58:42 GMT, Jorn Vernee  wrote:

> We are seeing some failures of this test in our CI, particularly on Windows 
> machines, which are known to starve out certain threads.
> 
> To make sure the compiler threads get a chance to work, I've added `-Xbatch` 
> to the flags with which the test runs (this was missed in the original PR). 
> I've also removed the timeout from the `awaitCompilation` method. The intent 
> is to instead rely on the overall test's timeout, which can also be adjusted 
> with a timeout factor for slower machines, rather than putting an absolute 
> time limit on the compilation.
> 
> Finally, Alan asked me to increase the timeout for the executor shutdown, 
> since there have also been some timeouts there. The current timeout is 
> borrowed from `test/jdk/java/foreign/TestHandshake`, but that test uses a lot 
> less threads, so it can probably get await with waiting just 20 seconds.
> 
> Testing: I ran this test 200 time in Oracle CI, half of the runs were with 
> `-Duse.JTREG_TEST_THREAD_FACTORY=Virtual -XX:-VerifyContinuations` (which is 
> one of the failing configurations).

Will integrate this now, since we're seeing sporadic failures in tier 1

-

PR Comment: https://git.openjdk.org/jdk/pull/20325#issuecomment-2250908883

Integrated: 8337060: Test java/foreign/TestConcurrentClose.java failed: IllegalStateException: SegmentAccessor::doAccess method not being compiled

[Jorn Vernee]

On Thu, 25 Jul 2024 10:58:42 GMT, Jorn Vernee  wrote:

> We are seeing some failures of this test in our CI, particularly on Windows 
> machines, which are known to starve out certain threads.
> 
> To make sure the compiler threads get a chance to work, I've added `-Xbatch` 
> to the flags with which the test runs (this was missed in the original PR). 
> I've also removed the timeout from the `awaitCompilation` method. The intent 
> is to instead rely on the overall test's timeout, which can also be adjusted 
> with a timeout factor for slower machines, rather than putting an absolute 
> time limit on the compilation.
> 
> Finally, Alan asked me to increase the timeout for the executor shutdown, 
> since there have also been some timeouts there. The current timeout is 
> borrowed from `test/jdk/java/foreign/TestHandshake`, but that test uses a lot 
> less threads, so it can probably get await with waiting just 20 seconds.
> 
> Testing: I ran this test 200 time in Oracle CI, half of the runs were with 
> `-Duse.JTREG_TEST_THREAD_FACTORY=Virtual -XX:-VerifyContinuations` (which is 
> one of the failing configurations).

This pull request has now been integrated.

Changeset: 81ed0287
Author:Jorn Vernee 
URL:   
https://git.openjdk.org/jdk/commit/81ed0287175617e6f7ad79dd7036b2fbde27e80a
Stats: 7 lines in 1 file changed: 1 ins; 5 del; 1 mod

8337060: Test java/foreign/TestConcurrentClose.java failed: 
IllegalStateException: SegmentAccessor::doAccess method not being compiled

Reviewed-by: mcimadamore, alanb

-

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

Re: RFR: 8336315: tools/jpackage/windows/WinChildProcessTest.java Failed: Check is calculator process is alive [v2]

[Alexander Matveev]

On Thu, 25 Jul 2024 08:40:44 GMT, Vanitha B P  wrote:

>> tools/jpackage/windows/WinChildProcessTest.java was failing intermittently, 
>> fixed the issue and changes are tested.
>
> Vanitha B P has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Addressed the review comments

Marked as reviewed by almatvee (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20312#pullrequestreview-2200371354

Re: RFR: 8337168: Optimize LocalDateTime.toString

[Naoto Sato]

On Thu, 25 Jul 2024 05:06:17 GMT, Shaojin Wen  wrote:

> The current LocalDateTime.toString implementation concatenates the toString 
> results of date and time, which can be passed to StringBuilder to reduce 
> additional object allocation and improve performance.

LGTM

src/java.base/share/classes/java/time/LocalTime.java line 1645:

> 1643: int nanoValue = nano;
> 1644: buf.append(hourValue < 10 ? "0" : "").append(hourValue)
> 1645: .append(minuteValue < 10 ? ":0" : 
> ":").append(minuteValue);

Nit: unnecessary modification

-

Marked as reviewed by naoto (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20321#pullrequestreview-2200429700
PR Review Comment: https://git.openjdk.org/jdk/pull/20321#discussion_r1692090342

Re: RFR: 8337168: Optimize LocalDateTime.toString

[Roger Riggs]

On Thu, 25 Jul 2024 05:06:17 GMT, Shaojin Wen  wrote:

> The current LocalDateTime.toString implementation concatenates the toString 
> results of date and time, which can be passed to StringBuilder to reduce 
> additional object allocation and improve performance.

src/java.base/share/classes/java/time/LocalDate.java line 2155:

> 2153: }
> 2154: 
> 2155: void formatTo(StringBuilder buf) {

For methods used outside of a class (i.e. not private) a minimal method 
description is recommended.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20321#discussion_r1692113590

Re: RFR: 8337167: StringSize deduplication [v2]

[Roger Riggs]

On Thu, 25 Jul 2024 14:29:05 GMT, Shaojin Wen  wrote:

>> Integer.stringSize and Long.stringSize are used not only in java.lang, but 
>> also in other places. We put stringSize in DecimalDigits to reduce duplicate 
>> code and the use of JLA.
>
> Shaojin Wen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   remove unused Integer.stringSize & Long.stringSize

lgtm

-

Marked as reviewed by rriggs (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/20322#pullrequestreview-2200503680

Re: RFR: 8336934: Clean up JavaLangReflectAccess

[Roger Riggs]

On Tue, 23 Jul 2024 04:10:38 GMT, Chen Liang  wrote:

> Removed redundant APIs in `JavaLangReflectAccess` and added general warning 
> against using `SharedSecrets`.

The description should include a summary of the changes and the rationale.
The Jira description is a bit better and could also be more complete.

-

PR Comment: https://git.openjdk.org/jdk/pull/20290#issuecomment-2251449428

Re: RFR: 8337168: Optimize LocalDateTime.toString [v2]

[Shaojin Wen]

> The current LocalDateTime.toString implementation concatenates the toString 
> results of date and time, which can be passed to StringBuilder to reduce 
> additional object allocation and improve performance.

Shaojin Wen has updated the pull request incrementally with two additional 
commits since the last revision:

 - add comments
 - reduce change

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20321/files
  - new: https://git.openjdk.org/jdk/pull/20321/files/c03ed26f..7b68b32b

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

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

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

Re: RFR: 8336934: Clean up JavaLangReflectAccess

[Roger Riggs]

On Tue, 23 Jul 2024 04:10:38 GMT, Chen Liang  wrote:

> Removed redundant APIs in `JavaLangReflectAccess` and added general warning 
> against using `SharedSecrets`.

src/java.base/share/classes/java/lang/reflect/Constructor.java line 168:

> 166: }
> 167: 
> 168: /** Creates a new root constructor with a custom accessor for 
> serialization hooks. */

Preserve "/**" comments for generated javadoc, a "//" comment would be more 
appropriate for a 1-liner.

src/java.base/share/classes/jdk/internal/access/JavaLangReflectAccess.java line 
53:

> 51: //
> 52: 
> 53: //

Personal editor comments should not be included.

src/java.base/share/classes/jdk/internal/access/JavaLangReflectAccess.java line 
68:

> 66: 
> 67: //
> 68: 

Remove

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20290#discussion_r1692174982
PR Review Comment: https://git.openjdk.org/jdk/pull/20290#discussion_r1692164266
PR Review Comment: https://git.openjdk.org/jdk/pull/20290#discussion_r1692165039

Re: RFR: 8336934: Clean up JavaLangReflectAccess

[Chen Liang]

On Thu, 25 Jul 2024 21:38:04 GMT, Roger Riggs  wrote:

>> Removed redundant APIs in `JavaLangReflectAccess` and added general warning 
>> against using `SharedSecrets`.
>
> src/java.base/share/classes/jdk/internal/access/JavaLangReflectAccess.java 
> line 53:
> 
>> 51: //
>> 52: 
>> 53: //
> 
> Personal editor comments should not be included.

Ah, thought editor folds were acceptable as they appear in a few places in 
javac code. Will remove.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20290#discussion_r1692193282

Re: RFR: 8337168: Optimize LocalDateTime.toString [v2]

[Chen Liang]

On Thu, 25 Jul 2024 21:52:03 GMT, Shaojin Wen  wrote:

>> The current LocalDateTime.toString implementation concatenates the toString 
>> results of date and time, which can be passed to StringBuilder to reduce 
>> additional object allocation and improve performance.
>
> Shaojin Wen has updated the pull request incrementally with two additional 
> commits since the last revision:
> 
>  - add comments
>  - reduce change

src/java.base/share/classes/java/time/LocalDate.java line 2157:

> 2155: 
> 2156: /**
> 2157:  *  Outputs this date into {@code StringBuilder}, such as {@code 
> 2007-12-03}.

You shouldn't copy the docs for `toString` as that is not we should be careful 
about using this `formatTo`. You should write something helpful like:

/**
 * Prints the toString result to the given buf, avoiding extra string 
allocations.
 * Requires extra capacity of 10 to avoid StringBuilder reallocation.
 */

src/java.base/share/classes/java/time/LocalTime.java line 1640:

> 1638: 
> 1639: /**
> 1640:  * Outputs this time into {@code StringBuilder}, such as {@code 
> 10:15}.

Same here:

/**
 * Prints the toString result to the given buf, avoiding extra string 
allocations.
 * Requires extra capacity of 18 to avoid StringBuilder reallocation.
 */

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20321#discussion_r1692207893
PR Review Comment: https://git.openjdk.org/jdk/pull/20321#discussion_r1692209966

RFR: 8313205: Modernize java.text.Format with StringBuilder

[Justin Lu]

Please review this PR which adds public StringBuilder overloads to the 
formatting methods of java.text.Format and implementing subclasses.

While Format, NumberFormat, and DateFormat are abstract, these new methods are 
not added as abstract to prevent compatibility concerns. Instead they are added 
as non-abstract methods, with a default implementation that throws UOE and a 
recommendation that subclasses override and provide their own implementations. 
These new methods use the same specification as the StringBuffer ones, with 
exception of `MessageFormat.format(format(Object[] arguments, StringBuilder 
result, FieldPosition pos)` which omits the table, and instead links to it.

The JDK implementing Format classes, (such as DecimalFormat) leverage the 
StringBuf proxy methods.

A corresponding CSR has been drafted: 
https://bugs.openjdk.org/browse/JDK-8337141, which goes into detail on 
motivation/history. (Holding off on uploading the specification to CSR until 
wording finalized).

-

Commit messages:
 - init

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

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

Re: RFR: 8336934: Clean up JavaLangReflectAccess [v2]

[Chen Liang]

> Removed redundant APIs in `JavaLangReflectAccess` and added general warning 
> against using `SharedSecrets`.
> 
> The cleanup in `JavaLangReflectAccess` is:
> - Converted `newConstructor` taking parameters to taking another constructor 
> and a new accessor. The old existing API essentially does a copy and sets an 
> accessor.
> - Removed lots of constructor getters, unused after `newConstructor` cleanup.
> - Removed accessor getter/setters, unused after `newConstructor` cleanup.
> - Removed `leafCopyMethod`. We can already use `getRoot` plus `copyMethod` to 
> achieve the same effect.

Chen Liang has updated the pull request with a new target base due to a merge 
or a rebase. The incremental webrev excludes the unrelated changes brought in 
by the merge/rebase. The pull request contains three additional commits since 
the last revision:

 - Remove editor folds, improve comments
 - Merge branch 'm5' into cleanup/reflect-access
 - 8336934: Clean up JavaLangReflectAccess

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20290/files
  - new: https://git.openjdk.org/jdk/pull/20290/files/5d6c60bc..7973c8ff

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

  Stats: 8152 lines in 178 files changed: 5432 ins; 1729 del; 991 mod
  Patch: https://git.openjdk.org/jdk/pull/20290.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20290/head:pull/20290

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

Re: RFR: 8336934: Clean up JavaLangReflectAccess

[Chen Liang]

On Thu, 25 Jul 2024 21:45:21 GMT, Roger Riggs  wrote:

>> Removed redundant APIs in `JavaLangReflectAccess` and added general warning 
>> against using `SharedSecrets`.
>> 
>> The cleanup in `JavaLangReflectAccess` is:
>> - Converted `newConstructor` taking parameters to taking another constructor 
>> and a new accessor. The old existing API essentially does a copy and sets an 
>> accessor.
>> - Removed lots of constructor getters, unused after `newConstructor` cleanup.
>> - Removed accessor getter/setters, unused after `newConstructor` cleanup.
>> - Removed `leafCopyMethod`. We can already use `getRoot` plus `copyMethod` 
>> to achieve the same effect.
>
> The description should include a summary of the changes and the rationale.
> The Jira description is a bit better and could also be more complete.

@RogerRiggs Is the description now cleaner?

-

PR Comment: https://git.openjdk.org/jdk/pull/20290#issuecomment-2251513818

RFR: 8337225: Demote maxStack and maxLocals from CodeModel to CodeAttribute

[Chen Liang]

As discussed in offline meeting, the max stack and locals information are part 
of the code attribute and not meaningful for buffered code elements. 
Computation would be costly and these see no real usage during transformations. 
Thus, the proposed solution is to move these APIs to be CodeAttribute specific, 
as this is already how all these APIs' users are using.

Also removed useless `Writable` on buffered models, and fixed 
`BufferedMethodBuilder::code` implementation.

-

Commit messages:
 - 8337225: Demote maxStack and maxLocals from CodeModel to CodeAttribute

Changes: https://git.openjdk.org/jdk/pull/20338/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20338&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8337225
  Stats: 106 lines in 11 files changed: 32 ins; 54 del; 20 mod
  Patch: https://git.openjdk.org/jdk/pull/20338.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20338/head:pull/20338

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

Re: RFR: 8337168: Optimize LocalDateTime.toString [v3]

[Shaojin Wen]

> The current LocalDateTime.toString implementation concatenates the toString 
> results of date and time, which can be passed to StringBuilder to reduce 
> additional object allocation and improve performance.

Shaojin Wen has updated the pull request incrementally with one additional 
commit since the last revision:

  change description

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20321/files
  - new: https://git.openjdk.org/jdk/pull/20321/files/7b68b32b..e7c5bc69

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

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

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

Re: RFR: 8337168: Optimize LocalDateTime.toString [v3]

[Chen Liang]

On Thu, 25 Jul 2024 22:51:09 GMT, Shaojin Wen  wrote:

>> The current LocalDateTime.toString implementation concatenates the toString 
>> results of date and time, which can be passed to StringBuilder to reduce 
>> additional object allocation and improve performance.
>
> Shaojin Wen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   change description

src/java.base/share/classes/java/time/LocalTime.java line 1641:

> 1639: /**
> 1640:  * Prints the toString result to the given buf, avoiding extra 
> string allocations.
> 1641:  * Requires extra capacity of 10 to avoid StringBuilder 
> reallocation.

Suggestion:

 * Requires extra capacity of 18 to avoid StringBuilder reallocation.

Sorry I left a typo, you see toString is `new StringBuilder(18)`

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20321#discussion_r1692240103

Re: RFR: 8337168: Optimize LocalDateTime.toString [v4]

[Shaojin Wen]

> The current LocalDateTime.toString implementation concatenates the toString 
> results of date and time, which can be passed to StringBuilder to reduce 
> additional object allocation and improve performance.

Shaojin Wen has updated the pull request incrementally with one additional 
commit since the last revision:

  Update src/java.base/share/classes/java/time/LocalTime.java
  
  Co-authored-by: Chen Liang 

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20321/files
  - new: https://git.openjdk.org/jdk/pull/20321/files/e7c5bc69..f17ea04a

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

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

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

Re: RFR: 8337168: Optimize LocalDateTime.toString [v4]

[Chen Liang]

On Thu, 25 Jul 2024 23:05:46 GMT, Shaojin Wen  wrote:

>> The current LocalDateTime.toString implementation concatenates the toString 
>> results of date and time, which can be passed to StringBuilder to reduce 
>> additional object allocation and improve performance.
>
> Shaojin Wen has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Update src/java.base/share/classes/java/time/LocalTime.java
>   
>   Co-authored-by: Chen Liang 

Marked as reviewed by liach (Reviewer).

-

PR Review: https://git.openjdk.org/jdk/pull/20321#pullrequestreview-2200599732

RFR: 8337219: AccessFlags factories do not require necessary arguments

[Chen Liang]

Removes 6 `AccessFlags` factories that do not take class-file versions as its 
arguments.

`AccessFlags` is a wrapper around a bit mask to support modifier streaming in 
ClassFile API. It additionally supports advanced validation based on location.

However, as class file versions evolve, we may also need a class file version 
argument to ensure that the flags are correctly constructed. For example, a 
pre-valhalla class modifier without `ACC_SUPER` should not be interpreted as a 
value class. The current factories cannot find good default class file 
versions, and if they always assume latest, they will fail in this scenario.

As a result, we should remove these 6 factories; note that users can still set 
the flags via `XxxBuilder::withFlags` with either `int` or `AccessFlag...` 
flags. In contrast, these builder APIs can fetch the previously-passed class 
file versions, and correctly validate or interpret these flags. Same story goes 
for parsing, which can also construct the right flags with available 
information.

This enables us to add methods to interpret the logical flags with 
version-specific information. If there's need, we can always add a new 
`AccessFlags.of(int, AccessFlag.Location, ClassFileFormatVersion)` factory, 
given the flexibility from this removal.

-

Commit messages:
 - 8337219: AccessFlags factories do not require necessary arguments

Changes: https://git.openjdk.org/jdk/pull/20341/files
  Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=20341&range=00
  Issue: https://bugs.openjdk.org/browse/JDK-8337219
  Stats: 212 lines in 21 files changed: 71 ins; 80 del; 61 mod
  Patch: https://git.openjdk.org/jdk/pull/20341.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/20341/head:pull/20341

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

Re: RFR: 8313205: Modernize java.text.Format with StringBuilder

[Chen Liang]

On Thu, 25 Jul 2024 22:18:03 GMT, Justin Lu  wrote:

> Please review this PR which adds public StringBuilder overloads to the 
> formatting methods of java.text.Format and implementing subclasses.
> 
> While Format, NumberFormat, and DateFormat are abstract, these new methods 
> are not added as abstract to prevent compatibility concerns. Instead they are 
> added as non-abstract methods, with a default implementation that throws UOE 
> and a recommendation that subclasses override and provide their own 
> implementations. These new methods use the same specification as the 
> StringBuffer ones, with exception of `MessageFormat.format(format(Object[] 
> arguments, StringBuilder result, FieldPosition pos)` which omits the table, 
> and instead links to it.
> 
> The JDK implementing Format classes, (such as DecimalFormat) leverage the 
> StringBuf proxy methods.
> 
> A corresponding CSR has been drafted: 
> https://bugs.openjdk.org/browse/JDK-8337141, which goes into detail on 
> motivation/history. (Holding off on uploading the specification to CSR until 
> wording finalized).

You should add `@since` tags on all these new API additions.

-

PR Comment: https://git.openjdk.org/jdk/pull/20337#issuecomment-2251565706

Re: RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v8]

[Vladimir Ivanov]

On Thu, 25 Jul 2024 16:05:49 GMT, Andrew Haley  wrote:

>> This patch expands the use of a hash table for secondary superclasses
>> to the interpreter, C1, and runtime. It also adds a C2 implementation
>> of hashed lookup in cases where the superclass isn't known at compile
>> time.
>> 
>> HotSpot shared runtime
>> --
>> 
>> Building hashed secondary tables is now unconditional. It takes very
>> little time, and now that the shared runtime always has the tables, it
>> might as well take advantage of them. The shared code is easier to
>> follow now, I think.
>> 
>> There might be a performance issue with x86-64 in that we build
>> HotSpot for a default x86-64 target that does not support popcount.
>> This means that HotSpot C++ runtime on x86 always uses a software
>> emulation for popcount, even though the vast majority of machines made
>> for the past 20 years can do popcount in a single instruction. It
>> wouldn't be terribly hard to do something about that.
>> 
>> Having said that, the software popcount is really not bad.
>> 
>> x86
>> ---
>> 
>> x86 is rather tricky, because we still support
>> `-XX:-UseSecondarySupersTable` and `-XX:+UseSecondarySupersCache`, as
>> well as 32- and 64-bit ports. There's some further complication in
>> that only `RCX` can be used as a shift count, so there's some register
>> shuffling to do. All of this makes the logic in macroAssembler_x86.cpp
>> rather gnarly, with multiple levels of conditionals at compile time
>> and runtime.
>> 
>> AArch64
>> ---
>> 
>> AArch64 is considerably more straightforward. We always have a
>> popcount instruction and (thankfully) no 32-bit code to worry about.
>> 
>> Generally
>> -
>> 
>> I would dearly love simply to rip out the "old" secondary supers cache
>> support, but I've left it in just in case someone has a performance
>> regression.
>> 
>> The versions of `MacroAssembler::lookup_secondary_supers_table` that
>> work with variable superclasses don't take a fixed set of temp
>> registers, and neither do they call out to to a slow path subroutine.
>> Instead, the slow patch is expanded inline.
>> 
>> I don't think this is necessarily bad. Apart from the very rare cases
>> where C2 can't determine the superclass to search for at compile time,
>> this code is only used for generating stubs, and it seemed to me
>> ridiculous to have stubs calling other stubs.
>> 
>> I've followed the guidance from @iwanowww not to obsess too much about
>> the performance of C1-compiled secondary supers lookups, and to prefer
>> simplicity over absolute performance. Nonetheless, this i...
>
> Andrew Haley has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 40 commits:
> 
>  - Merge branch 'clean' into JDK-8331658-work
>  - Cleanup
>  - temp
>  - Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk into 
> JDK-8331658-work
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review feedback
>  - Review feedback
>  - ... and 30 more: https://git.openjdk.org/jdk/compare/34ee06f5...248f44dc

Thanks! The patch looks good, except there was one failure observed during 
testing with the latest patch [1]. It does look related to the latest changes 
you did in 
[54050a5](https://github.com/openjdk/jdk/pull/19989/commits/54050a5c2c0aa1d6a9e36d0240c66345765845e3)
 about `bitmap == SECONDARY_SUPERS_BITMAP_FULL` check. 

[1]

#  Internal Error (.../src/hotspot/share/oops/array.hpp:126), pid=1225147, 
tid=1273512
#  assert(i >= 0 && i< _length) failed: oob: 0 <= 63 < 63

V  [libjvm.so+0x7f7eab]  oopDesc::is_a(Klass*) const+0x21b  (array.hpp:126)
V  [libjvm.so+0xe9805e]  java_lang_Throwable::fill_in_stack_trace(Handle, 
methodHandle const&, JavaThread*)+0x158e  (javaClasses.cpp:2677)
V  [libjvm.so+0xe982cb]  java_lang_Throwable::fill_in_stack_trace(Handle, 
methodHandle const&)+0x6b  (javaClasses.cpp:2719)
V  [libjvm.so+0xfe045c]  JVM_FillInStackTrace+0x9c  (jvm.cpp:515)


RBX=0xb446bcf0 is a pointer to class: 
javasoft.sqe.tests.lang.clss029.clss02902.e67 {0xb446bcf0}
...
 - name:  'javasoft/sqe/tests/lang/clss029/clss02902/e67'
 - super: 'javasoft/sqe/tests/lang/clss029/clss02902/e66'
 - sub:   'javasoft/sqe/tests/lang/clss029/clss02902/e68'   
...
 - secondary supers: Array(0x7faff68b3058)
 - hash_slot: 39
 - secondary bitmap: 0x


R12=0xb446ba80 is a pointer to class: 
javasoft.sqe.tests.lang.clss029.clss02902.e66 {0xb446ba80}
...
 - name:  'javasoft/sqe/tests/lang/clss029/clss02902/e66'
 - super: 'javasoft/sqe/tests/lang/clss029/clss02902/e65'
 - sub:   'javasoft/sqe/tests/lang/clss029/clss02902/e67'   
...
 - secondary supers: Array(0x7faff68b2c90)
 - hash_slot: 63
 - secondary bitmap: 0xfffefffd


Do we miss

-

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

Re: RFR: 8336856: Optimize String Concat [v15]

[Shaojin Wen]

On Thu, 25 Jul 2024 10:15:20 GMT, Claes Redestad  wrote:

>> Shaojin Wen has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   add benchmark
>
> test/micro/org/openjdk/bench/java/lang/StringConcatGenerate.java line 47:
> 
>> 45: @Measurement(iterations = 5, time = 1000, timeUnit = 
>> TimeUnit.MILLISECONDS)
>> 46: @Fork(value = 3, jvmArgsAppend = 
>> "-Djava.lang.invoke.StringConcat.highArityThreshold=0")
>> 47: public class StringConcatGenerate extends StringConcat {
> 
> Adding a subclass with an overridden `@Fork` to pass a different 
> `jvmArgsAppend` is a reasonable trick, but could be moved to a nested class 
> within `StringConcat` to keep it in the same context. I'm not sure if this 
> micro brings a persistent added value, though: for experimentation we can 
> just run `StringConcat` with different 
> `-Djava.lang.invoke.StringConcat.highArityThreshold` settings, while for 
> continuous regression testing we're more interested in validating the default 
> settings. Supplying the new `main` method doesn't add anything here, either, 
> since a standalone execution wouldn't pick up the `jvmArgsAppend` value.

I have removed the newly added micro benchmark. But I don't know how to add 
jvmArgs in make test.

This will give an error

make test TEST="micro:java.lang.StringConcat.concat6String" 
-Djava.lang.invoke.StringConcat.highArityThreshold=0

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20273#discussion_r1692278005

Re: RFR: 8313205: Modernize java.text.Format with StringBuilder [v2]

[Justin Lu]

> Please review this PR which adds public StringBuilder overloads to the 
> formatting methods of java.text.Format and implementing subclasses.
> 
> While Format, NumberFormat, and DateFormat are abstract, these new methods 
> are not added as abstract to prevent compatibility concerns. Instead they are 
> added as non-abstract methods, with a default implementation that throws UOE 
> and a recommendation that subclasses override and provide their own 
> implementations. These new methods use the same specification as the 
> StringBuffer ones, with exception of `MessageFormat.format(format(Object[] 
> arguments, StringBuilder result, FieldPosition pos)` which omits the table, 
> and instead links to it.
> 
> The JDK implementing Format classes, (such as DecimalFormat) leverage the 
> StringBuf proxy methods.
> 
> A corresponding CSR has been drafted: 
> https://bugs.openjdk.org/browse/JDK-8337141, which goes into detail on 
> motivation/history. (Holding off on uploading the specification to CSR until 
> wording finalized).

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

  add since tags

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/20337/files
  - new: https://git.openjdk.org/jdk/pull/20337/files/3c78d9ac..314b8d7b

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

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

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

Re: RFR: 8313205: Modernize java.text.Format with StringBuilder

[Justin Lu]

On Thu, 25 Jul 2024 23:28:44 GMT, Chen Liang  wrote:

> You should add `@since` tags on all these new API additions.

Thanks for the reminder. Updated.

-

PR Comment: https://git.openjdk.org/jdk/pull/20337#issuecomment-2251577718

Re: RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v8]

[Vladimir Ivanov]

On Thu, 25 Jul 2024 16:05:49 GMT, Andrew Haley  wrote:

>> This patch expands the use of a hash table for secondary superclasses
>> to the interpreter, C1, and runtime. It also adds a C2 implementation
>> of hashed lookup in cases where the superclass isn't known at compile
>> time.
>> 
>> HotSpot shared runtime
>> --
>> 
>> Building hashed secondary tables is now unconditional. It takes very
>> little time, and now that the shared runtime always has the tables, it
>> might as well take advantage of them. The shared code is easier to
>> follow now, I think.
>> 
>> There might be a performance issue with x86-64 in that we build
>> HotSpot for a default x86-64 target that does not support popcount.
>> This means that HotSpot C++ runtime on x86 always uses a software
>> emulation for popcount, even though the vast majority of machines made
>> for the past 20 years can do popcount in a single instruction. It
>> wouldn't be terribly hard to do something about that.
>> 
>> Having said that, the software popcount is really not bad.
>> 
>> x86
>> ---
>> 
>> x86 is rather tricky, because we still support
>> `-XX:-UseSecondarySupersTable` and `-XX:+UseSecondarySupersCache`, as
>> well as 32- and 64-bit ports. There's some further complication in
>> that only `RCX` can be used as a shift count, so there's some register
>> shuffling to do. All of this makes the logic in macroAssembler_x86.cpp
>> rather gnarly, with multiple levels of conditionals at compile time
>> and runtime.
>> 
>> AArch64
>> ---
>> 
>> AArch64 is considerably more straightforward. We always have a
>> popcount instruction and (thankfully) no 32-bit code to worry about.
>> 
>> Generally
>> -
>> 
>> I would dearly love simply to rip out the "old" secondary supers cache
>> support, but I've left it in just in case someone has a performance
>> regression.
>> 
>> The versions of `MacroAssembler::lookup_secondary_supers_table` that
>> work with variable superclasses don't take a fixed set of temp
>> registers, and neither do they call out to to a slow path subroutine.
>> Instead, the slow patch is expanded inline.
>> 
>> I don't think this is necessarily bad. Apart from the very rare cases
>> where C2 can't determine the superclass to search for at compile time,
>> this code is only used for generating stubs, and it seemed to me
>> ridiculous to have stubs calling other stubs.
>> 
>> I've followed the guidance from @iwanowww not to obsess too much about
>> the performance of C1-compiled secondary supers lookups, and to prefer
>> simplicity over absolute performance. Nonetheless, this i...
>
> Andrew Haley has updated the pull request with a new target base due to a 
> merge or a rebase. The pull request now contains 40 commits:
> 
>  - Merge branch 'clean' into JDK-8331658-work
>  - Cleanup
>  - temp
>  - Merge branch 'JDK-8331658-work' of https://github.com/theRealAph/jdk into 
> JDK-8331658-work
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review comments
>  - Review feedback
>  - Review feedback
>  - ... and 30 more: https://git.openjdk.org/jdk/compare/34ee06f5...248f44dc

src/hotspot/share/oops/klass.inline.hpp line 82:

> 80: // subtype check: true if is_subclass_of, or if k is interface and 
> receiver implements it
> 81: inline bool Klass::is_subtype_of(Klass* k) const {
> 82:   guarantee(secondary_supers() != nullptr, "must be");

Minor point: considering libjvm contains hundreds of copies, does it make sense 
to turn it into an assert instead? For example, on AArch64 the check costs 36 
bytes [1] in product build.  

[1]

libjvm.dylib[0x1306d4] <+28>:  ldrx9, [x8, #0x28]
libjvm.dylib[0x1306d8] <+32>:  cbzx9, 0x1307dc  ; <+292> 
[inlined] Klass::is_subtype_of(Klass*) const at klass.inline.hpp:82:3
...
libjvm.dylib[0x1307dc] <+292>: adrp   x0, 3200
libjvm.dylib[0x1307e0] <+296>: addx0, x0, #0x663; 
"open/src/hotspot/share/oops/klass.inline.hpp"
libjvm.dylib[0x1307e4] <+300>: adrp   x2, 3200
libjvm.dylib[0x1307e8] <+304>: addx2, x2, #0x690; 
"guarantee(secondary_supers() != nullptr) failed"
libjvm.dylib[0x1307ec] <+308>: adrp   x3, 3200
libjvm.dylib[0x1307f0] <+312>: addx3, x3, #0x6c0; "must be"
libjvm.dylib[0x1307f4] <+316>: movw1, #0x52
libjvm.dylib[0x1307f8] <+320>: bl 0x54f870  ; 
report_vm_error at debug.cpp:181

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1692288943

RFR: 8337245: Fix wrong comment of StringConcatHelper

[Shaojin Wen]

8337245: Fix wrong comment of StringConcatHelper

-

Commit messages:
 - fix typo

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

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

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[Jaikiran Pai]

On Thu, 25 Jul 2024 09:50:10 GMT, SendaoYan  wrote:

>> Hi all,
>> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails 
>> on rpm build mock environment. The `df -h` command return fail `df: cannot 
>> read table of mounted file systems: No such file or directory` on the rpm 
>> build mock environment also. I think it's a environmental issue, and the 
>> environmental issue should not cause the test fails, it should skip the test.
>> 
>> The rpmbuild mock enviroment is like a sandbox, which created by `chroot` 
>> shell command, in the rpmbuild mock enviroment, `df -h` report `cannot read 
>> table of mounted file systems`, and java Files.getFileStore also throw 
>> `IOException`. We want to build and test the jdk in this `sandbox`, and the 
>> default jtreg work directory is `JTWork` in current directory, so this 
>> testcase will report fails.
>> 
>> Only change the testcase, the change has been verified locally, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   1. Just catch the IOException here when getting the FileStore and skip the 
> test instead of checking for specific exception messages. 2. Throw a 
> org.testng.SkipException instead print and return

test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java line 217:

> 215: } catch (IOException e) {
> 216: e.printStackTrace();
> 217: throw new SkipException("WARNING: IOException occur. 
> Skipping testDumpDirNotWritable test.");

Nit: "occurred" instead "occur". Additionally, I would suggest even the 
exception's toString() in that message just to provide additional context at 
the location wherever this will get reported outside of a .jtr (if at all). So 
something like:

throw new SkipException("WARNING: IOException occurred: " + e + ", Skipping 
testDumpDirNotWritable test.");

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19905#discussion_r1692498834

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[Jaikiran Pai]

On Thu, 25 Jul 2024 09:50:10 GMT, SendaoYan  wrote:

>> Hi all,
>> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails 
>> on rpm build mock environment. The `df -h` command return fail `df: cannot 
>> read table of mounted file systems: No such file or directory` on the rpm 
>> build mock environment also. I think it's a environmental issue, and the 
>> environmental issue should not cause the test fails, it should skip the test.
>> 
>> The rpmbuild mock enviroment is like a sandbox, which created by `chroot` 
>> shell command, in the rpmbuild mock enviroment, `df -h` report `cannot read 
>> table of mounted file systems`, and java Files.getFileStore also throw 
>> `IOException`. We want to build and test the jdk in this `sandbox`, and the 
>> default jtreg work directory is `JTWork` in current directory, so this 
>> testcase will report fails.
>> 
>> Only change the testcase, the change has been verified locally, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   1. Just catch the IOException here when getting the FileStore and skip the 
> test instead of checking for specific exception messages. 2. Throw a 
> org.testng.SkipException instead print and return

> Additional, the org.testng.SkipException seems do not work normally in 
> [jtreg](https://bugs.openjdk.org/browse/CODETOOLS-7903708) for now.

We have had several enhancement reports (in jtreg) for better reporting skipped 
tests. Some relate to what the make scripts in the JDK report and some from 
jtreg reports themselves. I'll have to go back and check if the one you note 
about is something that we are already aware about. For now though, your change 
to use `SkippedException` looks correct to me.

-

PR Comment: https://git.openjdk.org/jdk/pull/19905#issuecomment-2251959084

Re: RFR: 8336895: BufferedReader doesn't read full \r\n line ending when it doesn't fit in buffer

[Jaikiran Pai]

On Thu, 25 Jul 2024 00:24:59 GMT, Brian Burkhalter  wrote:

> Add some verbiage stating that two buffered readers or input streams should 
> not be used to read from the same reader or input stream, respectively.

src/java.base/share/classes/java/io/BufferedInputStream.java line 56:

> 54:  * to read from the same {@code InputStream}. There is no way for a
> 55:  * {@code BufferedInputStream} to know what another {@code 
> BufferedInputStream}
> 56:  * has read from the underlying {@code InputStream}, nor dues it have 
> access

Hello Brian, there's a typo here - should have been "does" instead of "dues". 
Same in the other class in this PR.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/20320#discussion_r1692503702

Re: RFR: 8336895: BufferedReader doesn't read full \r\n line ending when it doesn't fit in buffer

[Jaikiran Pai]

On Thu, 25 Jul 2024 00:24:59 GMT, Brian Burkhalter  wrote:

> Add some verbiage stating that two buffered readers or input streams should 
> not be used to read from the same reader or input stream, respectively.

I went through the linked JBS issue to understand why we are adding this. The 
use case appears to be that the application uses a BufferedReader instance on 
top of the underlying Reader R1, then does some API calls on the 
BufferedReader, then uses a separate and new BufferedReader instance on top of 
the same underlying R1 Reader instance.
I think the proposal to add this text is OK, although I wonder if this should 
also be added or is applicable to `BufferedOutputStream` too. Also, do you 
think we could reword the proposed text a bit? It wasn't immediately clear to 
me what issue we were trying to address with that text. Would something like 
the following be clearer:
> More than one instance of a BufferedReader should not be used with the same 
> instance of the underlying Reader. Doing so can cause the BufferedReader 
> instances to return an incorrect result since each instance of the 
> BufferedReader maintains its own state.

-

PR Comment: https://git.openjdk.org/jdk/pull/20320#issuecomment-2251988591

Re: RFR: 8331341: secondary_super_cache does not scale well: C1 and interpreter [v8]

[Vladimir Ivanov]

On Thu, 25 Jul 2024 13:56:34 GMT, Andrew Haley  wrote:

>> Thanks, now I see that `Class::isInstance(Object)` is backed by 
>> `Runtime1::is_instance_of()` which uses `oopDesc::is_a()` to do the job.
>> 
>> If it turns out to be performance critical, the intrinsic implementation 
>> should be rewritten to exercise existing subtype checking support in C1. As 
>> it is implemented now, it's already quite inefficient.
>
> I did write an intrinsic for that, but it made this patch even larger. I have 
> a small patch for C1, for some other time.

FYI I filed a low-priority RFE against C1 to track it 
([JDK-8337251](https://bugs.openjdk.org/browse/JDK-8337251)).

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19989#discussion_r1692548251

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v4]

[SendaoYan]

> Hi all,
> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails on 
> rpm build mock environment. The `df -h` command return fail `df: cannot read 
> table of mounted file systems: No such file or directory` on the rpm build 
> mock environment also. I think it's a environmental issue, and the 
> environmental issue should not cause the test fails, it should skip the test.
> 
> The rpmbuild mock enviroment is like a sandbox, which created by `chroot` 
> shell command, in the rpmbuild mock enviroment, `df -h` report `cannot read 
> table of mounted file systems`, and java Files.getFileStore also throw 
> `IOException`. We want to build and test the jdk in this `sandbox`, and the 
> default jtreg work directory is `JTWork` in current directory, so this 
> testcase will report fails.
> 
> Only change the testcase, the change has been verified locally, no risk.

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

  add the exception's toString() into SkipException

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/19905/files
  - new: https://git.openjdk.org/jdk/pull/19905/files/9c7946f8..b3af3f87

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

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

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

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v3]

[SendaoYan]

On Fri, 26 Jul 2024 04:30:16 GMT, Jaikiran Pai  wrote:

>> SendaoYan has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   1. Just catch the IOException here when getting the FileStore and skip the 
>> test instead of checking for specific exception messages. 2. Throw a 
>> org.testng.SkipException instead print and return
>
> test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java line 217:
> 
>> 215: } catch (IOException e) {
>> 216: e.printStackTrace();
>> 217: throw new SkipException("WARNING: IOException occur. 
>> Skipping testDumpDirNotWritable test.");
> 
> Nit: "occurred" instead "occur". Additionally, I would suggest even the 
> exception's toString() in that message just to provide additional context at 
> the location wherever this will get reported outside of a .jtr (if at all). 
> So something like:
> 
> throw new SkipException("WARNING: IOException occurred: " + e + ", Skipping 
> testDumpDirNotWritable test.");

Thanks for your advice and review again.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/19905#discussion_r1692570083

Re: RFR: 8335150: Test LogGeneratedClassesTest.java fails on rpmbuild mock enviroment [v4]

[Jaikiran Pai]

On Fri, 26 Jul 2024 06:29:05 GMT, SendaoYan  wrote:

>> Hi all,
>> Test `test/jdk/java/lang/invoke/lambda/LogGeneratedClassesTest.java` fails 
>> on rpm build mock environment. The `df -h` command return fail `df: cannot 
>> read table of mounted file systems: No such file or directory` on the rpm 
>> build mock environment also. I think it's a environmental issue, and the 
>> environmental issue should not cause the test fails, it should skip the test.
>> 
>> The rpmbuild mock enviroment is like a sandbox, which created by `chroot` 
>> shell command, in the rpmbuild mock enviroment, `df -h` report `cannot read 
>> table of mounted file systems`, and java Files.getFileStore also throw 
>> `IOException`. We want to build and test the jdk in this `sandbox`, and the 
>> default jtreg work directory is `JTWork` in current directory, so this 
>> testcase will report fails.
>> 
>> Only change the testcase, the change has been verified locally, no risk.
>
> SendaoYan has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   add the exception's toString() into SkipException

The latest change looks OK to me. Please wait for Alan to decide if this is OK 
to integrate.

-

Marked as reviewed by jpai (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19905#pullrequestreview-2201036393