Request for Review (s) - 8149343: assert(rp->num_q() == no_of_gc_workers) failed: sanity

[Jon Masamitsu]

Thomas,

Thanks for having a look.

On 3/15/2016 5:12 AM, Thomas Schatzl wrote:
> Hi Jon,
>
> On Thu, 2016-03-10 at 11:53 -0800, Jon Masamitsu wrote:
>> https://bugs.openjdk.java.net/browse/JDK-8149343
>>
>> The error here was that the number of active workers was
>> not always set correctly for parallel Reference processing.
>> The first fix was to set the number of active workers in the
>> ReferenceProcessor in the task constructor.  Once thatDiff
>> was fixed a subsequent assertion failure occurred.
>>
>> #  Internal Error (/export/jmasa/java/jdk9-rt
>> -8149343/src/share/vm/gc/shared/referenceProcessor.cpp:884),
>> pid=18444, tid=18500
>> #  assert(id < _max_num_q) failed: Id is out-of-bounds id 23 and max
>> id 23)
>>
>> This was fixed by the change
>>
>> -    if (++_next_id == _num_q) {
>> +    assert(!_discovery_is_mt, "Round robin should only be used in
>> serial discovery");
>> +    if (++_next_id >= _num_q) {
>>
>> See the CR for an example log which showed _num_q changing
>> values between different phases of a collection and where the
>> value of _next_id was greater than _num_q.
> I assume that the cause for the _next_id being greater than _num_q is
> that in a previous GC the number of threads has been larger than in the
> current.

Yes.  The log in the CR shows the case where _num_q was 23, then 2 and
then 23 again.

>
> While this change seems to work, I would kind of prefer that _next_id
> is simply reset in some initialization. The greater or equal seems
> surprising here.

Early on I started to look for a place where I could add the 
initialization and
decided not to.  The _next_id is part of the reference processor and
reference processor created and reused so there didn't seem to
be a dependable place to put the initialization.   One place that might
work is to set it in the set_active_mt_degree()

   void set_active_mt_degree(uint v)        { _num_q = v; }

Try it?

Jon
>
> However I am fine with keeping that but maybe add a comment about why
> we need >= here.
>
>> The last change was to add a parameter to the logging function so
>> that
>> the logging only output the active lists (which made the reading of
>> the
>> logs simpler)
>>
>> -  void log_reflist_counts(DiscoveredList ref_lists[], size_t
>> total_count) PRODUCT_RETURN;
>> +  void log_reflist_counts(DiscoveredList ref_lists[], uint
>> active_length, size_t total_count) PRODUCT_RETURN;
>>
>> This patch fixes UseG1GC for the failure where
>> UseDynamicNumberOfGCThreads and ParallelRefProcEnabled
>> are both turned on.  There is still a failure with UseParallelGC
>> that is being fixed under 8150994.
>>
>> http://cr.openjdk.java.net/~jmasa/8149343/webrev.00/
>>
>> Testing: gc_test_suite with UseDynamicNumberOfGCThreads on and
>> ParallelRefProcEnabled on and off.   rbt in progress
> Could you also fix the indentation in referenceProcessor.cpp:717?
>
> Looks good from what I understand of the ref processing code.
>
> Thanks,
>    Thomas