RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1

[Thomas Schatzl]

Hi Stefan,

On Fri, 2018-11-23 at 15:57 +0100, Stefan Johansson wrote:
> Thanks Thomas for your review,
> 
> Here are my update webrevs. Comments inline:
> Full: http://cr.openjdk.java.net/~sjohanss/8213890/01/
> Inc: http://cr.openjdk.java.net/~sjohanss/8213890/00-01/
> 
> On 2018-11-21 11:26, Thomas Schatzl wrote:
> > Hi,
> > 
> > On Wed, 2018-11-14 at 16:57 +0100, Stefan Johansson wrote:
> > > Hi,
> > > 
> > > Please review this patch for "JEP 344: Abortable Mixed
> > > Collections for G1", the JEP is not yet targeted but the goal is
> > > to get it into JDK 12.
> > > 
> > > JEP: https://bugs.openjdk.java.net/browse/JDK-8190269
> > > Issue: https://bugs.openjdk.java.net/browse/JDK-8213890
> > > Webrev: http://cr.openjdk.java.net/~sjohanss/8213890/00
> > 
> > Some comments:
> > 
> > [...]
> > 
> > 

Thanks for all the updates!

> 
> > 
> >   - g1CollectionSet.cpp:514: the old log message contained
> > information about the number of regions added. I would prefer if
> > that would be kept.
> > 
> >   - g1CollectionSet.cpp:527: the log message could add the amount
> > of optional regions added.
> > 
> >   - g1CollectionSet.cpp:531: it would be nice to give an idea of
> > the predicted time (and available) in that message. The original
> > message contained that.
> > 
> 
> I tried to make the log a bit more consistent, and not have to much 
> redundant info:
> * The first message includes min, max and time constraints.
> * The break conditions contains the reason.
> * The last log contains the number of regions really added and the
> times.
> 
> I think this makes it easier to follow, but if you feel strongly
> about this I will add it back the numbers mentioned above.

I think the messages are okay now.

> 
> >   - g1CollectionSet.cpp:508: the new code is lacking the message
> > that if the predicted time of the minimum set of regions we are
> > going to take already exceeds the pause time goal or not (i.e. the
> > "expensive regions" message).
> > 
> > Note that the original message was confusing, see
> > https://bugs.openjdk.java.net/browse/JDK-8165849; maybe this could
> > be
> > fixed here while we are here. Note that I do not see this as
> > mandatory,
> > but I would like to have information about that we took additional
> > regions that will likely exceed the pause time goal.
> 
> I added another log_debug-line if we have expensive regions, this
> should make it clear and we keep the information. And yes, after this
> that issue can be closed.

You can add multiple CRs in a single commit message to get resolved.

> > 
> >   - G1OptionalCSet looks and feels like an iterator over the
> > optional regions the G1CollectionSet manages. Maybe a better name
> > like G1OptionalCSetIterator would be nice.
> > 
> > [...]
> > 
> > I think that would tighten the interface a bit, but I haven't
> > really evaluated how much effort that would be, or if it is even
> > possible.
> 
> I agree that this is somewhat an iterator, it wraps the CSet to keep 
> track of how much of the optional work has been done. I'm not sure
> how  to retrofit the whole optional evac code to fit the normal
> iterator pattern and I would like to defer this to a later
> cleanup/re-write.

Okay.

> 
> I did some cleanup of the G1OptionalCSet though. I renamed values
> and methods as well as moved some calls to make the code easier to
> follow, and I think that improved readability quite a bit.

Thanks!

> 
> > 
> >   - that's something to be evaluated separately I guess, but one
> > could encode the "index_in_opt_cset" into the InCSetState table.
> > 
> > The hope is that that would remove the need for index_in_opt_cset
> > member in HeapRegion.
> 
> I agree that it is unfortunate that we need to add more members to
> the  HeapRegion, but I would rather look at only having one "index in
> cset" and not both _young_index_in_cset and _index_in_opt_cset. I
> started looking at this but decided that it was out of the scope for
> this feature and that it should be looked at separately. I can file
> an RFE for this.

Okay.

> > 
> > Otherwise seems fine :)
> 
> Thanks again Thomas,
> Stefan
> 

Some additional minor nits:

g1CollectionSet.cpp:566; I think the prefix "To ensure progress the" of
the log message is not necessary.

g1CollectionSet.cpp:567: s/even if/although (I would think, no native
speaker)

g1GCPhaseTimes.hpp:102: unaligned closing curly brace

g1Policy.hpp:405/408: I would prefer that the description does not
mention actual values from the code but uses "that percent(age)"
instead to remove redundancy.

Thanks,
  Thomas