RFR: 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing [v8]

[Erik Österlund]

On Tue, 29 Sep 2020 16:09:48 GMT, Robbin Ehn <rehn at 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.barrett at oracle.com) on
> [hotspot-dev](mailto:hotspot-dev at 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