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

[Erik Österlund]

On Thu, 24 Sep 2020 12:45:00 GMT, Coleen Phillimore <coleenp 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 seven commits:
>>  - 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
>>  - Review: SteafanK CR 1
>>  - 8253180: ZGC: Implementation of JEP 376: ZGC: Concurrent Thread-Stack Processing
>
> src/hotspot/share/runtime/vframe.hpp line 340:
> 
>> 338:  public:
>> 339:   // Constructors
>> 340:   vframeStream(JavaThread* thread, bool stop_at_java_call_stub = false, bool process_frames = true);
> 
> I was wondering if you supply arguments to all callers of vframeStream now?  It seems like having default parameters is
> an invitation to errors. Same with RegMap.

I have tried to have explicit arguments as much as possible. But RegMap and vframeStream seemed to clutter my whole
patch if I made them explicit (they are already implicit today). I agree though that having them explicit would be
better. But what do you think about doing that in a follow-up cleanup RFE instead? It might reduce the amount of
scrolling needed to review this patch. The defaults are on the safe conservative side, so not supplying the extra
parameter should not harm you.

> src/hotspot/share/classfile/javaClasses.cpp line 2440:
> 
>> 2438:   // trace as utilizing vframe.
>> 2439: #ifdef ASSERT
>> 2440:   vframeStream st(thread, false /* stop_at_java_call_stub */, false /* process_frames */);
> 
> This is a nit, but could you put the /* stop_at_java_call_stub*/ and /* process_frames */ before the values? Having a
> bunch of bool parameters seems like it could also become buggy.  In linkResolver.hpp we have enums so that the values
> can be checked and are more explicit at the call sites.  Not this change, but can we fix this later to do the same
> thing?

Per and Stefan would kill me if I put the comments before the value (they explicitly prefer it on the right side of the
value). Personally I have no strong preference. And yes, this also sounds like a good follow-up cleanup RFE.

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

PR: https://git.openjdk.java.net/jdk/pull/296