RFR: 8254028: G1 incorrectly updates scan_top for collection set regions during preparation of evacuation

[Stefan Johansson]

On Thu, 8 Oct 2020 18:45:49 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that makes values of G1RemSetScanState::_scan_top for regions in the initial
>>   collection set consistent with ones in optional collection set?
>> So currently G1RemSetScanState::_scan_top is top() for regions in the initial collection set although they will never
>> be scanned as enforced when dropping the remsets onto the card table. For the optional collection set, G1 sets scan_top
>> manually to NULL when selecting the next few optional regions (using G1RemSet::exclude_region_from_scan()).  When
>> debugging this discrepancy recently posed some slight surprise for me, so I would like to change this. Also there is
>> some risk that future code that relies on that property will be surprised.  Testing: tier1-4; lots of kitchensink runs
>> with JDK-8254164.
>> Thanks,
>>   Thomas
>
> I'm not to fond of the comment saying this is not really needed. After the fix for
> [JDK-8252752](https://bugs.openjdk.java.net/browse/JDK-8252752) `prepare_region_for_scan()` don't do anything with
> regions in the collection-set so I would prefer if we instead filtered those regions at the call-site.  In
> `G1PrepareRegionsClosure::do_heap_region(HeapRegion* hr)` we could add:  ++
>       // Pepare regions not in the collection set for scanning.
>       if (!hr->in_collection_set())  {
>         _g1h->rem_set()->prepare_region_for_scan(hr);
>       }
> 
> This way we don't need to check for collection-set regions in the assert either. What do you think?

> I considered changing the caller as suggested, but this adds additional
> (unspoken) requirements on it:

But it also makes the code clearer showing that we don't do any thing for collection-set regions in this call.

> - the caller needs to know that the initialization of the corresponding
scan_top field is NULL (or something to that effect).
> - and that this is the only preparation work performed in that method.
> (I.e. there might be future things to do)

But I do agree that it's nice to assert that the collection-set regions are in the correct state. And if we want to
prepare for future preparation that also do stuff for the collection-set regions I think we should go back to the old
if-else like this: ++ if (r->in_collection_set()) {
  // Regions in the collection-set should not be scanned.
  assert(_scan_state->scan_top(hrm_index) == NULL,
         "scan_top of region %u is unexpectedly " PTR_FORMAT,
         hrm_index, p2i(_scan_state->scan_top(hrm_index)));
} else if(r->is_old_or_humongous_or_archive()) {
  // Prepare all other regions for scanning.
  _scan_state->set_scan_top(hrm_index, r->top());
} else {
  assert(r->is_free(), "Region %u should be free but is %s",
         hrm_index, r->get_type_str());
}

I think this satisfies both our wishes, do you agree?

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

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