Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v3]
On Tue, 30 Apr 2024 02:05:10 GMT, Serguei Spitsyn wrote: >> Looks like in JVMTI `current_thread` is more common (and `current` is >> usually used in runtime :) > > The plan is to unify this with the approach used by the Runtime team. Replaced all touched "current_thread" and "calling_thread" with "current" - PR Review Comment: https://git.openjdk.org/jdk/pull/18986#discussion_r1585718011
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v3]
> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method > > Testing: tier1-6 Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: renamed current_thread tp current - Changes: - all: https://git.openjdk.org/jdk/pull/18986/files - new: https://git.openjdk.org/jdk/pull/18986/files/d5d614bc..46026322 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18986=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18986=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18986/head:pull/18986 PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8330852: All callers of JvmtiEnvBase::get_threadOop_and_JavaThread should pass current thread explicitly [v2]
> Some cleanup related to JvmtiEnvBase::get_threadOop_and_JavaThread method > > Testing: tier1-6 Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: renamed current_thread to current - Changes: - all: https://git.openjdk.org/jdk/pull/18986/files - new: https://git.openjdk.org/jdk/pull/18986/files/f472f669..d5d614bc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18986=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18986=00-01 Stats: 131 lines in 2 files changed: 0 ins; 1 del; 130 mod Patch: https://git.openjdk.org/jdk/pull/18986.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18986/head:pull/18986 PR: https://git.openjdk.org/jdk/pull/18986
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v3]
On Tue, 30 Apr 2024 01:56:13 GMT, Serguei Spitsyn wrote: >> This is a fix of the following JVMTI scalability issue. A closed benchmark >> with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has >> been loaded. For instance, this is observable when an app is executed under >> control of the Oracle Studio `collect` utility. >> For performance analysis, experiments and numbers, please, see the comment >> below this description. >> >> The fix is to replace the global counter `_VTMS_transition_count` with the >> mark bit `_VTMS_transition_mark` in each `JavaThread`'. >> >> Testing: >> - Tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: correct comments related to VTMS transition counters Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18937#pullrequestreview-2032369700
Re: RFR: 8330969: scalability issue with loaded JVMTI agent [v3]
On Tue, 30 Apr 2024 01:56:13 GMT, Serguei Spitsyn wrote: >> This is a fix of the following JVMTI scalability issue. A closed benchmark >> with millions of virtual threads shows 3X-4X overhead when a JVMTI agent has >> been loaded. For instance, this is observable when an app is executed under >> control of the Oracle Studio `collect` utility. >> For performance analysis, experiments and numbers, please, see the comment >> below this description. >> >> The fix is to replace the global counter `_VTMS_transition_count` with the >> mark bit `_VTMS_transition_mark` in each `JavaThread`'. >> >> Testing: >> - Tested with mach5 tiers 1-6 > > Serguei Spitsyn has updated the pull request incrementally with one > additional commit since the last revision: > > review: correct comments related to VTMS transition counters Marked as reviewed by amenkov (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18937#pullrequestreview-2032255537
Re: RFR: 8322043: HeapDumper should use parallel dump by default [v6]
> The fix makes VM heap dumping parallel by default. > `jcmd GC.heap_dump` and `jmap -dump` had parallel dumping by default, the fix > affects `HotSpotDiagnosticMXBean.dumpHeap()`, `-XX:+HeapDumpBeforeFullGC`, > `-XX:+HeapDumpAfterFullGC` and `-XX:+HeapDumpOnOutOfMemoryError`. > > Testing: > - manually tested different heap dump scenarios with `-Xlog:heapdump`; > - tier1,tier2,hs-tier5-svc; > - all reg.tests that use heap dump. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: removed space-only change in diagnosticCommand.cpp - Changes: - all: https://git.openjdk.org/jdk/pull/18748/files - new: https://git.openjdk.org/jdk/pull/18748/files/641bedc5..eb38705d Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18748=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18748=04-05 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18748.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18748/head:pull/18748 PR: https://git.openjdk.org/jdk/pull/18748
Integrated: 8318682: SA decoding of scalar replaced objects is broken
On Fri, 12 Jan 2024 21:26:36 GMT, Tom Rodriguez wrote: > The changes for JDK-8287061 didn't update the SA decoding logic and there are > other places where the decoding has gotten out of sync with HotSpot. Some of > them can't be tested because they are part of JVMCI but I've added a directed > test for the JDK-8287061 code and a more brute force test that tries to > decode everything. This pull request has now been integrated. Changeset: b96b38c2 Author:Tom Rodriguez URL: https://git.openjdk.org/jdk/commit/b96b38c2c9a310d5fe49b2eda3e113a71223c7d1 Stats: 427 lines in 13 files changed: 413 ins; 4 del; 10 mod 8318682: SA decoding of scalar replaced objects is broken Reviewed-by: cjplummer, cslucas - PR: https://git.openjdk.org/jdk/pull/17407
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v6]
On Wed, 17 Jan 2024 19:52:32 GMT, Tom Rodriguez wrote: >> The changes for JDK-8287061 didn't update the SA decoding logic and there >> are other places where the decoding has gotten out of sync with HotSpot. >> Some of them can't be tested because they are part of JVMCI but I've added a >> directed test for the JDK-8287061 code and a more brute force test that >> tries to decode everything. > > Tom Rodriguez has updated the pull request incrementally with one additional > commit since the last revision: > > Review comments I merged with master and reran all tests which were clean - PR Comment: https://git.openjdk.org/jdk/pull/17407#issuecomment-2086158736
Re: RFR: 8318682: SA decoding of scalar replaced objects is broken [v7]
> The changes for JDK-8287061 didn't update the SA decoding logic and there are > other places where the decoding has gotten out of sync with HotSpot. Some of > them can't be tested because they are part of JVMCI but I've added a directed > test for the JDK-8287061 code and a more brute force test that tries to > decode everything. Tom Rodriguez 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 10 additional commits since the last revision: - Merge branch 'master' into tkr-hsdb-assert - Merge branch 'master' into tkr-hsdb-assert - Review comments - Remove testdebuginfodecode command - Update src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/code/NMethod.java Co-authored-by: Andrey Turbanov - Update src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/CommandProcessor.java Co-authored-by: Andrey Turbanov - Pass the proper options to the lingered app - Fix problem list and correct jtreg arguments - Add missing files - 8318682: SA decoding of scalar replaced objects is broken - Changes: - all: https://git.openjdk.org/jdk/pull/17407/files - new: https://git.openjdk.org/jdk/pull/17407/files/f25c92ef..3cafefc6 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17407=06 - incr: https://webrevs.openjdk.org/?repo=jdk=17407=05-06 Stats: 663952 lines in 9058 files changed: 132224 ins; 168721 del; 363007 mod Patch: https://git.openjdk.org/jdk/pull/17407.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17407/head:pull/17407 PR: https://git.openjdk.org/jdk/pull/17407
Re: RFR: 8308033: The jcmd thread dump related tests should test virtual threads [v2]
On Tue, 30 Apr 2024 12:08:21 GMT, Jaikiran Pai wrote: >> Can I please get a review of these test-only changes which proposes to >> remove the `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and >> `test/jdk/sun/tools/jstack/BasicJStackTest.java` tests from >> `ProblemList-Virtual.txt`? >> >> When jtreg was enhanced to allow running the tests from within a virtual >> (main) thread, some tests were problem listed since they either were failing >> at that time or the test code would require additional work to make it work >> when the current thread is a virtual thread. The >> `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and >> `test/jdk/sun/tools/jstack/BasicJStackTest.java` are 2 such threads which >> require special handling when the current thread is a virtual thread. >> >> `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` has been updated to >> use a different command to dump stacktraces when the test runs in a virtual >> thread. I went back and looked at the original issue for which this test was >> introduced and based on that, using a different command to dump the >> stacktraces shouldn't impact what the test was originally intended to test. >> >> `test/jdk/sun/tools/jstack/BasicJStackTest.java` has been updated to be >> skipped when the current thread is the virtual thread. This is because the >> test exercises the `jstack` tool which doesn't print stacktraces of virtual >> threads and thus the test isn't applicable for virtual threads. >> >> I've run these tests with this change, both with platform threads and >> virtual threads in our CI and the tests complete without failures. > > Jaikiran Pai has updated the pull request incrementally with one additional > commit since the last revision: > > address additional problem listed tests in hotspot/jtreg/serviceability I've opened this for review again. The additional changes were straightforward. The `test/hotspot/jtreg/serviceability/dcmd/thread/` tests which were problem listed were testing `Thread.print` command against the current thread. Those have now been updated to skip the tests when the current thread is a virtual thread, since `Thread.print` doesn't generate thread dumps for virtual threads. The `test/hotspot/jtreg/serviceability/tmtools/jstack/DaemonThreadTest.java` has been updated to explicitly mark a (platform) thread launched from within the test as non-daemon to verify that the output of `jstack` correctly captures that state. With these changes all these tests pass, both when jtreg uses platform thread and virtual thread to launch the tests. - PR Comment: https://git.openjdk.org/jdk/pull/19016#issuecomment-2085163934
Re: RFR: 8308033: The jcmd thread dump related tests should test virtual threads [v2]
> Can I please get a review of these test-only changes which proposes to remove > the `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and > `test/jdk/sun/tools/jstack/BasicJStackTest.java` tests from > `ProblemList-Virtual.txt`? > > When jtreg was enhanced to allow running the tests from within a virtual > (main) thread, some tests were problem listed since they either were failing > at that time or the test code would require additional work to make it work > when the current thread is a virtual thread. The > `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and > `test/jdk/sun/tools/jstack/BasicJStackTest.java` are 2 such threads which > require special handling when the current thread is a virtual thread. > > `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` has been updated to use > a different command to dump stacktraces when the test runs in a virtual > thread. I went back and looked at the original issue for which this test was > introduced and based on that, using a different command to dump the > stacktraces shouldn't impact what the test was originally intended to test. > > `test/jdk/sun/tools/jstack/BasicJStackTest.java` has been updated to be > skipped when the current thread is the virtual thread. This is because the > test exercises the `jstack` tool which doesn't print stacktraces of virtual > threads and thus the test isn't applicable for virtual threads. > > I've run these tests with this change, both with platform threads and virtual > threads in our CI and the tests complete without failures. Jaikiran Pai has updated the pull request incrementally with one additional commit since the last revision: address additional problem listed tests in hotspot/jtreg/serviceability - Changes: - all: https://git.openjdk.org/jdk/pull/19016/files - new: https://git.openjdk.org/jdk/pull/19016/files/201be489..6193cfbb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=19016=01 - incr: https://webrevs.openjdk.org/?repo=jdk=19016=00-01 Stats: 20 lines in 3 files changed: 12 ins; 4 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/19016.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19016/head:pull/19016 PR: https://git.openjdk.org/jdk/pull/19016
Re: RFR: 8308033: The jcmd thread dump related tests should test virtual threads
On Tue, 30 Apr 2024 09:54:40 GMT, Jaikiran Pai wrote: > Can I please get a review of these test-only changes which proposes to remove > the `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and > `test/jdk/sun/tools/jstack/BasicJStackTest.java` tests from > `ProblemList-Virtual.txt`? > > When jtreg was enhanced to allow running the tests from within a virtual > (main) thread, some tests were problem listed since they either were failing > at that time or the test code would require additional work to make it work > when the current thread is a virtual thread. The > `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and > `test/jdk/sun/tools/jstack/BasicJStackTest.java` are 2 such threads which > require special handling when the current thread is a virtual thread. > > `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` has been updated to use > a different command to dump stacktraces when the test runs in a virtual > thread. I went back and looked at the original issue for which this test was > introduced and based on that, using a different command to dump the > stacktraces shouldn't impact what the test was originally intended to test. > > `test/jdk/sun/tools/jstack/BasicJStackTest.java` has been updated to be > skipped when the current thread is the virtual thread. This is because the > test exercises the `jstack` tool which doesn't print stacktraces of virtual > threads and thus the test isn't applicable for virtual threads. > > I've run these tests with this change, both with platform threads and virtual > threads in our CI and the tests complete without failures. > 8308033 is used in problem lists: [test/hotspot/jtreg/ProblemList-Virtual.txt] It looks like the same bug id is being used in multiple different areas for different tests. I'll put this PR on draft for now, till I review if the following tests that are linked against this bug id can be removed from the problem listing or fixed too: serviceability/dcmd/thread/PrintConcurrentLocksTest.java 8308033 generic-all serviceability/dcmd/thread/PrintTest.java 8308033 generic-all serviceability/dcmd/thread/ThreadDumpToFileTest.java 8308033 generic-all serviceability/tmtools/jstack/DaemonThreadTest.java 8308033 generic-all - PR Comment: https://git.openjdk.org/jdk/pull/19016#issuecomment-2085044069
RFR: 8308033: The jcmd thread dump related tests should test virtual threads
Can I please get a review of these test-only changes which proposes to remove the `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and `test/jdk/sun/tools/jstack/BasicJStackTest.java` tests from `ProblemList-Virtual.txt`? When jtreg was enhanced to allow running the tests from within a virtual (main) thread, some tests were problem listed since they either were failing at that time or the test code would require additional work to make it work when the current thread is a virtual thread. The `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` and `test/jdk/sun/tools/jstack/BasicJStackTest.java` are 2 such threads which require special handling when the current thread is a virtual thread. `test/jdk/sun/tools/jcmd/JcmdOutputEncodingTest.java` has been updated to use a different command to dump stacktraces when the test runs in a virtual thread. I went back and looked at the original issue for which this test was introduced and based on that, using a different command to dump the stacktraces shouldn't impact what the test was originally intended to test. `test/jdk/sun/tools/jstack/BasicJStackTest.java` has been updated to be skipped when the current thread is the virtual thread. This is because the test exercises the `jstack` tool which doesn't print stacktraces of virtual threads and thus the test isn't applicable for virtual threads. I've run these tests with this change, both with platform threads and virtual threads in our CI and the tests complete without failures. - Commit messages: - 8308033: The jcmd thread dump related tests should test virtual threads Changes: https://git.openjdk.org/jdk/pull/19016/files Webrev: https://webrevs.openjdk.org/?repo=jdk=19016=00 Issue: https://bugs.openjdk.org/browse/JDK-8308033 Stats: 51 lines in 3 files changed: 42 ins; 3 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/19016.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/19016/head:pull/19016 PR: https://git.openjdk.org/jdk/pull/19016
Re: RFR: 8330694: Rename 'HeapRegion' to 'G1HeapRegion' [v4]
On Mon, 29 Apr 2024 22:54:18 GMT, Kim Barrett wrote: > > mach5 higher tier SA tests are fine. What are your plans for the remaining > > SA renames (would highly recommend to add) and the G1HeapRegion related > > helper classes? > > I suggest the related helper classes be done in further followups, not make > this change even larger. Fine with me, will file an issue about the helper classes. - PR Comment: https://git.openjdk.org/jdk/pull/18871#issuecomment-2084539234
