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