RFR: 8299414: JVMTI FollowReferences should support references from VirtualThread stack [v9]

[Serguei Spitsyn]

On Wed, 3 May 2023 22:04:37 GMT, Alex Menkov <amenkov at openjdk.org> wrote:

>> 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.

It'd be nice to do even more factoring + renaming.
The lines 2326-2345 can be refactored to a function:

  bool StackRootCollector::report_native_frame_refs(jmethodID method) {
    _blk->set_context(_thread_tag, _tid, _depth, method);
    if (_is_top_frame) {
      // JNI locals for the top frame.
      assert(_java_thread != nullptr, "sanity");
      _java_thread->active_handles()->oops_do(_blk);
      if (_blk->stopped()) {
        return false;
      }
    } else {
      if (_last_entry_frame != nullptr) {
        // JNI locals for the entry frame
        assert(_last_entry_frame->is_entry_frame(), "checking");
        _last_entry_frame->entry_frame_call_wrapper()->handles()->oops_do(_blk);
        if (_blk->stopped()) {
          return false;
        }
      }
    }
    return true;
  }


The function `report_stack_refs` can be renamed to `report_java_frame_refs`
to make function name more consistent.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/13254#discussion_r1184463655