[From nobody Tue Mar 21 21:06:42 2017 Received: from [140.78.145.205] (/140.78.145.205) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Wed, 15 Mar 2017 04:17:22 -0700 Message-ID: <1489576640.3398.21.camel@oracle.com> Subject: Re: Question about JDK-8145911 - Create an AbstractGangTask that parallelizes work on region basis From: Thomas Schatzl <thomas.schatzl@oracle.com> To: Alexander Harlap <alexander.harlap@oracle.com> Date: Wed, 15 Mar 2017 12:17:20 +0100 In-Reply-To: <18ea93c9-e622-55d2-6a7a-c9c869755200@oracle.com> References: <18ea93c9-e622-55d2-6a7a-c9c869755200@oracle.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu3.1 Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Hi Alex, =C2=A0 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:=C2=A0 > http://cr.openjdk.java.net/~aharlap/8145911/webrev.00/ =C2=A0 I looked at the change, some comments: - the name G1ParAbstractGangTask=C2=A0is 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, =C2=A0 Thomas ]