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

[Stefan Johansson]

On Thu, 8 Oct 2020 08:52:20 GMT, Thomas Schatzl <tschatzl 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?

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

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