On Tue, 2 May 2023 10:10:32 GMT, Serguei Spitsyn <sspit...@openjdk.org> wrote:
>> Alex Menkov has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Added "no continuations" test case
>
> src/hotspot/share/prims/jvmtiTagMap.cpp line 2245:
>
>> 2243: bool is_top_frame;
>> 2244: int depth;
>> 2245: frame* last_entry_frame;
>
> The field names of a helper class are usually started with '_' symbol.
renamed all fields
> src/hotspot/share/prims/jvmtiTagMap.cpp line 2319:
>
>> 2317: }
>> 2318: }
>> 2319: }
>
> The fragments 2289-2303 and 2305-2319 are based on the `StackValueCollection`
> and look very similar.
> It can be worth to refactor these fragments into two function calls:
>
> bool report_stack_value_collection(jmethodID method, int idx_base,
> StackValueCollection* elems, jlocation bci) {
> for (int index = 0; index < exprs->size(); index++) {
> if (exprs->at(index)->type() == T_OBJECT) {
> oop obj = elems->obj_at(index)();
> if (obj == nullptr) {
> continue;
> }
> // stack reference
> if (!CallbackInvoker::report_stack_ref_root(thread_tag, tid, depth,
> method,
> bci, idx_base + index,
> obj)) {
> return false;
> }
> }
> }
> return true; // ???
>
> . . . . .
> jlocation bci = (jlocation)jvf->bci();
> StackValueCollection* locals = jvf->locals();
> if (!report_stack_value_collection(method, locals, 0 /* idx_base*/,
> bci)) {
> return false;
> }
> StackValueCollection* exprs = jvf->expressions();
> if (!report_stack_value_collection(method, exprs, locals->size(), bci))
> {
> return false;
> }
>
> Other complete fragments can be also implemented as separate functions:
> 2321-2328 (?), 2330-2351
refactored.
> src/hotspot/share/prims/jvmtiTagMap.cpp line 2796:
>
>> 2794: if (!java_thread->has_last_Java_frame()) {
>> 2795: // this may be only platform thread
>> 2796: assert(mounted_vt == nullptr, "must be");
>
> I'm not sure this assert is right.
> I think, a virtual thread may have an empty stack observable from a VM_op,
> for instance when it is in a process of being terminated.
> Though, it is not that easy to make this assert fired with a test case and
> prove this can happen.
> Another danger is that a virtual thread can be observed from a VM_op as in a
> VTMS (mount/unmount) transition. I need to think a little bit about possible
> consequences. Is it better to treat current thread identity as of a carrier
> thread in such a case?
removed the assert for safety.
I have no idea how vthread stack (frames on carrier thread and stack chunks)
can look like during VTMS transitions (and it's very hard to reproduce the case
by test)
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1184336378
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1184337458
PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1184335758
