Request for review JDK-8145911 - Create an AbstractGangTask that parallelizes work on region basis

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Request for review JDK-8145911 - Create an AbstractGangTask that parallelizes work on region basis

Alexander Harlap

Please review change for JDK-8145911 - Create an AbstractGangTask that parallelizes work on region basis.

Change is located at http://cr.openjdk.java.net/~aharlap/8145911/webrev.01/

Attached are  pre-review comments from Thomas ( for version webrev.00).

Alex




Hi Alex,

  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: 
> http://cr.openjdk.java.net/~aharlap/8145911/webrev.00/

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

- I think the G1ParAbstractGangTask should not be used for the
G1ParRemoveSelfForwardPtrsTask as it uses the hrclaimer differently
than others.

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

- similar to other tasks, passing the G1CollectedHeap seems unnecessary
for the G1ParScrubRemSetTask constructor.

- there is a superfluous space in the par_work call in
G1ClearBitMapTask.

Overall I think it is still a worthwhile (if only small) cleanup.

Thanks,
  Thomas

Loading...