> On Sep 29, 2020, at 11:03 AM, Erik Österlund <eosterl...@openjdk.java.net>
> wrote:
>
> Changes: https://git.openjdk.java.net/jdk/pull/296/files
> Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=296&range=07
> Stats: 2705 lines in 129 files changed: 2137 ins; 310 del; 258 mod
> Patch: https://git.openjdk.java.net/jdk/pull/296.diff
> Fetch: git fetch https://git.openjdk.java.net/jdk pull/296/head:pull/296
>
> PR: https://git.openjdk.java.net/jdk/pull/296
I've only looked at scattered pieces, but what I've looked at seemed to be
in good shape. Only a few minor comments.
------------------------------------------------------------------------------
src/hotspot/share/runtime/frame.cpp
456 // for(StackFrameStream fst(thread); !fst.is_done(); fst.next()) {
Needs to be updated for the new constructor arguments. Just in general, the
class documentation seems to need some updating for this change.
------------------------------------------------------------------------------
src/hotspot/share/runtime/frame.cpp
466 StackFrameStream(JavaThread *thread, bool update, bool process_frames);
Something to consider is that bool parameters like that, especially when
there are multiple, are error prone. An alternative is distinct enums, which
likely also obviates the need for comments in calls.
------------------------------------------------------------------------------
src/hotspot/share/runtime/thread.hpp
956 void set_processed_thread(Thread *thread) { _processed_thread = thread; }
I think this should assert that either _processed_thread or thread are NULL.
Or maybe the RememberProcessedThread constructor should be asserting that
_cur_thr->processed_thread() is NULL.
------------------------------------------------------------------------------
