RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1
thomas.schatzl at oracle.com
Mon Nov 26 11:03:08 UTC 2018
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
> 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
> 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.
> > - 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.
> > Otherwise seems fine :)
> Thanks again Thomas,
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
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.
More information about the hotspot-gc-dev