RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1 (original) (raw)
Thomas Schatzl thomas.schatzl at oracle.com
Wed Nov 21 10:26:34 UTC 2018
- Previous message (by thread): RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1
- Next message (by thread): RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
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-
getnextoptionalregions(timeleft);
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
- Previous message (by thread): RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1
- Next message (by thread): RFR: 8213890: Implementation of JEP 344: Abortable Mixed Collections for G1
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]