RFR (S): 8205426: Humongous continues remembered set does not match humongous start region one in Kitchensink

[Thomas Schatzl]

Hi Kim,

  thanks for your review.

On Wed, 2018-07-04 at 19:17 -0400, Kim Barrett wrote:
> > On Jun 26, 2018, at 1:15 PM, Thomas Schatzl <thomas.schatzl at oracle.
> > com> wrote:
> > 
> > Hi all,
> > 
> >  can I have reviews for this bug in keeping remembered sets
> > consistent between HC and HS regions, causing crashes with
> > verification?
> > 
[...]
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8205426
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8205426/webrev/
> > Testing:
> > new test case, hs-tier1-4,jdk-tier1-3
> > 
> > Thanks,
> >  Thomas
> 
> The new live_bytes_for seems like it's overly verbose and
> complicated, and could instead just be something like:

The verbosity mainly comes from me just trying to add the comment about
the approximation somehwere fitting.
Cramming it into a "?" operator statement may cause confusion. But see
further below.

> 
> size_t live_bytes_for(HeapRegion* r) {
>   // For humongous regions, use liveness of associated starts region.
>   HeapRegion* hr = r->is_humongous() ? r->humongous_start_region() :
> r;
>   return _cm->liveness(hr->hrm_index()) * HeapWordSize;
> }
> 
> However, I wonder if this is the right way to go?
> 
> It seems to me that the underlying problem is that we're even asking
> the live_bytes question of humongous_continues regions, when nobody
> really cares about the answer (after fixing the policy).  We're also
> computing it for young regions and for humongous_start regions
> containing an objarray, where again the (fixed) policy doesn't care.
> 
> It seems to me that things would be simpler if it were the policy
> that asked the live_bytes question, after it has determined that it
> was interested in the answer.  The only downside I can think of is
> that the G1RemSetTrackingPolicy would be additionally coupled to the
> G1ConcurrentMark object.

I would like to have the G1RemSetTrackingPolicy mostly self-contained
and not looking through things all over the place; however the main
issue seems to be that we actually need to ask for the liveness and
update the remembered sets for HC regions.

It would be much nicer, and remove a lot of distinction between regular
regions and humongous regions if the remembered sets were ready.

So my master plan ;) had been to have the incremental mixed gcs ready
by jdk11 and then fairly easily implement sharing of remembered sets
between multiple regions.
That not only solves this issue, but also quite much decreases
remembered set overhead in many dimensions (e.g. if we process old gen
regions during mixed gc in increments >> 1 anyway, why provide that
possibility; of course there are some drawbacks to that in reducing
flexibility that is not used at the moment anyway).

(The exactly same thought came up when talking to ErikD about this
change).

There is a new webrev at

http://cr.openjdk.java.net/~tschatzl/8205426/webrev.1 (full)
http://cr.openjdk.java.net/~tschatzl/8205426/webrev.0_to_1 (diff, but
almost useless due to many changes)

That at least separates the concerns about humongous/regular region a
bit.

Thanks,
  Thomas