Re: RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Tue, 26 Sep 2023 12:47:42 GMT, Coleen Phillimore wrote: > This change makes WeakHandle and OopHandle release null out the obj pointer, > at the cost of making the release function non-const and some changes that > propagated from that. This enables ObjectMonitor code to test for null to > see if the obj was already released, and seems like the right thing to do. > See comments from related PR in the bug report. > Tested with tier1-4. Nulling out `_obj` seems quite reasonable. But I'm struggling to understand why we support `release` as a public API instead of having it handled by the destructor? One you have released a handle it is dangerous to try and use it so why keep it around instead of deleting it (and thus running the destructor)? Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15920#pullrequestreview-1645748643
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads
On Thu, 21 Sep 2023 20:06:13 GMT, Alex Menkov wrote: > This is subtask of JDK-8299426: Heap dump does not contain virtual Thread > stack references > The change: > - reorganize thread-related code/prepare it to use for unmounted vthreads: > - new ThreadDumper class caches stack frames, thread serial num, frame > serial number (trace serial number is calculated from thread serial); > ThreadDumper objects for all platform/carrier and mounted virtual threads > are cached instead of ThreadStackTrace objects (they are created during > HPROF_FRAME/HPROF_TRACE dumping, used lated for writing > HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL > subrecords); > - new helper class JavaStackRefDumper to dump references from threadf stack; > - separate track traces for mounted virtual threads: > - separate HPROF_FRAME/HPROF_TRACE records for mounted vthreads and carrier > threads; > - separate > HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL > subrecords; > - updated hprof parser test lib to collect data about threads > (HPROF_GC_ROOT_THREAD_OBJ subrecords) and corresponding stack traces and > stack references. > > Testing - tier1-tier3, new test > > Output of the test for VtreadInHeapDumpTarg$VthreadMounted thread > without the fix: > `thread 0x8101be90, 16 frames > - [0] VtreadInHeapDumpTarg$VthreadMounted.run()V (VtreadInHeapDump.java:127) > Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted > Java Local Reference: VtreadInHeapDumpTarg$VThreadMountedReferenced > - [1] java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V > (Thread.java:1583) > Java Local Reference: java.lang.VirtualThread > Java Local Reference: java.lang.Class > Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted > - [2] java.lang.VirtualThread.run(Ljava/lang/Runnable;)V > (VirtualThread.java:309) > Java Local Reference: java.lang.VirtualThread > Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted > Java Local Reference: java.lang.Class > - [3] java.lang.VirtualThread$VThreadContinuation$1.run()V > (VirtualThread.java:190) > Java Local Reference: java.lang.VirtualThread$VThreadContinuation$1 > - [4] jdk.internal.vm.Continuation.enter0()V (Continuation.java:320) > Java Local Reference: java.lang.VirtualThread$VThreadContinuation > - [5] jdk.internal.vm.Continuation.enter(Ljdk/internal/vm/Continuation;Z)V > (Continuation.java:312) > Java Local Reference: java.lang.VirtualThread$VThreadContinuation > - [6] > jdk.internal.vm.Continuation.enterSpecial(Ljdk/internal/vm/Continuatio... test/hotspot/jtreg/serviceability/jvmti/vthread/HeapDump/VtreadInHeapDump.java line 1: > 1: /* The file/class name is wrong Vtread instead of VThread - PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1338145621
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads
On Thu, 21 Sep 2023 20:06:13 GMT, Alex Menkov wrote:
> This is subtask of JDK-8299426: Heap dump does not contain virtual Thread
> stack references
> The change:
> - reorganize thread-related code/prepare it to use for unmounted vthreads:
> - new ThreadDumper class caches stack frames, thread serial num, frame
> serial number (trace serial number is calculated from thread serial);
> ThreadDumper objects for all platform/carrier and mounted virtual threads
> are cached instead of ThreadStackTrace objects (they are created during
> HPROF_FRAME/HPROF_TRACE dumping, used lated for writing
> HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL
> subrecords);
> - new helper class JavaStackRefDumper to dump references from threadf stack;
> - separate track traces for mounted virtual threads:
> - separate HPROF_FRAME/HPROF_TRACE records for mounted vthreads and carrier
> threads;
> - separate
> HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL
> subrecords;
> - updated hprof parser test lib to collect data about threads
> (HPROF_GC_ROOT_THREAD_OBJ subrecords) and corresponding stack traces and
> stack references.
>
> Testing - tier1-tier3, new test
>
> Output of the test for VtreadInHeapDumpTarg$VthreadMounted thread
> without the fix:
> `thread 0x8101be90, 16 frames
> - [0] VtreadInHeapDumpTarg$VthreadMounted.run()V (VtreadInHeapDump.java:127)
> Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted
> Java Local Reference: VtreadInHeapDumpTarg$VThreadMountedReferenced
> - [1] java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V
> (Thread.java:1583)
> Java Local Reference: java.lang.VirtualThread
> Java Local Reference: java.lang.Class
> Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted
> - [2] java.lang.VirtualThread.run(Ljava/lang/Runnable;)V
> (VirtualThread.java:309)
> Java Local Reference: java.lang.VirtualThread
> Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted
> Java Local Reference: java.lang.Class
> - [3] java.lang.VirtualThread$VThreadContinuation$1.run()V
> (VirtualThread.java:190)
> Java Local Reference: java.lang.VirtualThread$VThreadContinuation$1
> - [4] jdk.internal.vm.Continuation.enter0()V (Continuation.java:320)
> Java Local Reference: java.lang.VirtualThread$VThreadContinuation
> - [5] jdk.internal.vm.Continuation.enter(Ljdk/internal/vm/Continuation;Z)V
> (Continuation.java:312)
> Java Local Reference: java.lang.VirtualThread$VThreadContinuation
> - [6]
> jdk.internal.vm.Continuation.enterSpecial(Ljdk/internal/vm/Continuatio...
Is there anything to tell you which carrier thread is associated with which
mounted VThread?
test/lib/jdk/test/lib/hprof/model/Root.java line 145:
> 143:
> 144: public long getReferrerId() {
> 145: return refererId;
Spelling: `referrerId`
test/lib/jdk/test/lib/hprof/model/ThreadObject.java line 9:
> 7: * published by the Free Software Foundation. Oracle designates this
> 8: * particular file as subject to the "Classpath" exception as provided
> 9: * by Oracle in the LICENSE file that accompanied this code.
Test files do not use the CPE.
test/lib/jdk/test/lib/hprof/model/ThreadObject.java line 33:
> 31:
> 32: private final long id;// ID of the JavaThing we refer to
> 33: //private JavaHeapObject referer = null;
Remove this?
test/lib/jdk/test/lib/hprof/parser/HprofReader.java line 917:
> 915: System.out.println("WARNING: " + msg);
> 916: }
> 917: /*
Should this be deleted?
-
PR Comment: https://git.openjdk.org/jdk/pull/15869#issuecomment-1736839223
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1338149011
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1338149626
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1338149989
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1338150749
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads
On Wed, 27 Sep 2023 07:18:27 GMT, David Holmes wrote: > Is there anything to tell you which carrier thread is associated with which > mounted VThread? The carrier and virtual threads are distinct and the HPROF format doesn't have record types to support associations like this. However, your question does make me wonder how it behaves when running with +ShowCarrierFrames. - PR Comment: https://git.openjdk.org/jdk/pull/15869#issuecomment-1736848975
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v2]
> Similar issue [JDK-8303549](https://bugs.openjdk.org/browse/JDK-8303549) > where AttachCurrentThread is failing on AIX due t stack size issue. > Test cases: > runtime/jni/terminatedThread/TestTerminatedThread.java > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java > > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java > > vmTestbase/nsk/jvmti/SetJNIFunctionTable/setjniftab001/TestDescription.java > > Reported Issue : [JDK-8219652](https://bugs.openjdk.org/browse/JDK-8219652) Varada M has updated the pull request incrementally with two additional commits since the last revision: - AttachCurrentThread() failure solution - Revert "AttachCurrentThread() failure solution" This reverts commit b2648f02ba7e693eca99074849a1b503c0b9d30c. - Changes: - all: https://git.openjdk.org/jdk/pull/15924/files - new: https://git.openjdk.org/jdk/pull/15924/files/b2648f02..210eadfb Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=00-01 Stats: 186 lines in 6 files changed: 14 ins; 148 del; 24 mod Patch: https://git.openjdk.org/jdk/pull/15924.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15924/head:pull/15924 PR: https://git.openjdk.org/jdk/pull/15924
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v2]
On Tue, 26 Sep 2023 15:48:01 GMT, Chris Plummer wrote:
>> Varada M has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - AttachCurrentThread() failure solution
>> - Revert "AttachCurrentThread() failure solution"
>>
>>This reverts commit b2648f02ba7e693eca99074849a1b503c0b9d30c.
>
> test/hotspot/jtreg/ProblemList.txt line 155:
>
>> 153:
>> 154: vmTestbase/nsk/jvmti/AttachOnDemand/attach045/TestDescription.java
>> 8202971 generic-all
>> 155:
>> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java
>> 8219652 aix-ppc64
>
> It looks like you need to remove this entry also.
Is it like we should create separate PR to remove/add tests from ProblemList ?
> test/hotspot/jtreg/vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/ji06t001.cpp
> line 52:
>
>> 50: NSK_DISPLAY0("Detaching thread ...\n"); \
>> 51: return 0; \
>> 52: exit(-1); \
>
> The indent of the `return statement` is wrong. It's at the same level as the
> `exit()`, which means the `exit()` is never executed.
>
> This code now ignores the `status` argument, so it doesn't seem to be correct.
>
> I'm not sure why any of the changes here were necessary.
@plummercj I have applied changes to native_thread.cpp instead of changing in
every executables. Please review the change. It will be working fine on windows
as well.
Thank you
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1338219781
PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1338219919
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v2]
On Tue, 26 Sep 2023 22:38:21 GMT, David Holmes wrote: > You should be changing the thread creation logic in > ./nsk/share/native/native_thread.cpp for the nsk test changes. > > Thanks Thank you @dholmes-ora .I have applied changes there. Please review the code change - PR Comment: https://git.openjdk.org/jdk/pull/15924#issuecomment-1736920228
Integrated: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists
On Mon, 18 Sep 2023 09:54:18 GMT, Axel Boldt-Christmas wrote: > ObjectMonitorIterator fails to return the most resent monitor added. It start > with returning the `nextOM()` ObjectMonitor from the `_head` ObjectMonitor > but fails to ever return the `_head` ObjectMonitor. > The current implementation can also not handle that the `_head` is nullptr > (no monitors in the system) and returns a null ObjectMonitorIterator. Which > is interpreted as `monitor list not supported, too old hotspot VM`. > > Changed the iterator to keep return the current monitor (starts with `_head`) > and decoupled `_head == nullptr` from the question if ObjectMonitorIterator > is supported. > > Testing: > * Passes all `serviceability/sa` tests > * Passed tier 1-5 > * Passed GHA This pull request has now been integrated. Changeset: 50a7a04e Author:Axel Boldt-Christmas URL: https://git.openjdk.org/jdk/commit/50a7a04e9adef8d6e7adffb83b01d551e22cd910 Stats: 36 lines in 6 files changed: 8 ins; 10 del; 18 mod 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists Reviewed-by: cjplummer, dholmes - PR: https://git.openjdk.org/jdk/pull/15782
Re: RFR: 8316417: ObjectMonitorIterator does not return the most recent monitor and is incorrect if no monitors exists [v6]
On Thu, 21 Sep 2023 06:21:25 GMT, Axel Boldt-Christmas wrote: >> ObjectMonitorIterator fails to return the most resent monitor added. It >> start with returning the `nextOM()` ObjectMonitor from the `_head` >> ObjectMonitor but fails to ever return the `_head` ObjectMonitor. >> The current implementation can also not handle that the `_head` is nullptr >> (no monitors in the system) and returns a null ObjectMonitorIterator. Which >> is interpreted as `monitor list not supported, too old hotspot VM`. >> >> Changed the iterator to keep return the current monitor (starts with >> `_head`) and decoupled `_head == nullptr` from the question if >> ObjectMonitorIterator is supported. >> >> Testing: >> * Passes all `serviceability/sa` tests >> * Passed tier 1-5 >> * Passed GHA > > Axel Boldt-Christmas has updated the pull request incrementally with one > additional commit since the last revision: > > Update copyright year Thanks for the reviews. - PR Comment: https://git.openjdk.org/jdk/pull/15782#issuecomment-1736918541
Re: RFR: 8299915: Remove ArrayAllocatorMallocLimit and associated code [v4]
On Tue, 26 Sep 2023 21:08:36 GMT, Coleen Phillimore wrote: >> Afshin Zafari has updated the pull request incrementally with one additional >> commit since the last revision: >> >> MetaspaceSize and its lower bound is used. > > This looks good to me. Thank you @coleenp and @dholmes-ora for your reviews. - PR Comment: https://git.openjdk.org/jdk/pull/15859#issuecomment-1736934979
Integrated: 8299915: Remove ArrayAllocatorMallocLimit and associated code
On Thu, 21 Sep 2023 12:02:24 GMT, Afshin Zafari wrote:
> 1. `ArrayAllocatorMallocLimit` is removed. The test cases that tested it also
> are removed.
> 2. `AllocArrayAllocator` instances are replaced with `MallocArrayAllocator`.
> 3. The signature of `CHeapBitMap::free(ptr, size)` is kept as it is, since it
> is called in this way from `GrowableBitMap::resize`, where `T` can be also
> `ArenaBitMap` and `ResourceBitMap`. However, it uses
> `MallocArrayAllocator::free(ptr)` and ignores the `size`:
> ```C++
> void CHeapBitMap::free(bm_word_t* map, idx_t size_in_words) const {
> MallocArrayAllocator::free(map);
> }
>
> ### Test
> tiers1-4 passed on all platforms.
This pull request has now been integrated.
Changeset: 45a145e5
Author:Afshin Zafari
URL:
https://git.openjdk.org/jdk/commit/45a145e5bc3d3216bb03379896f66a3b719a06dc
Stats: 213 lines in 8 files changed: 2 ins; 202 del; 9 mod
8299915: Remove ArrayAllocatorMallocLimit and associated code
Reviewed-by: dholmes, coleenp
-
PR: https://git.openjdk.org/jdk/pull/15859
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v3]
> Similar issue [JDK-8303549](https://bugs.openjdk.org/browse/JDK-8303549) > where AttachCurrentThread is failing on AIX due t stack size issue. > Test cases: > runtime/jni/terminatedThread/TestTerminatedThread.java > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java > > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java > > vmTestbase/nsk/jvmti/SetJNIFunctionTable/setjniftab001/TestDescription.java > > Reported Issue : [JDK-8219652](https://bugs.openjdk.org/browse/JDK-8219652) Varada M has updated the pull request incrementally with one additional commit since the last revision: Added pthread attritubes to libterminatedThread.c - Changes: - all: https://git.openjdk.org/jdk/pull/15924/files - new: https://git.openjdk.org/jdk/pull/15924/files/210eadfb..51ffd3c2 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=01-02 Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15924.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15924/head:pull/15924 PR: https://git.openjdk.org/jdk/pull/15924
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v4]
> Similar issue [JDK-8303549](https://bugs.openjdk.org/browse/JDK-8303549) > where AttachCurrentThread is failing on AIX due t stack size issue. > Test cases: > runtime/jni/terminatedThread/TestTerminatedThread.java > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java > > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java > > vmTestbase/nsk/jvmti/SetJNIFunctionTable/setjniftab001/TestDescription.java > > Reported Issue : [JDK-8219652](https://bugs.openjdk.org/browse/JDK-8219652) Varada M has updated the pull request incrementally with one additional commit since the last revision: Added pthread attritubes to libterminatedThread.c - Changes: - all: https://git.openjdk.org/jdk/pull/15924/files - new: https://git.openjdk.org/jdk/pull/15924/files/51ffd3c2..1b4290ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15924.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15924/head:pull/15924 PR: https://git.openjdk.org/jdk/pull/15924
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v5]
On Tue, 26 Sep 2023 21:05:22 GMT, David Holmes wrote:
>> Afshin Zafari has updated the pull request with a new target base due to a
>> merge or a rebase. The pull request now contains five commits:
>>
>> - Merge branch 'master' into _8314502
>> - changed the `E` param of find methods to `const E&`.
>> - find_from_end and its caller are also updated.
>> - 8314502: Change the comparator taking version of GrowableArray::find to
>> be a template method
>> - 8314502: GrowableArray: Make find with comparator take template
>
> src/hotspot/share/gc/parallel/mutableNUMASpace.hpp line 1:
>
>> 1: /*
>
> This seems an unrelated change.
This change came after fixing a merge conflict.
In `mutableNUMASpace.cpp`, at lines 163, 182, 202 and 586 the `find` function
is called in this way:
int i = lgrp_spaces()->find(&lgrp_id, LGRPSpace::equals);
where `lgrp_id` is `int`. Therefore, the `LGRPSpace::equals` has to take an
`int*` in its first argument. The definition of `find` is:
int find(T* token, bool f(T*, const E&)) const {
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1338278635
Re: RFR: 8315966: Relativize initial_sp in interpreter frames [v3]
> Relativize initial_sp in interpreter frames. > > By changing the "initial_sp" (AKA "monitor_block_top" or "monitors" on > PowerPC) member in interpreter frames from being an absolute address into an > offset that is relative to the frame pointer, we don't need to change the > value as we freeze and thaw frames of virtual threads. This is since we might > freeze and thaw from and to different worker threads, so the absolute address > to locals might change, but the offset from the frame pointer will be > constant. > > This subtask only handles relativization of "initial_sp" and > "monitor_block_top" since it's the same slot in interpreter frames (roughly > the same as "monitors" on PowerPC). Relativization of other interpreter frame > members are handled in other subtasks to JDK-8289296. > > Tested tier1-tier7 on supported platforms. The rest was sanity tested using > Qemu. Fredrik Bredberg has updated the pull request incrementally with one additional commit since the last revision: Removed whitespace (RISC-V only). - Changes: - all: https://git.openjdk.org/jdk/pull/15815/files - new: https://git.openjdk.org/jdk/pull/15815/files/29b576fd..68d94c03 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15815&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15815&range=01-02 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/15815.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15815/head:pull/15815 PR: https://git.openjdk.org/jdk/pull/15815
Re: RFR: 8315966: Relativize initial_sp in interpreter frames [v3]
On Wed, 27 Sep 2023 09:07:23 GMT, Fredrik Bredberg wrote: >> Relativize initial_sp in interpreter frames. >> >> By changing the "initial_sp" (AKA "monitor_block_top" or "monitors" on >> PowerPC) member in interpreter frames from being an absolute address into an >> offset that is relative to the frame pointer, we don't need to change the >> value as we freeze and thaw frames of virtual threads. This is since we >> might freeze and thaw from and to different worker threads, so the absolute >> address to locals might change, but the offset from the frame pointer will >> be constant. >> >> This subtask only handles relativization of "initial_sp" and >> "monitor_block_top" since it's the same slot in interpreter frames (roughly >> the same as "monitors" on PowerPC). Relativization of other interpreter >> frame members are handled in other subtasks to JDK-8289296. >> >> Tested tier1-tier7 on supported platforms. The rest was sanity tested using >> Qemu. > > Fredrik Bredberg has updated the pull request incrementally with one > additional commit since the last revision: > > Removed whitespace (RISC-V only). Marked as reviewed by fyang (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15815#pullrequestreview-1645996807
Re: RFR: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization
On Tue, 26 Sep 2023 19:28:47 GMT, Chris Plummer wrote: >> Fine then. Thank you for the explanation. > > What was the cause of all the 0's in the output, and how did you get rid of > them so you could see the assert message? Thanks Leonid! On the \u content, I don't see that mess in the console (cygwin) if I make these asserts trigger in a local windows build and test. But I do see the \u... in the .jtr file created by jtreg. So a character encoding disagreement when the output is captured and saved..? The jtreg faq mentions this, https://openjdk.org/jtreg/faq.html but I haven't worked on it further. In logs/bug reports that have them, I just search and replace to remove them... - PR Review Comment: https://git.openjdk.org/jdk/pull/15750#discussion_r1338409202
Re: RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Tue, 26 Sep 2023 12:47:42 GMT, Coleen Phillimore wrote:
> This change makes WeakHandle and OopHandle release null out the obj pointer,
> at the cost of making the release function non-const and some changes that
> propagated from that. This enables ObjectMonitor code to test for null to
> see if the obj was already released, and seems like the right thing to do.
> See comments from related PR in the bug report.
> Tested with tier1-4.
OopHandles and WeakHandles don't have destructors (or copy constructors and
assignment operators). Places where we release the storage would have to be
reworked significantly to have this sort of usage model: eg, releasing a String
from the StringTable is done with a callback from the CHT
static void free_node(void* context, void* memory, Value const& value) {
value.release(StringTable::_oop_storage);
FreeHeap(memory);
StringTable::item_removed();
}
-
PR Comment: https://git.openjdk.org/jdk/pull/15920#issuecomment-1737281090
Re: RFR: 8315966: Relativize initial_sp in interpreter frames [v3]
On Wed, 27 Sep 2023 09:07:23 GMT, Fredrik Bredberg wrote: >> Relativize initial_sp in interpreter frames. >> >> By changing the "initial_sp" (AKA "monitor_block_top" or "monitors" on >> PowerPC) member in interpreter frames from being an absolute address into an >> offset that is relative to the frame pointer, we don't need to change the >> value as we freeze and thaw frames of virtual threads. This is since we >> might freeze and thaw from and to different worker threads, so the absolute >> address to locals might change, but the offset from the frame pointer will >> be constant. >> >> This subtask only handles relativization of "initial_sp" and >> "monitor_block_top" since it's the same slot in interpreter frames (roughly >> the same as "monitors" on PowerPC). Relativization of other interpreter >> frame members are handled in other subtasks to JDK-8289296. >> >> Tested tier1-tier7 on supported platforms. The rest was sanity tested using >> Qemu. > > Fredrik Bredberg has updated the pull request incrementally with one > additional commit since the last revision: > > Removed whitespace (RISC-V only). Thank you all. If no one else has anything to add, I'll integrate (as soon as I can convince a sponsor). - PR Comment: https://git.openjdk.org/jdk/pull/15815#issuecomment-1737349402
Integrated: 8315966: Relativize initial_sp in interpreter frames
On Tue, 19 Sep 2023 09:00:01 GMT, Fredrik Bredberg wrote: > Relativize initial_sp in interpreter frames. > > By changing the "initial_sp" (AKA "monitor_block_top" or "monitors" on > PowerPC) member in interpreter frames from being an absolute address into an > offset that is relative to the frame pointer, we don't need to change the > value as we freeze and thaw frames of virtual threads. This is since we might > freeze and thaw from and to different worker threads, so the absolute address > to locals might change, but the offset from the frame pointer will be > constant. > > This subtask only handles relativization of "initial_sp" and > "monitor_block_top" since it's the same slot in interpreter frames (roughly > the same as "monitors" on PowerPC). Relativization of other interpreter frame > members are handled in other subtasks to JDK-8289296. > > Tested tier1-tier7 on supported platforms. The rest was sanity tested using > Qemu. This pull request has now been integrated. Changeset: 347bd15e Author:Fredrik Bredberg Committer: Coleen Phillimore URL: https://git.openjdk.org/jdk/commit/347bd15e49f5632e16d0ae4dd7240a3648baf539 Stats: 202 lines in 30 files changed: 84 ins; 48 del; 70 mod 8315966: Relativize initial_sp in interpreter frames Reviewed-by: fyang, mdoerr, pchilanomate - PR: https://git.openjdk.org/jdk/pull/15815
Re: RFR: 8316401: sun/tools/jhsdb/JStackStressTest.java failed with "InternalError: We should have found a thread that owns the anonymous lock"
On Wed, 27 Sep 2023 04:17:57 GMT, David Holmes wrote: >> I wonder if we are in the middle of a GC, the object has been moved, and the >> lock stack object reference has been updated but the monitor reference has >> not (or vice versa). > > I think you are right @plummercj - I'm adding details to the JBS issue. I think given this, issuing a warning is the appropriate thing to do. The warning might want to mention that the failure is likely because we are in the middle of a GC. That way the user has some context as to why SA info is not reliable. - PR Review Comment: https://git.openjdk.org/jdk/pull/15907#discussion_r1338812930
Re: RFR: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization
On Thu, 14 Sep 2023 17:24:39 GMT, Kevin Walls wrote: > This assert happens rarely, but is seen in testing a few times. > > getCurrentQueryIndexForProcess comments that it can return -1, but it asserts > that the value is >=0 > > If we let it return -1 for failure as its comment documents, the caller can > handle the failure and not assert and end the JVM. > > Conversely, currentQueryIndexForProcess() clearly can return -1 on failure, > so add the comment like we already have in getCurrentQueryIndexForProcess(). > > This assert is not reproducing on demand, but with this change I've done 50+ > iterations of the test on windows-x64 and windows-x64-debug in mach5, and > hundreds locally. > > The test which has been seen to trigger the assert > "test/jdk/com/sun/management/OperatingSystemMXBean/GetProcessCpuLoad.java" > ...checks the range of the load value returned, and is happy enough if -1 is > the answer. Marked as reviewed by cjplummer (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/15750#pullrequestreview-1647041495
Integrated: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization
On Thu, 14 Sep 2023 17:24:39 GMT, Kevin Walls wrote: > This assert happens rarely, but is seen in testing a few times. > > getCurrentQueryIndexForProcess comments that it can return -1, but it asserts > that the value is >=0 > > If we let it return -1 for failure as its comment documents, the caller can > handle the failure and not assert and end the JVM. > > Conversely, currentQueryIndexForProcess() clearly can return -1 on failure, > so add the comment like we already have in getCurrentQueryIndexForProcess(). > > This assert is not reproducing on demand, but with this change I've done 50+ > iterations of the test on windows-x64 and windows-x64-debug in mach5, and > hundreds locally. > > The test which has been seen to trigger the assert > "test/jdk/com/sun/management/OperatingSystemMXBean/GetProcessCpuLoad.java" > ...checks the range of the load value returned, and is happy enough if -1 is > the answer. This pull request has now been integrated. Changeset: 5350fd61 Author:Kevin Walls URL: https://git.openjdk.org/jdk/commit/5350fd617390aaaedf8dd8821418c796cb1c38b3 Stats: 10 lines in 1 file changed: 2 ins; 2 del; 6 mod 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization Reviewed-by: lmesnik, cjplummer - PR: https://git.openjdk.org/jdk/pull/15750
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v4]
On Wed, 27 Sep 2023 08:13:21 GMT, Varada M wrote: >> test/hotspot/jtreg/ProblemList.txt line 155: >> >>> 153: >>> 154: vmTestbase/nsk/jvmti/AttachOnDemand/attach045/TestDescription.java >>> 8202971 generic-all >>> 155: >>> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java >>> 8219652 aix-ppc64 >> >> It looks like you need to remove this entry also. > > Is it like we should create separate PR to remove/add tests from ProblemList > or can I remove those from ProblemList in this PR itself. One of the test > "TestTerminatedThread.java" still fails so it should be under ProblemList > itself. > > Thank you Any test fixed by this PR should be removed from the problem list by this PR. If there are any tests problem listed with this CR that still fail after it is fixed, then they need to have a new CR filed for them, and the problem list updated to reference the new CR. That problem list updating can also be done in this PR. - PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1338828322
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v4]
On Wed, 27 Sep 2023 15:52:47 GMT, Chris Plummer wrote: >> Is it like we should create separate PR to remove/add tests from ProblemList >> or can I remove those from ProblemList in this PR itself. One of the test >> "TestTerminatedThread.java" still fails so it should be under ProblemList >> itself. >> >> Thank you > > Any test fixed by this PR should be removed from the problem list by this PR. > If there are any tests problem listed with this CR that still fail after it > is fixed, then they need to have a new CR filed for them, and the problem > list updated to reference the new CR. That problem list updating can also be > done in this PR. ...or you could file a new CR to fix just the tests that this PR is fixing (and update this PR to reference that CR). Then you would only need to remove problem list entries and not update any existing ones. - PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1338830501
Re: RFR: 8299560: Assertion failed: currentQueryIndex >= 0 && currentQueryIndex < numberOfJavaProcessesAtInitialization
On Wed, 27 Sep 2023 10:36:08 GMT, Kevin Walls wrote: >> What was the cause of all the 0's in the output, and how did you get rid of >> them so you could see the assert message? > > Thanks Leonid! > > On the \u content, I don't see that mess in the console (cygwin) if I > make these asserts trigger in a local windows build and test. > But I do see the \u... in the .jtr file created by jtreg. So a character > encoding disagreement when the output is captured and saved..? The jtreg faq > mentions this, https://openjdk.org/jtreg/faq.html but I haven't worked on it > further. > > In logs/bug reports that have them, I just search and replace to remove > them... Thanks Chris! - PR Review Comment: https://git.openjdk.org/jdk/pull/15750#discussion_r1338830889
Re: RFR: 8316401: sun/tools/jhsdb/JStackStressTest.java failed with "InternalError: We should have found a thread that owns the anonymous lock"
On Wed, 27 Sep 2023 15:42:51 GMT, Chris Plummer wrote: >> I think you are right @plummercj - I'm adding details to the JBS issue. > > I think given this, issuing a warning is the appropriate thing to do. The > warning might want to mention that the failure is likely because we are in > the middle of a GC. That way the user has some context as to why SA info is > not reliable. Could you advice how such a warning could look like? Does SA use a special stream for that? - PR Review Comment: https://git.openjdk.org/jdk/pull/15907#discussion_r1338928770
Re: RFR: 8316401: sun/tools/jhsdb/JStackStressTest.java failed with "InternalError: We should have found a thread that owns the anonymous lock"
On Wed, 27 Sep 2023 16:59:17 GMT, Roman Kennke wrote: >> I think given this, issuing a warning is the appropriate thing to do. The >> warning might want to mention that the failure is likely because we are in >> the middle of a GC. That way the user has some context as to why SA info is >> not reliable. > > Could you advice how such a warning could look like? Does SA use a special > stream for that? Most warnings go to System.err, but I think System.out is better in most cases so the warning appears in line with the rest of the related output. You could do something like: Warning: We failed to find a thread that owns an anonymous lock. This is likely due to the JVM currently running a GC. Locking information may not be accurate. - PR Review Comment: https://git.openjdk.org/jdk/pull/15907#discussion_r1338959696
Re: RFR: 8316401: sun/tools/jhsdb/JStackStressTest.java failed with "InternalError: We should have found a thread that owns the anonymous lock" [v2]
> The SA can run concurrently with Java threads, SA code that inspects locking > state should be able to deal with that. On the other hand, the particular > code is only used in printing routine and is not expected to be precise. When > resolving an anonymous owner, we may not find one, because Java threads may > already have moved on. Instead of crashing with a stacktrace, we should > gracefully return null here. > > Testing: > - [x] sun/tools/jhsdb/JStackStressTest.java > - [x] sun/tools/jhsdb > - [x] serviceability/sa Roman Kennke has updated the pull request incrementally with one additional commit since the last revision: Print warning when anonymous lock cannot be found - Changes: - all: https://git.openjdk.org/jdk/pull/15907/files - new: https://git.openjdk.org/jdk/pull/15907/files/18e46db6..4ddaf577 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15907&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15907&range=00-01 Stats: 2 lines in 1 file changed: 2 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/15907.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15907/head:pull/15907 PR: https://git.openjdk.org/jdk/pull/15907
Re: RFR: 8316401: sun/tools/jhsdb/JStackStressTest.java failed with "InternalError: We should have found a thread that owns the anonymous lock" [v2]
On Wed, 27 Sep 2023 17:54:51 GMT, Roman Kennke wrote: >> The SA can run concurrently with Java threads, SA code that inspects locking >> state should be able to deal with that. On the other hand, the particular >> code is only used in printing routine and is not expected to be precise. >> When resolving an anonymous owner, we may not find one, because Java threads >> may already have moved on. Instead of crashing with a stacktrace, we should >> gracefully return null here. >> >> Testing: >> - [x] sun/tools/jhsdb/JStackStressTest.java >> - [x] sun/tools/jhsdb >> - [x] serviceability/sa > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Print warning when anonymous lock cannot be found Looks good. Thanks! - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15907#pullrequestreview-1647405613
RFR: 8303773: Replace "main.wrapper" with "test.thread.factory" property in test code
The main.wrapper was the first name for jtreg test thread factory plugin. However, during integration of this feature in jtreg it was decided to use test.thread.factory name. So this fix just renames "main.wrapper" property to "test.thread.factory" so it is more compliant with jtreg naming. Also, it makes more sense for tests when it is used to create other then main threads in test. Testing: tier1-5. Verified that "main.wrapper" is not used in test sources anymore. I haven't rename DebugeeWrapperd and MainWrapper classes in JDI test frameworks because they are actually more main wrappers than thread factories. - Commit messages: - fixed tests Changes: https://git.openjdk.org/jdk/pull/15950/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15950&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303773 Stats: 63 lines in 21 files changed: 0 ins; 1 del; 62 mod Patch: https://git.openjdk.org/jdk/pull/15950.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15950/head:pull/15950 PR: https://git.openjdk.org/jdk/pull/15950
RFR: 8313631: SA: stack trace printed using "where" command does not show class name
Please review trivial fix to display class name in the output of "where" command of SA. Testing: hotspot_serviceability - Commit messages: - 8313631: SA: stack trace printed using "where" command does not show class name Changes: https://git.openjdk.org/jdk/pull/15952/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15952&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8313631 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15952.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15952/head:pull/15952 PR: https://git.openjdk.org/jdk/pull/15952
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability Can you add to the CR a copy of the `where` output after this fix? Just a short snippet equivalent to the broken output already in the CR. thanks. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738078911
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability Updated CR with the output of `where` command after the fix. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738087405
Re: RFR: 8309271: A way to align already compiled methods with compiler directives [v7]
> Compiler Control (https://openjdk.org/jeps/165) provides method-context > dependent control of the JVM compilers (C1 and C2). The active directive > stack is built from the directive files passed with the > `-XX:CompilerDirectivesFile` diagnostic command-line option and the > Compiler.add_directives diagnostic command. It is also possible to clear all > directives or remove the top from the stack. > > A matching directive will be applied at method compilation time when such > compilation is started. If directives are added or changed, but compilation > does not start, then the state of compiled methods doesn't correspond to the > rules. This is not an error, and it happens in long running applications when > directives are added or removed after compilation of methods that could be > matched. For example, the user decides that C2 compilation needs to be > disabled for some method due to a compiler bug, issues such a directive but > this does not affect the application behavior. In such case, the target > application needs to be restarted, and such an operation can have high costs > and risks. Another goal is testing/debugging compilers. > > It would be convenient to optionally reconcile at least existing matching > nmethods to the current stack of compiler directives (so bypass inlined > methods). > > Natural way to eliminate the discrepancy between the result of compilation > and the broken rule is to discard the compilation result, i.e. > deoptimization. Prior to that we can try to re-compile the method letting > compile broker to perform it taking new directives stack into account. > Re-compilation helps to prevent hot methods from execution in the interpreter. > > A new flag `-r` has beed introduced for some directives related to compile > commands: `Compiler.add_directives`, `Compiler.remove_directives`, > `Compiler.clear_directives`. The default behavior has not changed (no flag). > If the new flag is present, the command scans already compiled methods and > puts methods that have any active non-default matching compiler directives to > re-compilation if possible, otherwise marks them for deoptimization. There is > currently no distinction which directives are found. In particular, this > means that if there are rules for inlining into some method, it will be > refreshed. On the other hand, if there are rules for a method and it was > inlined, top-level methods won't be refreshed, but this can be achieved by > having rules for them. > > In addition, a new diagnostic command `Compiler.replace_directives`, has been > added for ... Dmitry Chuyko has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 25 commits: - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - jcheck - Unnecessary import - force_update->refresh - Merge branch 'openjdk:master' into compiler-directives-force-update - Use only top directive for add/remove; better mutex rank definition; texts - Merge branch 'openjdk:master' into compiler-directives-force-update - Merge branch 'openjdk:master' into compiler-directives-force-update - Safe handling of non-java methods - ... and 15 more: https://git.openjdk.org/jdk/compare/750da001...e451f509 - Changes: https://git.openjdk.org/jdk/pull/14111/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=14111&range=06 Stats: 372 lines in 15 files changed: 339 ins; 3 del; 30 mod Patch: https://git.openjdk.org/jdk/pull/14111.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/14111/head:pull/14111 PR: https://git.openjdk.org/jdk/pull/14111
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability Looks good. Thanks! - Marked as reviewed by cjplummer (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15952#pullrequestreview-1647654443
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads [v2]
> This is subtask of JDK-8299426: Heap dump does not contain virtual Thread > stack references > The change: > - reorganize thread-related code/prepare it to use for unmounted vthreads: > - new ThreadDumper class caches stack frames, thread serial num, frame > serial number (trace serial number is calculated from thread serial); > ThreadDumper objects for all platform/carrier and mounted virtual threads > are cached instead of ThreadStackTrace objects (they are created during > HPROF_FRAME/HPROF_TRACE dumping, used lated for writing > HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL > subrecords); > - new helper class JavaStackRefDumper to dump references from threadf stack; > - separate track traces for mounted virtual threads: > - separate HPROF_FRAME/HPROF_TRACE records for mounted vthreads and carrier > threads; > - separate > HPROF_GC_ROOT_THREAD_OBJ/HPROF_GC_ROOT_JAVA_FRAME/HPROF_GC_ROOT_JNI_LOCAL > subrecords; > - updated hprof parser test lib to collect data about threads > (HPROF_GC_ROOT_THREAD_OBJ subrecords) and corresponding stack traces and > stack references. > > Testing - tier1-tier3, new test > > Output of the test for VtreadInHeapDumpTarg$VthreadMounted thread > without the fix: > `thread 0x8101be90, 16 frames > - [0] VtreadInHeapDumpTarg$VthreadMounted.run()V (VtreadInHeapDump.java:127) > Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted > Java Local Reference: VtreadInHeapDumpTarg$VThreadMountedReferenced > - [1] java.lang.Thread.runWith(Ljava/lang/Object;Ljava/lang/Runnable;)V > (Thread.java:1583) > Java Local Reference: java.lang.VirtualThread > Java Local Reference: java.lang.Class > Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted > - [2] java.lang.VirtualThread.run(Ljava/lang/Runnable;)V > (VirtualThread.java:309) > Java Local Reference: java.lang.VirtualThread > Java Local Reference: VtreadInHeapDumpTarg$VthreadMounted > Java Local Reference: java.lang.Class > - [3] java.lang.VirtualThread$VThreadContinuation$1.run()V > (VirtualThread.java:190) > Java Local Reference: java.lang.VirtualThread$VThreadContinuation$1 > - [4] jdk.internal.vm.Continuation.enter0()V (Continuation.java:320) > Java Local Reference: java.lang.VirtualThread$VThreadContinuation > - [5] jdk.internal.vm.Continuation.enter(Ljdk/internal/vm/Continuation;Z)V > (Continuation.java:312) > Java Local Reference: java.lang.VirtualThread$VThreadContinuation > - [6] > jdk.internal.vm.Continuation.enterSpecial(Ljdk/internal/vm/Continuatio... Alex Menkov has updated the pull request incrementally with one additional commit since the last revision: David's feedback - Changes: - all: https://git.openjdk.org/jdk/pull/15869/files - new: https://git.openjdk.org/jdk/pull/15869/files/f2d3c374..2de09397 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15869&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15869&range=00-01 Stats: 26 lines in 3 files changed: 0 ins; 17 del; 9 mod Patch: https://git.openjdk.org/jdk/pull/15869.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15869/head:pull/15869 PR: https://git.openjdk.org/jdk/pull/15869
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads [v2]
On Wed, 27 Sep 2023 07:11:26 GMT, David Holmes wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> David's feedback
>
> test/hotspot/jtreg/serviceability/jvmti/vthread/HeapDump/VtreadInHeapDump.java
> line 1:
>
>> 1: /*
>
> The file/class name is wrong Vtread instead of VThread
Fixed.
> test/lib/jdk/test/lib/hprof/model/Root.java line 145:
>
>> 143:
>> 144: public long getReferrerId() {
>> 145: return refererId;
>
> Spelling: `referrerId`
This is existing field in the class.
There are a number (45) of misspelled "referer" in the lib, including some
public methods (like getReferer() in line 140).
If you think it makes sense to fix the, I can file a separate CR for this.
> test/lib/jdk/test/lib/hprof/model/ThreadObject.java line 9:
>
>> 7: * published by the Free Software Foundation. Oracle designates this
>> 8: * particular file as subject to the "Classpath" exception as provided
>> 9: * by Oracle in the LICENSE file that accompanied this code.
>
> Test files do not use the CPE.
Fixed.
> test/lib/jdk/test/lib/hprof/model/ThreadObject.java line 33:
>
>> 31:
>> 32: private final long id;// ID of the JavaThing we refer to
>> 33: //private JavaHeapObject referer = null;
>
> Remove this?
Done.
> test/lib/jdk/test/lib/hprof/parser/HprofReader.java line 917:
>
>> 915: System.out.println("WARNING: " + msg);
>> 916: }
>> 917: /*
>
> Should this be deleted?
Yes. Done.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1339290812
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1339261751
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1339291528
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1339291698
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1339291988
Re: RFR: 8303773: Replace "main.wrapper" with "test.thread.factory" property in test code
On Wed, 27 Sep 2023 20:23:03 GMT, Leonid Mesnik wrote:
> The main.wrapper was the first name for jtreg test thread factory plugin.
> However, during integration of this feature in jtreg it was decided to use
> test.thread.factory name. So this fix just renames "main.wrapper" property to
> "test.thread.factory" so it is more compliant with jtreg naming. Also, it
> makes more sense for tests when it is used to create other then main threads
> in test.
> Testing: tier1-5.
> Verified that "main.wrapper" is not used in test sources anymore.
>
> I haven't rename DebugeeWrapperd and MainWrapper classes in JDI test
> frameworks because they are actually more main wrappers than thread factories.
test/hotspot/jtreg/vmTestbase/nsk/share/runner/RunParams.java line 228:
> 226: }
> 227: // Allow to force using vthreads using wrapper property
> 228: if(System.getProperty("test.thread.factory") != null &&
> System.getProperty("test.thread.factory").equals("Virtual")) {
I think there are better examples of how to do this in the diffs above. Maybe a
good time to clean it up.
test/lib/jdk/test/lib/process/ProcessTools.java line 390:
> 388:
> 389: /*
> 390: Convert arguments for tests running with virtual threads test
> thread factory
Add a period at the end of the first sentence.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15950#discussion_r1339299467
PR Review Comment: https://git.openjdk.org/jdk/pull/15950#discussion_r1339300522
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads
On Wed, 27 Sep 2023 07:25:50 GMT, Alan Bateman wrote: > Is there anything to tell you which carrier thread is associated with which > mounted VThread? No (at least for now). I'm not sure if we need this information in heap dump (and if we need a way to differentiate platform(carrier)/virtual threads). If we need it, we can introduce new record type ((outside of HPROF_HEAP_DUMP record) for this later (new record type should not break existing hprof readers/parsers) - PR Comment: https://git.openjdk.org/jdk/pull/15869#issuecomment-1738203784
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 21:07:18 GMT, Chris Plummer wrote: >> Please review trivial fix to display class name in the output of "where" >> command of SA. >> >> Testing: hotspot_serviceability > > Can you add to the CR a copy of the `where` output after this fix? Just a > short snippet equivalent to the broken output already in the CR. thanks. thanks @plummercj for reviewing it. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738215616
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 21:07:18 GMT, Chris Plummer wrote: >> Please review trivial fix to display class name in the output of "where" >> command of SA. >> >> Testing: hotspot_serviceability > > Can you add to the CR a copy of the `where` output after this fix? Just a > short snippet equivalent to the broken output already in the CR. thanks. @plummercj for some reason the bot refuses to recognize my committer status in jdk. As per https://openjdk.org/census#jdk I see my name in the Committers list. Any idea what needs to be done here? - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738219081
Re: RFR: 8316401: sun/tools/jhsdb/JStackStressTest.java failed with "InternalError: We should have found a thread that owns the anonymous lock" [v2]
On Wed, 27 Sep 2023 17:54:51 GMT, Roman Kennke wrote: >> The SA can run concurrently with Java threads, SA code that inspects locking >> state should be able to deal with that. On the other hand, the particular >> code is only used in printing routine and is not expected to be precise. >> When resolving an anonymous owner, we may not find one, because Java threads >> may already have moved on. Instead of crashing with a stacktrace, we should >> gracefully return null here. >> >> Testing: >> - [x] sun/tools/jhsdb/JStackStressTest.java >> - [x] sun/tools/jhsdb >> - [x] serviceability/sa > > Roman Kennke has updated the pull request incrementally with one additional > commit since the last revision: > > Print warning when anonymous lock cannot be found Changes requested by dholmes (Reviewer). src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/runtime/Threads.java line 245: > 243: } > 244: // We should have found the owner. However, the code can > run concurrently with > 245: // Java code and locking state can change at any time. > This code is not The second sentence is not accurate as things are not running concurrently and cannot change state at any time. Suggestion: // We should have found the owner, however, as the VM could be in any state, including the middle // of performing GC, it is not always possible to do so. Just return null if we can't locate it. - PR Review: https://git.openjdk.org/jdk/pull/15907#pullrequestreview-1647736396 PR Review Comment: https://git.openjdk.org/jdk/pull/15907#discussion_r1339374587
Re: RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Wed, 27 Sep 2023 12:19:31 GMT, Coleen Phillimore wrote: > OopHandles and WeakHandles don't have destructors Hmmm okay - it seems fragile to have a psuedo-destructor in `release()`. - PR Comment: https://git.openjdk.org/jdk/pull/15920#issuecomment-1738324423
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability https://wiki.openjdk.org/display/SKARA#Skara-AssociatingyourGitHubaccountandyourOpenJDKusername - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738329734
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 23:04:22 GMT, Ashutosh Mehra wrote: >> Can you add to the CR a copy of the `where` output after this fix? Just a >> short snippet equivalent to the broken output already in the CR. thanks. > > @plummercj for some reason the bot refuses to recognize my committer status > in jdk. As per https://openjdk.org/census#jdk I see my name in the Committers > list. Any idea what needs to be done here? @ashu-mehra I suspect you need to link your github user name to your OpenJDK user name. You will need to file an issue like this: https://bugs.openjdk.org/browse/SKARA-1887 - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738328411
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability Has the change been tested in the HSDB GUI as requested in the JBS issue? - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738331406
RFR: JDK-8316778: test hprof lib: invalid array element type from JavaValueArray.elementSize
The change fixes 2 issues in hprof test library.
The issue were discovered during test development (logging values of dumped
heap objects).
- JavaValueArray.elementSize cannot determine size of the array elements and
throws RuntimeException: invalid array element type
- JavaObject.toString() method for dumped String objects tries to construct
string value of the object from "value" field assuming "value" field is array
of char. Actually it's byte array, so toString return array values like "{65,
66, 67}" (with fixed JavaValueArray.elementSize)
-
Commit messages:
- jcheck
- hprof lib fix
Changes: https://git.openjdk.org/jdk/pull/15953/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=15953&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8316778
Stats: 195 lines in 3 files changed: 184 ins; 0 del; 11 mod
Patch: https://git.openjdk.org/jdk/pull/15953.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/15953/head:pull/15953
PR: https://git.openjdk.org/jdk/pull/15953
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Thu, 28 Sep 2023 01:57:59 GMT, David Holmes wrote: >> Please review trivial fix to display class name in the output of "where" >> command of SA. >> >> Testing: hotspot_serviceability > > Has the change been tested in the HSDB GUI as requested in the JBS issue? @dholmes-ora thanks for pointing out the issue. I have created one - https://bugs.openjdk.org/browse/SKARA-2046 > Has the change been tested in the HSDB GUI as requested in the JBS issue? Yes, I have checked hsdb as well. It shows same contents as the `where` command. - PR Comment: https://git.openjdk.org/jdk/pull/15952#issuecomment-1738335000
Re: RFR: 8314502: Change the comparator taking version of GrowableArray::find to be a template method [v5]
On Wed, 27 Sep 2023 08:48:27 GMT, Afshin Zafari wrote:
>> src/hotspot/share/gc/parallel/mutableNUMASpace.hpp line 1:
>>
>>> 1: /*
>>
>> This seems an unrelated change.
>
> This change came after fixing a merge conflict.
> In `mutableNUMASpace.cpp`, at lines 163, 182, 202 and 586 the `find` function
> is called in this way:
>
> int i = lgrp_spaces()->find(&lgrp_id, LGRPSpace::equals);
>
> where `lgrp_id` is `int`. Therefore, the `LGRPSpace::equals` has to take an
> `int*` in its first argument. The definition of `find` is:
>
> int find(T* token, bool f(T*, const E&)) const {
After JDK-8316115 `lgrp_id` is `uint`.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15418#discussion_r1339427450
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads [v2]
On Wed, 27 Sep 2023 21:51:38 GMT, Alex Menkov wrote:
>> test/lib/jdk/test/lib/hprof/model/Root.java line 145:
>>
>>> 143:
>>> 144: public long getReferrerId() {
>>> 145: return refererId;
>>
>> Spelling: `referrerId`
>
> This is existing field in the class.
> There are a number (45) of misspelled "referer" in the lib, including some
> public methods (like getReferer() in line 140).
> If you think it makes sense to fix the, I can file a separate CR for this.
Ugghh! Line 140 was not visible by default. It doesn't look good to add a new
method called `getReferrerId` when the other usages are `referer`. Lets go for
consistency in this PR and fix them all in a separate one. Thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15869#discussion_r1339431813
Integrated: 8316658: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java fails intermittently
On Fri, 22 Sep 2023 08:36:10 GMT, Jean-Philippe Bempel wrote: > increase Metaspace size and loop count to avoid OOME in nominal case This pull request has now been integrated. Changeset: 84390dd0 Author:Jean-Philippe Bempel Committer: David Holmes URL: https://git.openjdk.org/jdk/commit/84390dd0639e29ddb792964cca9ebf79e29cfcad Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod 8316658: serviceability/jvmti/RedefineClasses/RedefineLeakThrowable.java fails intermittently Reviewed-by: coleenp, dholmes - PR: https://git.openjdk.org/jdk/pull/15882
Re: RFR: JDK-8316691: Heap dump: separate stack traces for mounted virtual threads
On Wed, 27 Sep 2023 07:25:50 GMT, Alan Bateman wrote: > > Is there anything to tell you which carrier thread is associated with which > > mounted VThread? > > The carrier and virtual threads are distinct and the HPROF format doesn't > have record types to support associations like this. However, your question > does make me wonder how it behaves when running with +ShowCarrierFrames. IFAIU +ShowCarrierFrames does not makes sense for "general" heap dump (like generated by "jcmd GC.heap_dump"). We can handle it for HeapDumpOnOutOfMemoryError case (when we add fake frame with OOME ctor), but I don't think it'd add a value for diagnostics. - PR Comment: https://git.openjdk.org/jdk/pull/15869#issuecomment-1738362115
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v4]
On Wed, 27 Sep 2023 08:40:48 GMT, Varada M wrote: >> Similar issue [JDK-8303549](https://bugs.openjdk.org/browse/JDK-8303549) >> where AttachCurrentThread is failing on AIX due t stack size issue. >> Test cases: >> runtime/jni/terminatedThread/TestTerminatedThread.java >> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java >> >> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java >> >> vmTestbase/nsk/jvmti/SetJNIFunctionTable/setjniftab001/TestDescription.java >> >> Reported Issue : [JDK-8219652](https://bugs.openjdk.org/browse/JDK-8219652) > > Varada M has updated the pull request incrementally with one additional > commit since the last revision: > > Added pthread attritubes to libterminatedThread.c That is much cleaner and simpler - thanks. A couple of minor nits, plus the ProblemList fixing that Chris mentioned. test/hotspot/jtreg/vmTestbase/nsk/share/native/native_thread.cpp line 132: > 130: #else // !windows & !sun > 131: pthread_attr_t attr; > 132: pthread_attr_init(&attr); You need a corresponding `pthread_attr_destroy` below. test/hotspot/jtreg/vmTestbase/nsk/share/native/native_thread.cpp line 135: > 133: size_t stack_size = 0x10; > 134: pthread_attr_setstacksize(&attr, stack_size); > 135: int result = pthread_create(&(thread->id),&attr,procedure,thread); As you are changing this line can you add spaces after the commas please. - Changes requested by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15924#pullrequestreview-1647869605 PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1339511737 PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1339511485
Re: RFR: 8313631: SA: stack trace printed using "where" command does not show class name
On Wed, 27 Sep 2023 20:45:00 GMT, Ashutosh Mehra wrote: > Please review trivial fix to display class name in the output of "where" > command of SA. > > Testing: hotspot_serviceability Okay looks reasonable then. Thanks - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15952#pullrequestreview-1647874631
Re: RFR: 8309599: WeakHandle and OopHandle release should clear obj pointer
On Tue, 26 Sep 2023 12:47:42 GMT, Coleen Phillimore wrote: > This change makes WeakHandle and OopHandle release null out the obj pointer, > at the cost of making the release function non-const and some changes that > propagated from that. This enables ObjectMonitor code to test for null to > see if the obj was already released, and seems like the right thing to do. > See comments from related PR in the bug report. > Tested with tier1-4. Looks good. - Marked as reviewed by kbarrett (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15920#pullrequestreview-1647918875
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v5]
> Similar issue [JDK-8303549](https://bugs.openjdk.org/browse/JDK-8303549) > where AttachCurrentThread is failing on AIX due t stack size issue. > Test cases: > runtime/jni/terminatedThread/TestTerminatedThread.java > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java > > vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java > > vmTestbase/nsk/jvmti/SetJNIFunctionTable/setjniftab001/TestDescription.java > > Reported Issue : [JDK-8219652](https://bugs.openjdk.org/browse/JDK-8219652) Varada M has updated the pull request incrementally with one additional commit since the last revision: JDK-8219652: [aix] Tests failing with JNI attach problems. - Changes: - all: https://git.openjdk.org/jdk/pull/15924/files - new: https://git.openjdk.org/jdk/pull/15924/files/1b4290ed..4c88e5e8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15924&range=03-04 Stats: 6 lines in 2 files changed: 1 ins; 4 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/15924.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15924/head:pull/15924 PR: https://git.openjdk.org/jdk/pull/15924
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v4]
On Thu, 28 Sep 2023 04:30:16 GMT, David Holmes wrote: >> Varada M has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Added pthread attritubes to libterminatedThread.c > > test/hotspot/jtreg/vmTestbase/nsk/share/native/native_thread.cpp line 132: > >> 130: #else // !windows & !sun >> 131: pthread_attr_t attr; >> 132: pthread_attr_init(&attr); > > You need a corresponding `pthread_attr_destroy` below. Thank you @dholmes-ora. I have made the suggested the changes. - PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1339575167
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v5]
On Wed, 27 Sep 2023 15:54:15 GMT, Chris Plummer wrote: >> Any test fixed by this PR should be removed from the problem list by this >> PR. If there are any tests problem listed with this CR that still fail after >> it is fixed, then they need to have a new CR filed for them, and the problem >> list updated to reference the new CR. That problem list updating can also be >> done in this PR. > > ...or you could file a new CR to fix just the tests that this PR is fixing > (and update this PR to reference that CR). Then you would only need to > remove problem list entries and not update any existing ones. Thank you @plummercj . I have removed these tests in this PR itself. - PR Review Comment: https://git.openjdk.org/jdk/pull/15924#discussion_r1339582562
Re: RFR: JDK-8219652: [aix] Tests failing with JNI attach problems. [v5]
On Thu, 28 Sep 2023 06:11:52 GMT, Varada M wrote: >> Similar issue [JDK-8303549](https://bugs.openjdk.org/browse/JDK-8303549) >> where AttachCurrentThread is failing on AIX due t stack size issue. >> Test cases: >> runtime/jni/terminatedThread/TestTerminatedThread.java >> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI05/ji05t001/TestDescription.java >> >> vmTestbase/nsk/jvmti/scenarios/jni_interception/JI06/ji06t001/TestDescription.java >> >> vmTestbase/nsk/jvmti/SetJNIFunctionTable/setjniftab001/TestDescription.java >> >> Reported Issue : [JDK-8219652](https://bugs.openjdk.org/browse/JDK-8219652) > > Varada M has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8219652: [aix] Tests failing with JNI attach problems. That all looks fine to me. Thanks. - Marked as reviewed by dholmes (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/15924#pullrequestreview-1648003442
