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.
- 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.
- 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.
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.
- 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.
- g1CollectionSet.cpp: s/activly/actively
- 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.
- g1CollectionSet.cpp: maybe in the add_as_old() method we could add a
trace log entry like the one in line 438.
- 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.
- 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.
- 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.
In general the new methods are not documented at all.
- 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.
- 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.
- g1OopClosures.hpp: please add a short comment on how the
G1ScanRSForOptionalClosure is used/what it is for.
- 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.
- heapRegion.hpp: please describe _index_in_opt_cset a bit
Otherwise seems fine :)
Thanks,
Thomas