RFR: 8254164: G1 only removes self forwarding pointers for last collection set increment

[Stefan Johansson]

On Thu, 8 Oct 2020 08:05:57 GMT, Thomas Schatzl <tschatzl at openjdk.org> wrote:

> Hi all,
> 
>   can I have reviews for this change that fixes a previously hard to reproduce crash that started showing up much more
>   frequently in conjunction with changes to young gen sizing (JDK-8244603)?
> 
> The issues is that the code to process regions where evacuation failed only processes the last increment. This leaves
> forwarded pointers in the mark word of some objects. Obviously other code does not like that, e.g. the crashes in
> JDK-8248438 which I plan to close as duplicate.  Only checking the last collection set increment for regions that
> failed evacuation is wrong in case there is an evacuation failure caused by reference processing in a region that has
> been evacuated in earlier evacuation increments. Reference processing (e.g. finalizers) can make it necessary to
> resurrect an otherwise unreachable object at the very end of the collection that can't be copied and is located in a
> region evacuated in an earlier increment.  This optimization to only look at the last increment for removal of self
> forwarding pointers has been introduced in JDK-8218668.  Until changes to young gen sizing in JDK-8244603 this crashes
> has been a very rare occurrence, but with that it has been common in some tier8 tests (KitchenSink8/24h, DaCapo24h)
> particularly with some additional targeted verification (enable verification only at the end of mixed gcs with optional
> evacuation). Without this fix both tests fail within 10 minutes to 2 hours. With the patch everything completes fine.
> Testing:
> - tier1-5
> - KitchenSink/Dacapo24h with verification code and JDK-8244603.
> 
> Note: since this change strictly does more work during evacuation failure handling I consider this amount of testing
> sufficient, i.e. with JDK-8244603. It is very hard to reproduce without JDK-8244603.

Nice work tracking this one down Thomas. The fix looks good, just a small thing about the comment.

src/hotspot/share/gc/g1/g1EvacFailure.cpp line 264:

> 262:   // We need to check all regions whether they need self forward removals, not only
> 263:   // the last collection set increment. Reference processing may copy over and fail
> 264:   // evacuation in any region in the collection set.

I think your explanation in the summary is very clear and it would be nice to capture it in the comment as well. Maybe
something like:  Suggestion:

  // We need to check all regions whether they need self forward removals, not only
  // the last collection set increment. The reason is that reference processing (e.g.
  // finalizers) can make it necessary to resurrect an otherwise unreachable object at
  // the very end of the collection. The resurrected object might be located in a region
  // evacuated in an earlier increment, but copying it at the end of the collection can
  // trigger an evacuation failure.

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

Marked as reviewed by sjohanss (Reviewer).

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