Request for Review (s) - 8157620: Guarantee in run_task(task, num_workers) fails
jon.masamitsu at oracle.com
Wed Jun 8 02:14:59 UTC 2016
Thanks for the review.
On 6/7/2016 1:43 PM, Derek White wrote:
> Hi Jon,
> The new code looks good!
> While reviewing this I looked over WorkerManager again, and saw a
> problem in the error handling:
> - Line 60 checks for errors creating the WorkerThread or OS thread.
> - Line 66-67 unconditionally update created_workers and tries to
> start a (possibly non-existent) thread.
> - And I don't like the spacing of line 61, but that's a nit.
> I'm not sure if this should get fixed as a separate change or as part
> of this one, but it looks bad.
Yes, that looks bad. I'll fix it as part of a separate change.
> - Derek
> On 6/7/16 2:46 PM, Jon Masamitsu wrote:
>> New webrev and delta, respectively, with changes indicated below.
>> On 6/7/2016 1:36 AM, Thomas Schatzl wrote:
>>> Hi Jon,
>>> On Fri, 2016-06-03 at 14:04 -0700, Jon Masamitsu wrote:
>>>> A phase of a G1 collection chooses its own number of GC workers
>>>> but doesn't add more workers if more are needed than have been
>>>> created. Add additional workers as needed. Also changed the
>>>> guarantee to check the number of workers against the total number
>>>> of workers since the number of GC workers used to execute the
>>>> task may be different than active_workers (as in this case).
>>>> Added an execution of TestGCOld with parameters that evoked the
>>>> Tested fix with TestGCOld.
>>>> Stability tested with gc_test_suite
>>> looks good, with two comments:
>>> - I would _prefer_ if run_task(workgang, uint) always called
>>> add_workers() itself. Otherwise all of these invocations need to call
>>> add_workers() first themselves (if they do not limit themselves to
>>> The run_task(workgang, uint) method has kind of been implemented as a
>>> short/quick way to run a workgang with a given number of threads, so
>>> this requirement adds additional boiler plate code again...
>>> - in the test, maybe put the -Xmx/-Xms right after the @run
>>> main/othervm like in the other tests just for consistency.
>>> One comment related to the code changed, but unrelated to this CR: the
>>> code in G1ConcurrentMark::calc_parallel_marking_threads() looks
>>> (almost?) the same as AdaptiveSizePolicy::calc_active_conc_workers().
>>> Any arguments about cleaning this up at some point?
>>> When looking at the calculation of the
>>> AdaptiveSizePolicy::calc_default_active_workers() method, wouldn't it
>>> be better if instead of using Universe::heap()->capacity(), use
>>> something like Universe::heap()->used() (and actually e.g. for g1 one
>>> could subtract young gen and areas covered by primitive arrays too).
>>> Actually, this calculation should be different for young/old gen gc
>>> imho, as for both completely different areas are covered by the
>>> collection... (just some random thoughts when looking at this).
More information about the hotspot-gc-dev