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

[Jon Masamitsu]

Thomas,

Thanks for reviewing this.

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

Fixed.

>
> - in the test, maybe put the -Xmx/-Xms right after the @run
> main/othervm like in the other tests just for consistency.

Fixed.

A new webrev will be coming after a bit of testing.


>
> ---------------------

I'll address these in a second mail (after some thought).

Jon
>
> 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
>