Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v3]
> The `find` method now is
> ```C++
> template
> int find(T* token, bool f(T*, E)) const {
> ...
>
> Any other functions which use this are also changed.
> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
> Windows passed.
Afshin Zafari has updated the pull request incrementally with one additional
commit since the last revision:
find_from_end and its caller are also updated.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/15418/files
- new: https://git.openjdk.org/jdk/pull/15418/files/f46cba87..266f6feb
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=15418&range=02
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=15418&range=01-02
Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod
Patch: https://git.openjdk.org/jdk/pull/15418.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15418/head:pull/15418
PR: https://git.openjdk.org/jdk/pull/15418
Re: RFR: 8314940: Use of NIO in JDKs Metrics implementation causes issues in GraalVM
On Fri, 25 Aug 2023 10:04:28 GMT, Alan Bateman wrote: >>> Something fishy here, is this working around a bug in GraaVM native image >>> or why does this change need to be in JDK? >> >> I've now realized that the bug had an incorrect statement in the >> description. The cycle happens due to the `Runtime.getRuntime().maxMemory()` >> implementation in GraalVM to use JDK `Metrics`, since the `ByteBuffer` [code >> relies on the `Runtime.getRuntime().maxMemory()` >> API](https://github.com/openjdk/jdk/blob/76b9011c9ecb8c0c713a58d034f281ba70d65d4e/src/java.base/share/classes/jdk/internal/misc/VM.java#L261). >> The GraalVM impl to use the JDK Metrics seems a reasonable thing to do, no? >> >> With that said, it's seems a rather uncontroversial change with very limited >> scope. Do you see anything problematic in this patch? Happy to revise if you >> think there are some no-no's :) > >> I've now realized that the bug had an incorrect statement in the >> description. The cycle happens due to the `Runtime.getRuntime().maxMemory()` >> implementation in GraalVM to use JDK `Metrics`, since the `ByteBuffer` [code >> relies on the `Runtime.getRuntime().maxMemory()` >> API](https://github.com/openjdk/jdk/blob/76b9011c9ecb8c0c713a58d034f281ba70d65d4e/src/java.base/share/classes/jdk/internal/misc/VM.java#L261). >> The GraalVM impl to use the JDK Metrics seems a reasonable thing to do, no? >> >> With that said, it's seems a rather uncontroversial change with very limited >> scope. Do you see anything problematic in this patch? Happy to revise if you >> think there are some no-no's :) > > Recursive initialization issues can tricky and often it comes down to > deciding where to break the cycle. In this case, it seems a bit fragile to do > it in CgroupUtil as it should be allowed to use any of the file system APIs > to access cgroups or proc files. Part of me wonders if this would be better > handled in their implementation of jdk.internal.misc.VM or Runtime.maxMemory > but maybe you've been there already? @AlanBateman Is there anything else you need me to do? If so, please let me know. Thanks! - PR Comment: https://git.openjdk.org/jdk/pull/15416#issuecomment-1695824275
Re: RFR: 8314940: Use of NIO in JDKs Metrics implementation causes issues in GraalVM
On Thu, 24 Aug 2023 13:16:16 GMT, Severin Gehwolf wrote: > Please review this rather trivial fix to not use `nio` in `CgroupUtil`, part > of the > JDK's Metrics API. The primary motivating factor is that it allows one to use > the > JDK's version of `Metrics` in GraalVM. See the bug for details as to why this > is > needed. > > Testing: > - [x] GraalVM builds with/without the fix and the reproducer (fails > before/works after) > - [x] `jdk/internal/platform` jtreg tests on Linux x86_64 (cgv1). > - [x] GHA - passed (Failed GHA cross compile on RISCV is unrelated) Looks good to me. A comment would be nice, possibly above the imports, warning about using nio. - Marked as reviewed by stuefe (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15416#pullrequestreview-1598518803
Re: RFR: 8314940: Use of NIO in JDKs Metrics implementation causes issues in GraalVM
On Fri, 25 Aug 2023 10:04:28 GMT, Alan Bateman wrote: >>> Something fishy here, is this working around a bug in GraaVM native image >>> or why does this change need to be in JDK? >> >> I've now realized that the bug had an incorrect statement in the >> description. The cycle happens due to the `Runtime.getRuntime().maxMemory()` >> implementation in GraalVM to use JDK `Metrics`, since the `ByteBuffer` [code >> relies on the `Runtime.getRuntime().maxMemory()` >> API](https://github.com/openjdk/jdk/blob/76b9011c9ecb8c0c713a58d034f281ba70d65d4e/src/java.base/share/classes/jdk/internal/misc/VM.java#L261). >> The GraalVM impl to use the JDK Metrics seems a reasonable thing to do, no? >> >> With that said, it's seems a rather uncontroversial change with very limited >> scope. Do you see anything problematic in this patch? Happy to revise if you >> think there are some no-no's :) > >> I've now realized that the bug had an incorrect statement in the >> description. The cycle happens due to the `Runtime.getRuntime().maxMemory()` >> implementation in GraalVM to use JDK `Metrics`, since the `ByteBuffer` [code >> relies on the `Runtime.getRuntime().maxMemory()` >> API](https://github.com/openjdk/jdk/blob/76b9011c9ecb8c0c713a58d034f281ba70d65d4e/src/java.base/share/classes/jdk/internal/misc/VM.java#L261). >> The GraalVM impl to use the JDK Metrics seems a reasonable thing to do, no? >> >> With that said, it's seems a rather uncontroversial change with very limited >> scope. Do you see anything problematic in this patch? Happy to revise if you >> think there are some no-no's :) > > Recursive initialization issues can tricky and often it comes down to > deciding where to break the cycle. In this case, it seems a bit fragile to do > it in CgroupUtil as it should be allowed to use any of the file system APIs > to access cgroups or proc files. Part of me wonders if this would be better > handled in their implementation of jdk.internal.misc.VM or Runtime.maxMemory > but maybe you've been there already? > @AlanBateman Is there anything else you need me to do? If so, please let me > know. Thanks! I don't think the JDK is the right place to workaround this issue. Also, we really need to get back re-implementing FileInputStream and friends on the new API, in which case the original issue will come back again. So I think it would be better to see how this can be fixed in the native image code. - PR Comment: https://git.openjdk.org/jdk/pull/15416#issuecomment-1695902738
RFR: 8315097: Rename createJavaProcessBuilder
Rename createJavaProcessBuilder so that it is not used by mistake instead of createTestJvm. I have used the following sed script: `find -name "*.java" | xargs -n 1 sed -i -e "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` Then I have manually modified ProcessTools.java. In that file I have moved one version of createJavaProcessBuilder so that it is close to the other version. Then I have added a javadoc comment in bold telling: /** * Create ProcessBuilder using the java launcher from the jdk to * be tested. * * Please observe that you likely should use * createTestJvm() instead of this method because createTestJvm() * will add JVM options from "test.vm.opts" and "test.java.opts" * and this method will not do that. * * @param command Arguments to pass to the java command. * @return The ProcessBuilder instance representing the java command. */ I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a better name I could do a rename of the method. I kind of like that it is long and clumsy, that makes it harder to use... I have run tier 1 testing, and I have started more exhaustive testing. - Commit messages: - 8315097: Rename createJavaProcessBuilder Changes: https://git.openjdk.org/jdk/pull/15452/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15452&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8315097 Stats: 756 lines in 462 files changed: 22 ins; 10 del; 724 mod Patch: https://git.openjdk.org/jdk/pull/15452.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15452/head:pull/15452 PR: https://git.openjdk.org/jdk/pull/15452
Withdrawn: 8314940: Use of NIO in JDKs Metrics implementation causes issues in GraalVM
On Thu, 24 Aug 2023 13:16:16 GMT, Severin Gehwolf wrote: > Please review this rather trivial fix to not use `nio` in `CgroupUtil`, part > of the > JDK's Metrics API. The primary motivating factor is that it allows one to use > the > JDK's version of `Metrics` in GraalVM. See the bug for details as to why this > is > needed. > > Testing: > - [x] GraalVM builds with/without the fix and the reproducer (fails > before/works after) > - [x] `jdk/internal/platform` jtreg tests on Linux x86_64 (cgv1). > - [x] GHA - passed (Failed GHA cross compile on RISCV is unrelated) This pull request has been closed without being integrated. - PR: https://git.openjdk.org/jdk/pull/15416
Re: RFR: 8314940: Use of NIO in JDKs Metrics implementation causes issues in GraalVM
On Mon, 28 Aug 2023 15:29:32 GMT, Alan Bateman wrote: > > @AlanBateman Is there anything else you need me to do? If so, please let me > > know. Thanks! > > I don't think the JDK is the right place to workaround this issue. Also, we > really need to get back re-implementing FileInputStream and friends on the > new API, in which case the original issue will come back again. So I think it > would be better to see how this can be fixed in the native image code. OK. I understand. Withdrawing this PR then. - PR Comment: https://git.openjdk.org/jdk/pull/15416#issuecomment-1696011153
Re: RFR: 8315097: Rename createJavaProcessBuilder
On Mon, 28 Aug 2023 15:54:08 GMT, Leo Korinth wrote: > Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. The. changes looks good. Please update copyrights for changed filed. I expect that you completed execution of all tests to ensure that nothing is broken. - Marked as reviewed by lmesnik (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1598884378
RFR: 8314824: Fix serviceability/jvmti/8036666/GetObjectLockCount.java to use vm flags
Arguments were added to the launcher arguments. - Commit messages: - 8314824: Fix serviceability/jvmti/803/GetObjectLockCount.java to use vm flags Changes: https://git.openjdk.org/jdk/pull/15454/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15454&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8314824 Stats: 5 lines in 1 file changed: 2 ins; 2 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15454.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15454/head:pull/15454 PR: https://git.openjdk.org/jdk/pull/15454
Re: RFR: 8311775: [TEST] duplicate verifyHeapDump in several tests [v3]
On Fri, 11 Aug 2023 02:46:36 GMT, Yi Yang wrote: >> This is a follow-up patch of #13667. verifyHeapDump is duplicated in several >> tests, this patch tries to consolidate them into one method. > > Yi Yang has updated the pull request incrementally with one additional commit > since the last revision: > > review from Alex test/hotspot/jtreg/serviceability/dcmd/gc/HeapDumpTest.java line 2: > 1: /* > 2: * Copyright (c) 2015, 2013, Oracle and/or its affiliates. All rights > reserved. typo - should be 2023, not 2013 - PR Review Comment: https://git.openjdk.org/jdk/pull/15202#discussion_r1307809311
RFR: 8314834: serviceability/jdwp/AllModulesCommandTest.java ignores VM flags
The test is fixed to start debuggee with tested VM options. Verified with tier1, running svc tests with different vm flags and virtual thread. - Commit messages: - 8314834: serviceability/jdwp/AllModulesCommandTest.java ignores VM flags Changes: https://git.openjdk.org/jdk/pull/15457/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15457&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8314834 Stats: 31 lines in 1 file changed: 3 ins; 26 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/15457.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15457/head:pull/15457 PR: https://git.openjdk.org/jdk/pull/15457
Re: RFR: JDK-8286789: Test forceEarlyReturn002.java timed out [v2]
On Mon, 21 Aug 2023 05:03:06 GMT, David Holmes wrote: > Infinite loops are not good to have so the test adjustment seems fine in that > regard. But I can't see how using the virtual thread factory could trigger a > problem with this code. It's not clear to me as well. Debugger sends 'quit' command, but debugee does not receive it (and debugee virtual thread is blocked reading from socket). The test is problem-listed and I cannot reproduce it in many runs, so I don't see other way to get progress with this issue. If the test continue to fail with this fix, we'll problemlist it back, at least it adds fresh data for analysis. - PR Comment: https://git.openjdk.org/jdk/pull/15301#issuecomment-1696516425
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v3]
On Mon, 28 Aug 2023 11:03:51 GMT, Afshin Zafari wrote:
>> The `find` method now is
>> ```C++
>> template
>> int find(T* token, bool f(T*, E)) const {
>> ...
>>
>> Any other functions which use this are also changed.
>> Local linux-x64-debug hotspot:tier1 passed. Mach5 tier1 build on linux and
>> Windows passed.
>
> Afshin Zafari has updated the pull request incrementally with one additional
> commit since the last revision:
>
> find_from_end and its caller are also updated.
This is similar to `std::find_if` and should be just:
template
int find(UnaryPredicate p) const {
for (int i = 0; i < _len; i++) {
if (p(_data[i])) {
return i;
}
}
return -1;
}
Regarding the current approach, the comparator should take a `const E&` instead
of an `E`, and the token passed in should be `const` also.
-
PR Comment: https://git.openjdk.org/jdk/pull/15418#issuecomment-1696695109
Re: RFR: 8315097: Rename createJavaProcessBuilder
On Mon, 28 Aug 2023 15:54:08 GMT, Leo Korinth wrote: > Rename createJavaProcessBuilder so that it is not used by mistake instead of > createTestJvm. > > I have used the following sed script: `find -name "*.java" | xargs -n 1 sed > -i -e > "s/createJavaProcessBuilder(/createJavaProcessBuilderIgnoreTestJavaOpts(/g"` > > Then I have manually modified ProcessTools.java. In that file I have moved > one version of createJavaProcessBuilder so that it is close to the other > version. Then I have added a javadoc comment in bold telling: > >/** > * Create ProcessBuilder using the java launcher from the jdk to > * be tested. > * > * Please observe that you likely should use > * createTestJvm() instead of this method because createTestJvm() > * will add JVM options from "test.vm.opts" and "test.java.opts" > * and this method will not do that. > * > * @param command Arguments to pass to the java command. > * @return The ProcessBuilder instance representing the java command. > */ > > > I have used the name createJavaProcessBuilderIgnoreTestJavaOpts because of > the name of Utils.prependTestJavaOpts that adds those VM flags. If you have a > better name I could do a rename of the method. I kind of like that it is long > and clumsy, that makes it harder to use... > > I have run tier 1 testing, and I have started more exhaustive testing. I think the admonition about using this method is a bit strong in that it is natural to use this plain process builder method when a test is going to set its own specific flags for the exec'd process. But I'm okay with renaming to avoid copy'n'paste errors that accidentally use the wrong version. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15452#pullrequestreview-1599483514
Re: Seeking Guidance and Identifying Suitable Bug for a New OpenJDK Contributor
Hello Tham, On 29/08/2023 12:19 pm, Tham To Thi Hong wrote: Dear esteemed members of the jtreg-dev mailing list, You actually sent this email to serviceability-dev. Cheers, David I hope this message finds you well. My name is Tham To, and I'm excited to introduce myself as a new contributor to the OpenJDK community. I am reaching out to seek your valuable guidance and assistance as I embark on this enriching journey. As a newcomer, I am enthusiastic about learning from the experienced developers and contributing to the OpenJDK project. I am particularly interested in making a positive impact by addressing and resolving bugs that align with my skills and interests. I believe that diving into practical challenges will provide me with invaluable learning opportunities and enable me to contribute effectively to the community. I am writing to kindly request your guidance in identifying a suitable bug that is well-suited for someone at my level of experience. While I am eager to contribute, I understand that starting with a bug that matches my familiarity with the project would be the most productive approach. Your insights will help me gain a clearer understanding of the OpenJDK bug landscape and direct my efforts in the right direction. Moreover, I am open to mentorship and collaboration with seasoned developers who can provide guidance and feedback as I work on my chosen bug. I truly believe that the collective knowledge and support of the OpenJDK community will play a pivotal role in shaping my journey as a contributor. I want to express my gratitude for considering my request. Your mentorship will not only facilitate my growth but also contribute to the success of the OpenJDK project. I am looking forward to engaging with the community and making meaningful contributions. Thank you for your time and consideration. Best regards, Tham To (Ms.) Software Engineer I IMT Solutions | Fostering IT Innovation 55-57 Bau Cat 4 St., Tan Binh Dist, Ho Chi Minh City, Vietnam Phone: +84 35 333 4110 Skype: totham024 http://www.imt-soft.com
Re: RFR: 8314834: serviceability/jdwp/AllModulesCommandTest.java ignores VM flags
On Mon, 28 Aug 2023 20:10:59 GMT, Leonid Mesnik wrote: > The test is fixed to start debuggee with tested VM options. > Verified with tier1, running svc tests with different vm flags and virtual > thread. Looks okay. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15457#pullrequestreview-1599609843
Re: RFR: 8314824: Fix serviceability/jvmti/8036666/GetObjectLockCount.java to use vm flags
On Mon, 28 Aug 2023 19:12:04 GMT, Leonid Mesnik wrote: > Arguments were added to the launcher arguments. Looks good. Copyright comment needs an update. Thanks, Serguei - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15454#pullrequestreview-1599614040
