On Wed, 10 May 2023 23:41:07 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> The fix updates JVMTI FollowReferences implementation to report references
>> from virtual threads:
>> - unmounted vthreads are detected, their stack references for
>> JVMTI_HEAP_REFERENCE_STACK_LOCAL/JVMTI_HEAP_REFERENCE_JNI_LOCAL;
>> - stacks of mounted vthreads are splitted into 2 parts (virtual thread stack
>> and carrier thread stack), references are reported with correct thread
>> id/class tag/object tags/frame depth;
>> - common code to handle stack frames are moved into separate class;
>>
>> Threads are reported as:
>> - platform threads: JVMTI_HEAP_REFERENCE_THREAD (as before);
>> - mounted vthreads (synthetic references, consider them as heap roots
>> because carrier threads are roots): JVMTI_HEAP_REFERENCE_OTHER;
>> - unmounted vthreads: not reported as heap roots.
>
> Alex Menkov has updated the pull request incrementally with one additional
> commit since the last revision:
>
> some refactoring
>
> added StackRefCollector::process_frames;
> used single RegisterMap instance;
> used RegisterMap::WalkContinuation::include for RegisterMap;
src/hotspot/share/prims/jvmtiTagMap.cpp line 2258:
> 2256:
> 2257: bool set_thread(oop o);
> 2258: // sets the thread and reports the reference to it with the specified
> kind.
Could I ask you to polish comments a little bit?
Let's use the following Consistent Comment Rule (CCR):
If comment is started with a capitol letter then it should be ended with dot.
Otherwise, it should not be ended with dot.
I'll mark with CCR other comments that are inconsistent with CCR.
src/hotspot/share/prims/jvmtiTagMap.cpp line 2262:
> 2260:
> 2261: bool do_frame(vframe* vf);
> 2262: // handles frames until vf->sender() is null.
CCR
src/hotspot/share/prims/jvmtiTagMap.cpp line 2290:
> 2288: continue;
> 2289: }
> 2290:
Unneeded empty line.
src/hotspot/share/prims/jvmtiTagMap.cpp line 2312:
> 2310: } else {
> 2311: if (_last_entry_frame != nullptr) {
> 2312: // JNI locals for the entry frame
CCR
src/hotspot/share/prims/jvmtiTagMap.cpp line 2328:
> 2326: javaVFrame* jvf = javaVFrame::cast(vf);
> 2327:
> 2328: // the jmethodID
It is unlikely this comment helps.
src/hotspot/share/prims/jvmtiTagMap.cpp line 2341:
> 2339: }
> 2340:
> 2341: // Follow oops from compiled nmethod
CCR
src/hotspot/share/prims/jvmtiTagMap.cpp line 2797:
> 2795: // Reports the thread as JVMTI_HEAP_REFERENCE_THREAD,
> 2796: // walks the stack of the thread, finds all references (locals
> 2797: // and JNI calls) and reports these as stack references
CCR
src/hotspot/share/prims/jvmtiTagMap.cpp line 2829:
> 2827: RegisterMap::WalkContinuation::include);
> 2828:
> 2829: // first handle mounted vthread (if any).
CCR
src/hotspot/share/prims/jvmtiTagMap.cpp line 2833:
> 2831: frame f = java_thread->last_frame();
> 2832: vframe* vf = vframe::new_vframe(&f, ®_map, java_thread);
> 2833: // report virtual thread as JVMTI_HEAP_REFERENCE_OTHER.
CCR
src/hotspot/share/prims/jvmtiTagMap.cpp line 2838:
> 2836: }
> 2837: // split virtual thread and carrier thread stacks by vthread entry
> ("enterSpecial") frame,
> 2838: // consider vthread entry frame as the last vthread stack frame.
CCR
src/hotspot/share/prims/jvmtiTagMap.cpp line 2901:
> 2899: StackRefCollector stack_collector(tag_map(), &blk, nullptr);
> 2900: // reference to the vthread is already reported.
> 2901: if (!stack_collector.set_thread(vt)) {
CCR
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190539577
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190539725
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190539996
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190540181
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190540801
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190540873
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190541100
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190541281
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190541349
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190541442
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1190541874
