Attached are pre-review comments from Thomas ( for version
really sorry for not answering earlier. :(
On Tue, 2017-03-07 at 14:06 -0500, Alexander Harlap wrote:
> Hi Thomas,
> I looked at JDK-8145911 - Create an AbstractGangTask that
> parallelizes work on region basis
> I attempted to work on it. Could you please just tell me if I
> understood task or ...
> Here is change:
I looked at the change, some comments:
- the name G1ParAbstractGangTask is too nondescriptive I think. It
should indicate that using this gang task, you distribute work on a per
region basis. Right now I do not have a better name.
Also the par_work method should probably be called something better as
it also has no indication on what this actually does. Even if only
calling it "all_heap_regions_[do_]work" or so would be an improvement
- I think the G1ParAbstractGangTask should not be used for the
G1ParRemoveSelfForwardPtrsTask as it uses the hrclaimer differently
- I would put the new abstract gang task where the heapregionclaimer
currently is, not at the end of an already large header file.
- some of the tasks could be simplified a bit in the process: e.g. in
ParKnownGarbageTask, or ParRebuildRSTask, remove the _g1 member that is
only used once anyway.
- similar to other such tasks, the G1ScrubRSClosure should probably be
an inner class of its task. G1ScrubRSClosure also never uses its _g1
- similar to other tasks, passing the G1CollectedHeap seems unnecessary
for the G1ParScrubRemSetTask constructor.
- there is a superfluous space in the par_work call in
Overall I think it is still a worthwhile (if only small) cleanup.