On Tue, 29 Sep 2020 16:09:48 GMT, Robbin Ehn <r...@openjdk.org> wrote:
>> Erik Österlund has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now >> contains 12 commits: >> - Review: Move barrier detach >> - Review: Remove assert that has outstayed its welcome >> - Merge branch 'master' into 8253180_conc_stack_scanning >> - Review: Albert CR2 and defensive programming >> - Review: StefanK CR 3 >> - Review: Per CR 1 >> - Merge branch 'master' into 8253180_conc_stack_scanning >> - Review: Albert CR 1 >> - Review: SteafanK CR 2 >> - Merge branch 'master' into 8253180_conc_stack_scanning >> - ... and 2 more: >> https://git.openjdk.java.net/jdk/compare/6bddeb70...2ffbd764 > > Marked as reviewed by rehn (Reviewer). > _Mailing list message from [Kim Barrett](mailto:kim.barr...@oracle.com) on > [hotspot-dev](mailto:hotspot-...@openjdk.java.net):_ > 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. Fixed. > ------------------------------------------------------------------------------ > 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. Coleen also had the same comment, and we agreed to file a follow-up RFE to clean that up. This also applies to all the existing parameters passed in. So I would still like to do that, but in a follow-up RFE. Said RFE will also make the parameter use in RegisterMap and vframeStream explicit. > ------------------------------------------------------------------------------ > 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. Fixed. > ------------------------------------------------------------------------------ Thanks for the review Kim! ------------- PR: https://git.openjdk.java.net/jdk/pull/296
