Request for Review (s) - 8157620: Guarantee in run_task(task, num_workers) fails

[Derek White]

On 6/8/16 1:11 PM, Jon Masamitsu wrote:
> Derek,
>
> Does this fix the issues you raised?
>
> http://cr.openjdk.java.net/~jmasa/8159073/webrev.00/
>
> Jon
>

Hi Jon,

Yes, that looks good! And to be clear, the original 8157620 looks good too.

  - Derek
>
> On 06/07/2016 01: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.
>>
>> - Derek
>>
>> On 6/7/16 2:46 PM, Jon Masamitsu wrote:
>>> New webrev and delta, respectively, with changes indicated below.
>>>
>>> http://cr.openjdk.java.net/~jmasa/8157620/webrev.01/
>>>
>>> http://cr.openjdk.java.net/~jmasa/8157620/webrev_delta.00_01/
>>>
>>> Jon
>>>
>>> On 6/7/2016 1:36 AM, Thomas Schatzl wrote:
>>>> Hi Jon,
>>>>
>>>> On Fri, 2016-06-03 at 14:04 -0700, Jon Masamitsu wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8157620
>>>>>
>>>>> 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
>>>>> failure.
>>>>>
>>>>> http://cr.openjdk.java.net/~jmasa/8157620/webrev.00/
>>>>>
>>>>> 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
>>>> active_workers).
>>>>
>>>> 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).
>>>>
>>>> Thanks,
>>>>    Thomas
>>>>
>>>
>>
>