RFR (M): 8210462: Fix remaining mentions of initial mark

Thomas Schatzl thomas.schatzl at oracle.com
Tue Jul 7 08:39:27 UTC 2020


Hi Kim,

   thanks for your review.

On 03.07.20 22:29, Kim Barrett wrote:
>> On Jul 3, 2020, at 2:06 PM, Thomas Schatzl <thomas.schatzl at oracle.com> wrote:
>>
>> Webrevs:
>>
>> http://cr.openjdk.java.net/~tschatzl/8210462/webrev.0_to_1/ (diff)
>> http://cr.openjdk.java.net/~tschatzl/8210462/webrev.1/ (full)
>>
>> Thanks,
>>   Thomas
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
>   537   // otherwise reachable ensure that it is marked in the bitmap for concurrent marking
> 
> [pre-existing]
> There should be a sentence break or a semi-colon or something between
> "reachable" and "ensure"
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1CollectorState.hpp
>    53   volatile bool _in_concurrent_mark_gc;
> 
> Shouldn't this be _in_concurrent_start_gc?
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1OopClosures.inline.hpp
>   254     // closure during a concurrent mark pause then attempt to mark the object.
>   
> s/concurrent mark/concurrent start/ ?
> 
> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1Policy.cpp
>   682   } else if (!is_young_only_pause(this_pause)) {
>   683     // This is a mixed GC. Here we decide whether to continue doing more
> 
> Maybe there should be an assertion that this really is a mixed pause?
> 
> Or maybe this should be testing for a mixed pause, and the else clause
> should assert is_young_only_pause.
> 

Fixed. Ran through tier1 again because of that change.

> ------------------------------------------------------------------------------
> src/hotspot/share/gc/g1/g1Policy.cpp
> [removed]
>   636   bool this_pause_was_young_only = collector_state()->in_young_only_phase();
> 
> Why was this variable removed and uses replaced with
> is_young_only_only(this_pause), rather than keeping the variable but
> updating the initialization?
> 
> ------------------------------------------------------------------------------
> 

The reason is that this_pause_was_young_only is a local defined waaay up 
at the top of that method and I thought instead of referencing that one 
it is better to do the (very simple) function call.
I can change that again if you want.

Webrevs:
http://cr.openjdk.java.net/~tschatzl/8210462/webrev.1_to_2/ (diff)
http://cr.openjdk.java.net/~tschatzl/8210462/webrev.2/ (full)
Testing:
tier1, local gc/g1 jtreg run

Thanks,
   Thomas


More information about the hotspot-gc-dev mailing list