On Wed, 3 May 2023 22:04:37 GMT, Alex Menkov <amen...@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