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

Fri Nov 23 14:57:12 UTC 2018

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:
> 
>   - I would really like to have a (trace?) log message at the end of the
> evacuation showing the used memory (and the max? sizes) for the
> optional remembered sets.
> 
> This is very interesting information to (in the future) size number of
> threads etc.

Good point, I totally forgot you requested this a time back. Added it 
along with the other scanning metrics.

> 
>   - g1CollectedHeap.cpp:3740: I am not sure why the "copy_time" is
> aggregated in the loop as the value is only ever used outside.
> If that is the case, i.e. it is otherwise inconsequential whether the
> time is aggregated in the pss or in the loop, I would prefer to get the
> reading only at the very end of the loop.
We need to call pss->reset_trim_ticks() each round to avoid hitting 
assertions in G1EvacPhaseWithTrimTimeTracker.

> 
>   - in G1EvacuateOptionalRegionTask::scan_roots(), I generally prefer to
> have declarations of variables close to their use - i.e. the change
> does not need to declare copy_time ahead if above comment is true, and
> scan_time is only used after the loop anyway.
> 
Moved scan_time down, but copy_time needs to stay.

> The reason is that I assume that if you declare variables at the top,
> that they are used throughout the code which is not the case here.
> 
>   - g1CollectedHeap.cpp:3761: would it be possible to make the comment
> in the assert more clear or at least it sounds awkward to me. E.g.
> "Should not do partial trimming here" would be just fine.
Changed to "Unexpected partial trimming done during optional evacuation".

> 
>   - g1CollecteHeap.cpp:3823: please remove the mention of the 75% (the
> actual value) in the comment, as the actual value is returned by the
> optional_evacuation_fraction() function.
> 
> In this case I would just remove the comment here, and add a comment
> explaining optional_evacuation_fraction() at its declaration.
> 
Removed this comment and the comment in g1Policy.hpp should be enough I 
think.

>   - g1CollectionSet.cpp: s/activly/actively
Check.

> 
>   - g1CollectionSet.cpp: the lines from 432-436 could be moved into an
> "add_old_optional_region(hr)" to enhance the similarities between
> add_as_optional() and add_as_old() functions.
> Also just in case, the code could assert !optional_is_full() before
> adding.
Done, also added two more assert form add_old_region().

> 
>   - g1CollectionSet.cpp: maybe in the add_as_old() method we could add a
> trace log entry like the one in line 438.
Good idea, added.

> 
>   - 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.

>   - 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.

> 
>   - g1CollectionSet.hpp: maybe the G1CollectionSet class could be
> documented a little. I think that "optional regions" are non-standard
> in gc literature, so it might be good to explain them here a bit.
> 
> I would also group the "optional_region_*_length" with the
> _optional_regions declaration, and put a small comment above it like
> done just above.
Done.

> 
> In general the new methods are not documented at all.
Looked through them and I think most methods are named in a way that 
they don't need documentation.

> 
>   - G1OptionalCSet looks and feels like an iterator over the optional
> regions the G1CollectionSet manages. Maybe a better name like
> G1OptionalCSetIterator would be nice.
> 
> Potentially this interface could be improved by having a method in
> G1CollectionSet that returns you this iterator over the "recently
> prepared" optional collection set. And the code retrieves that iterator
> for every set of optional regions.
> 
> And G1OptionalCSetIterator only returns you the regions one by one.
> 
> E.g. something like
> 
> G1OptionalCSetIterator it = _cset-
>> get_next_optional_regions(time_left);
> 
> for (element e in it) do {
>    ...
> }
> 
> (I am intentionally obtuse about the interface for retrieving the
> elements, either the usual has_next() or STL iterator style would be
> fine with me)
> 
> 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.

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.

> 
>   - g1OopClosures.hpp: please add a short comment on how the
> G1ScanRSForOptionalClosure is used/what it is for.
Done.

> 
>   - g1Policy.hpp: it would be nice to separately describe the purpose of
> the two constants here. In any case, there is a typo, i.e.
> s/fraction/fractions.
Done.

> 
>   - heapRegion.hpp: please describe _index_in_opt_cset a bit
Done.

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

> 
> Thanks,
>    Thomas
> 
> 