Re: RFR: 8323681: SA PointerFinder code should support G1 [v2]
> This PR adds G1 support to PointerFinder/PointerLocation. Previously we only
> had SerialGC support. Previously for G1 addresses SA would only report that
> the address is "In unknown section of Java heap" with no other details. Now
> it provides details for G1 addresses. Here are some examples for clhsdb
> `threadcontext` output. `threadcontext` dumps the contents of the thread's
> registers, some of which are often in the java heap. In the output below the
> first line is without verbose output and the 2nd is with:
>
>
> rbp: 0xa0004080: In G1 heap region
> rbp: 0xa0004080: In G1 heap Region:
> 0xa000,0xa0018a30,0xa100:Old
>
>
> I also added an improvement to how SA deals with addresses in the TLAB. It
> used to only report that the address is in a TLAB and provide details about
> the TLAB in verbose mode. Now if verbose mode is used, the heap region
> information is included after the TLAB information. Here again is an example
> without and with verbose output:
>
>
> rsi: 0x00016600: In TLAB for thread "main"
> sun.jvm.hotspot.runtime.JavaThread@0x7f11c8029250
> rsi: 0x00016600: In TLAB for thread ("main" #1 prio=5
> tid=0x7f11c8029250 nid=1503 runnable [0x]
>java.lang.Thread.State: RUNNABLE
>JavaThread state: _thread_in_java
> )
> [0x00016600,0x0001662d0c90,0x0001667ffdc0,{0x00016680})
> In G1 heap Region:
> 0x00016600,0x00016680,0x00016700:Eden
>
>
> Note at the end it indicates the address is in the Eden space, which is
> probably always the case for G1 TLAB addresses. For SerialGC is shows
> something similar.
>
>
> rsi: 0x88ff99e0: In TLAB for thread "main"
> sun.jvm.hotspot.runtime.JavaThread@0x7f0544029110
> rsi: 0x88ff99e0: In TLAB for thread ("main" #1 prio=5
> tid=0x7f0544029110 nid=3098 runnable [0x]
>java.lang.Thread.State: RUNNABLE
>JavaThread state: _thread_in_java
> )
> [0x88ff99e0,0x8978c090,0x89ae54b0,{0x89ae56f0})
> In heap new generation: eden
> [0x8020,0x89ae56f0,0xa242) space capacity =
> 572653568, 27.99656213789626 used
> from [0xa686,0xa6860030,0xaaca) space
> capacity = 71565312, 6.707160027472528E-5 used
> to [0xa242,0xa242,0xa686) space
> capacity = 71565312, 0.0 used
>
>
> Testing all svc test in tier1, tier2, and tier5. Currently in progress.
Chris Plummer has updated the pull request incrementally with three additional
commits since the last revision:
- Minor cleanup of output
- Don't assert if the address is in the G1 heap, but not in an region. Not all
of the mapped part of the heap is in use by regions.
- Fix isInRegion() to check the entire region, not just the in use part.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/17691/files
- new: https://git.openjdk.org/jdk/pull/17691/files/6da5b903..37b3f17b
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=17691&range=01
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=17691&range=00-01
Stats: 8 lines in 3 files changed: 2 ins; 0 del; 6 mod
Patch: https://git.openjdk.org/jdk/pull/17691.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17691/head:pull/17691
PR: https://git.openjdk.org/jdk/pull/17691
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v3]
On Sat, 3 Feb 2024 00:44:42 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> indent
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 64:
>
>> 62: static bool is_static_field(JNIEnv* env, jclass klass, jfieldID fid) {
>> 63: enum {
>> 64:ACC_STATIC= 0x0008,
>
> Nit: Indent is 1 instead of 2.
fixed.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1476916568
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v4]
> The fix adds new test for FollowReferences JVMTI function to verify > correctness of reported field indexes. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: indent - Changes: - all: https://git.openjdk.org/jdk/pull/17580/files - new: https://git.openjdk.org/jdk/pull/17580/files/a0fffef3..42e79303 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17580.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17580/head:pull/17580 PR: https://git.openjdk.org/jdk/pull/17580
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v3]
On Fri, 2 Feb 2024 23:10:32 GMT, Alex Menkov wrote:
>> The fix adds new test for FollowReferences JVMTI function to verify
>> correctness of reported field indexes.
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> indent
Marked as reviewed by sspitsyn (Reviewer).
test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
line 64:
> 62: static bool is_static_field(JNIEnv* env, jclass klass, jfieldID fid) {
> 63: enum {
> 64:ACC_STATIC= 0x0008,
Nit: Indent is 1 instead of 2.
-
PR Review: https://git.openjdk.org/jdk/pull/17580#pullrequestreview-1860688173
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1476897942
Re: RFR: JDK-8318566: Heap walking functions should not use FilteredFieldStream [v4]
On Sat, 3 Feb 2024 00:14:15 GMT, Alex Menkov wrote: >> FilteredFieldStream used by heap walking functions to iterate through >> klass/superclasses/interfaces fields are known to have poor performance (see >> [JDK-8317692](https://bugs.openjdk.org/browse/JDK-8317692) for details). >> Heap walking API implementation is the last user of the klasses. >> The fix reworks iteration through klass/superclasses/interfaces fields and >> drops FilteredFieldStream-related code. >> Additionally removed/updated includes of reflectionUtils.hpp. >> >> Testing: >> - tier1..4; >> - test/hotspot/jtreg/vmTestbase/nsk/jvmti (contains tests for different >> heap walking functions); >> - new test from #17580 (now the test runs several times faster). > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > updated comment Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17661#pullrequestreview-1860685491
Re: RFR: JDK-8318566: Heap walking functions should not use FilteredFieldStream [v3]
On Fri, 2 Feb 2024 08:05:06 GMT, Serguei Spitsyn wrote: >> Alex Menkov has updated the pull request incrementally with one additional >> commit since the last revision: >> >> jcheck > > src/hotspot/share/prims/jvmtiTagMap.cpp line 499: > >> 497: } >> 498: // update total_field_number for superclass >> 499: total_field_number = start_index; > > Nit: The local variable name `total_field_number` is a little bit confusing > because it is instantly decreased here (see also the line 490). I'm not sure > what name to suggest though. Maybe the comment at line 498 needs an update to > say this number is decreased here. done - PR Review Comment: https://git.openjdk.org/jdk/pull/17661#discussion_r1476887153
Re: RFR: JDK-8318566: Heap walking functions should not use FilteredFieldStream [v4]
> FilteredFieldStream used by heap walking functions to iterate through > klass/superclasses/interfaces fields are known to have poor performance (see > [JDK-8317692](https://bugs.openjdk.org/browse/JDK-8317692) for details). > Heap walking API implementation is the last user of the klasses. > The fix reworks iteration through klass/superclasses/interfaces fields and > drops FilteredFieldStream-related code. > Additionally removed/updated includes of reflectionUtils.hpp. > > Testing: > - tier1..4; > - test/hotspot/jtreg/vmTestbase/nsk/jvmti (contains tests for different > heap walking functions); > - new test from #17580 (now the test runs several times faster). Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: updated comment - Changes: - all: https://git.openjdk.org/jdk/pull/17661/files - new: https://git.openjdk.org/jdk/pull/17661/files/d6ab43b4..1152e718 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17661&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17661&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17661.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17661/head:pull/17661 PR: https://git.openjdk.org/jdk/pull/17661
RFR: 8323681: SA PointerFinder code should support G1
This PR adds G1 support to PointerFinder/PointerLocation. Previously we only
had SerialGC support. Previously for G1 addresses SA would only report that the
address is "In unknown section of Java heap" with no other details. Now it
provides details for G1 addresses. Here are some examples for clhsdb
`threadcontext` output. `threadcontext` dumps the contents of the thread's
registers, some of which are often in the java heap. In the output below the
first line is without verbose output and the 2nd is with:
rbp: 0xa0004080: In G1 heap region
rbp: 0xa0004080: In G1 heap Region:
0xa000,0xa0018a30,0xa100:Old
I also added an improvement to how SA deals with addresses in the TLAB. It used
to only report that the address is in a TLAB and provide details about the TLAB
in verbose mode. Now if verbose mode is used, the heap region information is
included after the TLAB information. Here again is an example without and with
verbose output:
rsi: 0x00016600: In TLAB for thread "main"
sun.jvm.hotspot.runtime.JavaThread@0x7f11c8029250
rsi: 0x00016600: In TLAB for thread ("main" #1 prio=5
tid=0x7f11c8029250 nid=1503 runnable [0x]
java.lang.Thread.State: RUNNABLE
JavaThread state: _thread_in_java
)
[0x00016600,0x0001662d0c90,0x0001667ffdc0,{0x00016680})
In G1 heap Region: 0x00016600,0x00016680,0x00016700:Eden
Note at the end it indicates the address is in the Eden space, which is
probably always the case for G1 TLAB addresses. For SerialGC is shows something
similar.
rsi: 0x88ff99e0: In TLAB for thread "main"
sun.jvm.hotspot.runtime.JavaThread@0x7f0544029110
rsi: 0x88ff99e0: In TLAB for thread ("main" #1 prio=5
tid=0x7f0544029110 nid=3098 runnable [0x]
java.lang.Thread.State: RUNNABLE
JavaThread state: _thread_in_java
)
[0x88ff99e0,0x8978c090,0x89ae54b0,{0x89ae56f0})
In heap new generation: eden
[0x8020,0x89ae56f0,0xa242) space capacity =
572653568, 27.99656213789626 used
from [0xa686,0xa6860030,0xaaca) space
capacity = 71565312, 6.707160027472528E-5 used
to [0xa242,0xa242,0xa686) space
capacity = 71565312, 0.0 used
Testing all svc test in tier1, tier2, and tier5. Currently in progress.
-
Commit messages:
- Add G1 support to PointerFinder and PointerLocation.
Changes: https://git.openjdk.org/jdk/pull/17691/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17691&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8323681
Stats: 70 lines in 4 files changed: 54 ins; 6 del; 10 mod
Patch: https://git.openjdk.org/jdk/pull/17691.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/17691/head:pull/17691
PR: https://git.openjdk.org/jdk/pull/17691
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v2]
On Fri, 2 Feb 2024 06:47:22 GMT, Serguei Spitsyn wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> feedback
>
> test/hotspot/jtreg/serviceability/jvmti/FollowReferences/FieldIndices/libFieldIndicesTest.cpp
> line 512:
>
>> 510: JNIEXPORT jboolean JNICALL
>> 511: Java_FieldIndicesTest_testFailed(JNIEnv *env, jclass cls) {
>> 512: return test_failed ? JNI_TRUE : JNI_FALSE;
>
> The indent for native files has to be 2, not 4. Even though there are still
> some tests with wrong indents I'd suggest for new files to follow this rule.
Fixed. Going to integrate the change after sanity testing
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17580#discussion_r1476865492
Re: RFR: JDK-8317636: Improve heap walking API tests to verify correctness of field indexes [v3]
> The fix adds new test for FollowReferences JVMTI function to verify > correctness of reported field indexes. Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: indent - Changes: - all: https://git.openjdk.org/jdk/pull/17580/files - new: https://git.openjdk.org/jdk/pull/17580/files/c5e7b787..a0fffef3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17580&range=01-02 Stats: 391 lines in 1 file changed: 56 ins; 56 del; 279 mod Patch: https://git.openjdk.org/jdk/pull/17580.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17580/head:pull/17580 PR: https://git.openjdk.org/jdk/pull/17580
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
Hi Kevin, Having a clear command spec to read and argue about would be helpful, especially since this is not a simple commnd but a whole command group. Exposing such a low level interface (this is supposed to go into product VMs, right?) may carry some risks that could arguably fall unter CSR. That said, what use case do you envisage? To me, these commands are usually only useful in the debugger, when I deal with numerical adresses. Cheers, Thomas p.s. please note that many folks are at fosdem right now. On Fri 2. Feb 2024 at 19:35, Kevin Walls wrote: > Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are > not general customer-facing tools. > > - > > Commit messages: > - Tidy up the safety checks > - Whitebox not required, just check option properties. > - unnecessary parameter to find > - Test update. Recognise ZGC oops differently. Formatting. > - typo > - Separate is_good_oop... method to avoid changing existing asserts. > - More oop safety > - 8318026: jcmd should provide access to low-level JVM debug information > > Changes: https://git.openjdk.org/jdk/pull/17655/files > Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17655&range=00 > Issue: https://bugs.openjdk.org/browse/JDK-8318026 > Stats: 371 lines in 5 files changed: 369 ins; 0 del; 2 mod > Patch: https://git.openjdk.org/jdk/pull/17655.diff > Fetch: git fetch https://git.openjdk.org/jdk.git > pull/17655/head:pull/17655 > > PR: https://git.openjdk.org/jdk/pull/17655 >
Integrated: 8323680: SA PointerFinder code can do a better job of leveraging existing code to determine if an address is in the TLAB
On Thu, 18 Jan 2024 22:54:26 GMT, Chris Plummer wrote:
> In PointerFinder.java we have some code to determine if a pointer is in a
> TLAB, but it only executes for the SerialGC. It should work for all GCs, so I
> moved the code out of the SerialGC block.
>
> I also cleaned up the printing in PointerLocation. java a bit so when not
> using verbose mode not as much info about the tlab address is printed. This
> is consistent with other addresses, such as java stack addresses, which is
> what I modeled this change on.
>
> It's hard to test this change since it is hard to consistently get an address
> to be in the tlab. I wrote a little test program that just sits in a loop
> doing allocations. I attached to it with clhsdb and ran the threadcontext
> command, which does a fincpc on each register. About half the time the main
> thread was suspended in a frame where some registers where pointing into the
> tlab, and I confirmed this was the case for both SerialGC and G1. Here's an
> example of one register with verbose off and verbose on:
>
> rsi: 0x8a5d4448: In TLAB for thread "main"
> sun.jvm.hotspot.runtime.JavaThread@0x7ffa24029000
>
> rsi: 0x8a5d4448: In TLAB for thread ("main" #1 prio=5
> tid=0x7ffa24029000 nid=25392 runnable [0x]
>java.lang.Thread.State: RUNNABLE
>JavaThread state: _thread_in_java
> )
> [0x8a5d4448,0x8ab724b8,0x8b0c0250,{0x8b0c0490})
>
> For testing I ran all tier1, tier2, and tier5 svc tests (still in progress)
This pull request has now been integrated.
Changeset: 7476e290
Author:Chris Plummer
URL:
https://git.openjdk.org/jdk/commit/7476e2905380a60c7653cb69e1afded116852785
Stats: 56 lines in 2 files changed: 25 ins; 22 del; 9 mod
8323680: SA PointerFinder code can do a better job of leveraging existing code
to determine if an address is in the TLAB
Reviewed-by: kevinw, sspitsyn
-
PR: https://git.openjdk.org/jdk/pull/17494
Re: RFR: 8323680: SA PointerFinder code can do a better job of leveraging existing code to determine if an address is in the TLAB [v2]
On Fri, 26 Jan 2024 21:20:04 GMT, Chris Plummer wrote:
>> In PointerFinder.java we have some code to determine if a pointer is in a
>> TLAB, but it only executes for the SerialGC. It should work for all GCs, so
>> I moved the code out of the SerialGC block.
>>
>> I also cleaned up the printing in PointerLocation. java a bit so when not
>> using verbose mode not as much info about the tlab address is printed. This
>> is consistent with other addresses, such as java stack addresses, which is
>> what I modeled this change on.
>>
>> It's hard to test this change since it is hard to consistently get an
>> address to be in the tlab. I wrote a little test program that just sits in a
>> loop doing allocations. I attached to it with clhsdb and ran the
>> threadcontext command, which does a fincpc on each register. About half the
>> time the main thread was suspended in a frame where some registers where
>> pointing into the tlab, and I confirmed this was the case for both SerialGC
>> and G1. Here's an example of one register with verbose off and verbose on:
>>
>> rsi: 0x8a5d4448: In TLAB for thread "main"
>> sun.jvm.hotspot.runtime.JavaThread@0x7ffa24029000
>>
>> rsi: 0x8a5d4448: In TLAB for thread ("main" #1 prio=5
>> tid=0x7ffa24029000 nid=25392 runnable [0x]
>>java.lang.Thread.State: RUNNABLE
>>JavaThread state: _thread_in_java
>> )
>> [0x8a5d4448,0x8ab724b8,0x8b0c0250,{0x8b0c0490})
>>
>> For testing I ran all tier1, tier2, and tier5 svc tests (still in progress)
>
> Chris Plummer has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Get rid of inTLAB field. Not needed
Thanks for the reviews Kevin and Seguei!
-
PR Comment: https://git.openjdk.org/jdk/pull/17494#issuecomment-1924667202
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote:
> Introduce the jcmd "VM.debug" to implement access to a useful set of the
> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...".
>
> Not recommended for live production use. Calling these "debug" utilities,
> and not including them in the jcmd help output, is to remind us they are not
> general customer-facing tools.
test/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java line 178:
> 176: String p = m.group(regexGroup);
> 177: ptr = new BigInteger(p, 16);
> 178: System.out.println("Found '" + pattern +"', using
> pointer: 0x" + ptr.toString(16));
Would it be more logical to say this?
"Found pointer: 0x" + ptr.toString(16)'" + " for pattern " +pattern
test/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java line 201:
> 199: run(new JMXExecutor());
> 200: }
> 201:*/
comment to remove
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1474686608
PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1474688481
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Thu, 1 Feb 2024 15:47:24 GMT, Ludvig Janiuk wrote: >> Introduce the jcmd "VM.debug" to implement access to a useful set of the >> established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". >> >> Not recommended for live production use. Calling these "debug" utilities, >> and not including them in the jcmd help output, is to remind us they are not >> general customer-facing tools. > > test/hotspot/jtreg/serviceability/dcmd/vm/VMDebugTest.java line 201: > >> 199: run(new JMXExecutor()); >> 200: } >> 201:*/ > > comment to remove plus some formatting below in this file - PR Review Comment: https://git.openjdk.org/jdk/pull/17655#discussion_r1474688937
RFR: 8318026: jcmd should provide access to low-level JVM debug information
Introduce the jcmd "VM.debug" to implement access to a useful set of the established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". Not recommended for live production use. Calling these "debug" utilities, and not including them in the jcmd help output, is to remind us they are not general customer-facing tools. - Commit messages: - Tidy up the safety checks - Whitebox not required, just check option properties. - unnecessary parameter to find - Test update. Recognise ZGC oops differently. Formatting. - typo - Separate is_good_oop... method to avoid changing existing asserts. - More oop safety - 8318026: jcmd should provide access to low-level JVM debug information Changes: https://git.openjdk.org/jdk/pull/17655/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17655&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8318026 Stats: 371 lines in 5 files changed: 369 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/17655.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17655/head:pull/17655 PR: https://git.openjdk.org/jdk/pull/17655
Re: RFR: 8318026: jcmd should provide access to low-level JVM debug information
On Wed, 31 Jan 2024 14:22:44 GMT, Kevin Walls wrote: > Introduce the jcmd "VM.debug" to implement access to a useful set of the > established debug.cpp utilities, with "jcmd PID VM.debug subcommand ...". > > Not recommended for live production use. Calling these "debug" utilities, > and not including them in the jcmd help output, is to remind us they are not > general customer-facing tools. (I started a CSR but have withdraw it, as it is established that jcmd is a diagnostic tool, and does not require a CSR for new options, similar to HotSpot diagnostic flags). Adding jcmd access to debug tools, for a live process. That includes a process started with -XX:+ShowMessageBoxOnError which has stopped with an error. At that point, the VM has not created the hs_err fatal error log, but we can already run "jcmd PID VM.info", to provide much of what would appear in the hs_err log, and a native debugger can be attached to discover the call stack. The "find" subcommand reads an address from the text of its parameter, to lookup pointers found in VM.info output or in gdb sessions. It is protected by the UnlockDiagnosticVMOptions option (so is always enabled in debug builds). With the dbg_is_good_oop changes here, I can examine hundreds of pointers around a known good oop, recognise bad objects as such and skip them, without crashing. Before the additions, it was possible to crash the target, e.g. following a null klass pointer when looking at an address that is not an object. The "not recommended for live production use" advice still stands, and diagnostic option requirements remain, in case a crash is possible when the find command is given a pointer to the right (or wrong) kind of data in the Java heap. Aimed at people with source code access/familiarity, hence not documenting the findclass/findmethod flags in great detail. Commands and output should be expected to evolve. Thanks @LudwikJaniuk for the nits above, and for the other testing you did with this. - PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1921013935 PR Comment: https://git.openjdk.org/jdk/pull/17655#issuecomment-1923692015
Re: RFR: 8325180: Rename jvmti_FollowRefObjects.h
On Fri, 2 Feb 2024 16:34:19 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_FollowRefObjects.h > to jvmti_FollowRefObjects.hpp, and replaces uses of NULL in the file. > > Testing: mach5 tier1 Looks good. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17689#pullrequestreview-1859989205
RFR: 8325180: Rename jvmti_FollowRefObjects.h
Please review this trivial change that renames the file test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/jvmti_FollowRefObjects.h to jvmti_FollowRefObjects.hpp, and replaces uses of NULL in the file. Testing: mach5 tier1 - Commit messages: - rename NULLs in jvmti_FollwRefObjects.hpp - rename jvmti_FollowRefObjects.h Changes: https://git.openjdk.org/jdk/pull/17689/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=17689&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8325180 Stats: 8 lines in 5 files changed: 0 ins; 0 del; 8 mod Patch: https://git.openjdk.org/jdk/pull/17689.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17689/head:pull/17689 PR: https://git.openjdk.org/jdk/pull/17689
Integrated: 8325055: Rename Injector.h
On Wed, 31 Jan 2024 15:08:14 GMT, Kim Barrett wrote: > Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/Injector.h to Injector.hpp. > > Testing: mach5 tier1 This pull request has now been integrated. Changeset: 6787c4c3 Author:Kim Barrett URL: https://git.openjdk.org/jdk/commit/6787c4c3dd11d4d8db8255e59a1d71b6ab03cebb Stats: 10 lines in 4 files changed: 0 ins; 0 del; 10 mod 8325055: Rename Injector.h Reviewed-by: dholmes, amenkov, sspitsyn - PR: https://git.openjdk.org/jdk/pull/17656
Re: RFR: 8325055: Rename Injector.h [v3]
On Thu, 1 Feb 2024 07:12:08 GMT, David Holmes wrote: >> Kim Barrett 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: >> >> - Merge branch 'master' into Injector >> - fix name in README >> - rename Injector.h > > Seems fine. > Thanks Thanks for reviews @dholmes-ora , @alexmenkov , and @sspitsyn . - PR Comment: https://git.openjdk.org/jdk/pull/17656#issuecomment-1924195846
Re: RFR: 8325055: Rename Injector.h [v3]
> Please review this trivial change that renames the file > test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/Injector.h to Injector.hpp. > > Testing: mach5 tier1 Kim Barrett 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: - Merge branch 'master' into Injector - fix name in README - rename Injector.h - Changes: - all: https://git.openjdk.org/jdk/pull/17656/files - new: https://git.openjdk.org/jdk/pull/17656/files/0ab5cf73..02077283 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=17656&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=17656&range=01-02 Stats: 6112 lines in 157 files changed: 4498 ins; 767 del; 847 mod Patch: https://git.openjdk.org/jdk/pull/17656.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17656/head:pull/17656 PR: https://git.openjdk.org/jdk/pull/17656
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 07:01:33 GMT, Sam James wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add kludge to BufferedRenderPipe.c for AIX > > make/modules/jdk.hotspot.agent/Lib.gmk line 31: > >> 29: >> 30: ifeq ($(call isTargetOs, linux), true) >> 31: SA_CFLAGS := -D_FILE_OFFSET_BITS=64 > > We have two choices to feel a bit more comfortable: > 1) We unconditionally `static_assert` in a few places for large `off_t`, or > 2) We only do it for platforms we know definitely support F_O_B and aren't > AIX (which we've handled separately). > > Not sure that's strictly necessary though. Also, if something understands > LARGEFILE*_SOURCE, then it probably understood F_O_B, or at least has some > macro to control it. Just thinking aloud. I'm guessing your comment was really more general, and just happened to be placed here? The reason I am removing `-D_FILE_OFFSET_BITS=64` here is that it is always set for all JDK compilations. 1. I don't know why you would not trust that compiler flags in the build system would work, but if you really want to add a static_assert, let me know where you want it. 2. No, AIX is not handled separately. It is handled as part of this PR. You are probably thinking about JDK-8324834, but that was only about Hotspot. This PR is about all the other JDK native libraries. - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476236956
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 15:48:04 GMT, Magnus Ihse Bursie wrote: >> src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c line 365: >> >>> 363: #else >>> 364: // Make sure we link to the 64-bit version of the functions >>> 365: my_openat_func = (openat_func*) dlsym(RTLD_DEFAULT, "openat64"); >> >> Explain this part to me, if you wouldn't mind? (Why do we keep the `64` >> variants?) > > I wrote earlier: > >> There is one change that merit highlighting: In >> src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c, I kept the dlsym >> lookup for openat64, fstatat64 and fdopendir64, on non-BSD OSes (i.e. Linux >> and AIX), and on AIX, respectively. This seems to me to be the safest >> choice, as we do not need to know if a lookup of openat would yield a 32-bit >> or a 64-bit version. (I frankly don't know, but I'm guessing it would yield >> the 32-bit version.) Basically, my understanding is that a call to "openat" in the source file will be converted into a call to openat64 on 32-bit platforms. When we look up the function using dlsym, I assume we need to find the 64-bit version directly. Even if this is incorrect, it seems at least *safe* to do it this way. - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476231574
Re: RFR: 8324539: Do not use LFS64 symbols in JDK libs [v7]
On Fri, 2 Feb 2024 07:02:43 GMT, Sam James wrote: >> Magnus Ihse Bursie has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add kludge to BufferedRenderPipe.c for AIX > > src/java.base/linux/native/libnio/ch/FileDispatcherImpl.c line 94: > >> 92: return IOS_UNSUPPORTED_CASE; >> 93: >> 94: loff_t offset = (loff_t)position; > > Is this `loff_t` for the benefit of `copy_file_range`? That is how copy_file_range is defined. The cast to off64_t was technically incorrect (but they ended up being the same type). > src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c line 365: > >> 363: #else >> 364: // Make sure we link to the 64-bit version of the functions >> 365: my_openat_func = (openat_func*) dlsym(RTLD_DEFAULT, "openat64"); > > Explain this part to me, if you wouldn't mind? (Why do we keep the `64` > variants?) I wrote earlier: > There is one change that merit highlighting: In > src/java.base/unix/native/libnio/fs/UnixNativeDispatcher.c, I kept the dlsym > lookup for openat64, fstatat64 and fdopendir64, on non-BSD OSes (i.e. Linux > and AIX), and on AIX, respectively. This seems to me to be the safest choice, > as we do not need to know if a lookup of openat would yield a 32-bit or a > 64-bit version. (I frankly don't know, but I'm guessing it would yield the > 32-bit version.) - PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476232283 PR Review Comment: https://git.openjdk.org/jdk/pull/17538#discussion_r1476229335
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
On Fri, 2 Feb 2024 07:38:24 GMT, Doug Simon wrote: >> The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can >> transiently fail when run with `-Xcomp`. This happens when the compilation >> of methods on the worker threads interleaves with the 2 calls on the main >> thread to `mbean.getThreadCpuTime` to get the CPU time for all worker >> threads. >> >> The way the test is currently written, the worker threads are all usually >> waiting on a shared monitor when the 2 timings are taken. That is, in a >> successful run, the delta between the timings is always 0. When `-Xcomp` >> compilations kick in at the "wrong" time or take sufficiently long, the >> expected delta of 100 nanoseconds is easily exceeded. >> >> It does not make sense to run a timing test with such a small expected delta >> with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix date in header Yes, if you're idle, you should not have used any cpu time, that's what I think it's testing, and the 100 ms slack (DELTA) to account for slack in the accounting. If compilation makes a thread state Thread.State.WAITING then it would fool the check in waitUntilThreadBlocked(). Next time it causes trouble that check could be revisited... 8-) - PR Comment: https://git.openjdk.org/jdk/pull/17675#issuecomment-1923615761
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
On Fri, 2 Feb 2024 10:48:26 GMT, Kevin Walls wrote: > Or maybe they are not done the initial -Xcomp compile and > waitUntilThreadBlocked() is mistakenly thinking they are now idle... Yes, I believe this is the case. It's not really clear to me what the test is testing. As far as I can see, if the 2 timings are meant to be taken when the worker threads are idle, I would have thought the expected delta should be 0. - PR Comment: https://git.openjdk.org/jdk/pull/17675#issuecomment-1923578655
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
On Fri, 2 Feb 2024 07:38:24 GMT, Doug Simon wrote: >> The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can >> transiently fail when run with `-Xcomp`. This happens when the compilation >> of methods on the worker threads interleaves with the 2 calls on the main >> thread to `mbean.getThreadCpuTime` to get the CPU time for all worker >> threads. >> >> The way the test is currently written, the worker threads are all usually >> waiting on a shared monitor when the 2 timings are taken. That is, in a >> successful run, the delta between the timings is always 0. When `-Xcomp` >> compilations kick in at the "wrong" time or take sufficiently long, the >> expected delta of 100 nanoseconds is easily exceeded. >> >> It does not make sense to run a timing test with such a small expected delta >> with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix date in header It doesn't seem that critical to test this with -Xcomp But just checking: the threads are meant to be waiting, after we call waitUntilThreadBlocked(), so do they wake up and do some deopt or compilation work for some reason? Or maybe they are not done the initial -Xcomp compile and waitUntilThreadBlocked() is mistakenly thinking they are now idle... - PR Comment: https://git.openjdk.org/jdk/pull/17675#issuecomment-1923550642
Integrated: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range
On Thu, 1 Feb 2024 18:25:33 GMT, Doug Simon wrote: > The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can transiently > fail when run with `-Xcomp`. This happens when the compilation of methods on > the worker threads interleaves with the 2 calls on the main thread to > `mbean.getThreadCpuTime` to get the CPU time for all worker threads. > > The way the test is currently written, the worker threads are all usually > waiting on a shared monitor when the 2 timings are taken. That is, in a > successful run, the delta between the timings is always 0. When `-Xcomp` > compilations kick in at the "wrong" time or take sufficiently long, the > expected delta of 100 nanoseconds is easily exceeded. > > It does not make sense to run a timing test with such a small expected delta > with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. This pull request has now been integrated. Changeset: 91d8dac9 Author:Doug Simon URL: https://git.openjdk.org/jdk/commit/91d8dac9cff5689abcf2fc8950b15d284f933afd Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range Reviewed-by: dholmes, sspitsyn - PR: https://git.openjdk.org/jdk/pull/17675
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
On Fri, 2 Feb 2024 07:38:24 GMT, Doug Simon wrote: >> The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can >> transiently fail when run with `-Xcomp`. This happens when the compilation >> of methods on the worker threads interleaves with the 2 calls on the main >> thread to `mbean.getThreadCpuTime` to get the CPU time for all worker >> threads. >> >> The way the test is currently written, the worker threads are all usually >> waiting on a shared monitor when the 2 timings are taken. That is, in a >> successful run, the delta between the timings is always 0. When `-Xcomp` >> compilations kick in at the "wrong" time or take sufficiently long, the >> expected delta of 100 nanoseconds is easily exceeded. >> >> It does not make sense to run a timing test with such a small expected delta >> with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix date in header Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/17675#issuecomment-1923543404
Re: RFR: 8325137: com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java can fail in Xcomp with out of expected range [v2]
On Fri, 2 Feb 2024 07:38:24 GMT, Doug Simon wrote: >> The `com/sun/management/ThreadMXBean/ThreadCpuTimeArray.java` can >> transiently fail when run with `-Xcomp`. This happens when the compilation >> of methods on the worker threads interleaves with the 2 calls on the main >> thread to `mbean.getThreadCpuTime` to get the CPU time for all worker >> threads. >> >> The way the test is currently written, the worker threads are all usually >> waiting on a shared monitor when the 2 timings are taken. That is, in a >> successful run, the delta between the timings is always 0. When `-Xcomp` >> compilations kick in at the "wrong" time or take sufficiently long, the >> expected delta of 100 nanoseconds is easily exceeded. >> >> It does not make sense to run a timing test with such a small expected delta >> with `-Xcomp` so this PR simply ignores the test when `-Xcomp` is present. > > Doug Simon has updated the pull request incrementally with one additional > commit since the last revision: > > fix date in header Looks good. Thank you for fixing! - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17675#pullrequestreview-1858895296
Re: RFR: 8325055: Rename Injector.h [v2]
On Wed, 31 Jan 2024 15:15:16 GMT, Kim Barrett wrote: >> Please review this trivial change that renames the file >> test/hotspot/jtreg/vmTestbase/nsk/share/jvmti/Injector.h to Injector.hpp. >> >> Testing: mach5 tier1 > > Kim Barrett has updated the pull request incrementally with one additional > commit since the last revision: > > fix name in README Marked as reviewed by sspitsyn (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17656#pullrequestreview-1858886273
Re: RFR: 8323680: SA PointerFinder code can do a better job of leveraging existing code to determine if an address is in the TLAB [v2]
On Fri, 26 Jan 2024 21:20:04 GMT, Chris Plummer wrote:
>> In PointerFinder.java we have some code to determine if a pointer is in a
>> TLAB, but it only executes for the SerialGC. It should work for all GCs, so
>> I moved the code out of the SerialGC block.
>>
>> I also cleaned up the printing in PointerLocation. java a bit so when not
>> using verbose mode not as much info about the tlab address is printed. This
>> is consistent with other addresses, such as java stack addresses, which is
>> what I modeled this change on.
>>
>> It's hard to test this change since it is hard to consistently get an
>> address to be in the tlab. I wrote a little test program that just sits in a
>> loop doing allocations. I attached to it with clhsdb and ran the
>> threadcontext command, which does a fincpc on each register. About half the
>> time the main thread was suspended in a frame where some registers where
>> pointing into the tlab, and I confirmed this was the case for both SerialGC
>> and G1. Here's an example of one register with verbose off and verbose on:
>>
>> rsi: 0x8a5d4448: In TLAB for thread "main"
>> sun.jvm.hotspot.runtime.JavaThread@0x7ffa24029000
>>
>> rsi: 0x8a5d4448: In TLAB for thread ("main" #1 prio=5
>> tid=0x7ffa24029000 nid=25392 runnable [0x]
>>java.lang.Thread.State: RUNNABLE
>>JavaThread state: _thread_in_java
>> )
>> [0x8a5d4448,0x8ab724b8,0x8b0c0250,{0x8b0c0490})
>>
>> For testing I ran all tier1, tier2, and tier5 svc tests (still in progress)
>
> Chris Plummer has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Get rid of inTLAB field. Not needed
The fix looks good.
-
Marked as reviewed by sspitsyn (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17494#pullrequestreview-1858878750
Re: RFR: JDK-8318566: Heap walking functions should not use FilteredFieldStream [v3]
On Fri, 2 Feb 2024 02:49:13 GMT, Alex Menkov wrote: >> FilteredFieldStream used by heap walking functions to iterate through >> klass/superclasses/interfaces fields are known to have poor performance (see >> [JDK-8317692](https://bugs.openjdk.org/browse/JDK-8317692) for details). >> Heap walking API implementation is the last user of the klasses. >> The fix reworks iteration through klass/superclasses/interfaces fields and >> drops FilteredFieldStream-related code. >> Additionally removed/updated includes of reflectionUtils.hpp. >> >> Testing: >> - tier1..4; >> - test/hotspot/jtreg/vmTestbase/nsk/jvmti (contains tests for different >> heap walking functions); >> - new test from #17580 (now the test runs several times faster). > > Alex Menkov has updated the pull request incrementally with one additional > commit since the last revision: > > jcheck Looks good. There is one minor comment though. src/hotspot/share/prims/jvmtiTagMap.cpp line 499: > 497: } > 498: // update total_field_number for superclass > 499: total_field_number = start_index; Nit: The local variable name `total_field_number` is a little bit confusing because it is instantly decreased here (see also the line 490). I'm not sure what name to suggest though. Maybe the comment at line 498 needs an update to say this number is decreased here. - Marked as reviewed by sspitsyn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17661#pullrequestreview-1858613748 PR Review Comment: https://git.openjdk.org/jdk/pull/17661#discussion_r1475699883
