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

[Jon Masamitsu]

On 03/15/2016 10:59 PM, Kim Barrett wrote:
>> On Mar 10, 2016, at 2:53 PM, Jon Masamitsu <jon.masamitsu at oracle.com> 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.
>>
>> 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
> Looks good.
>
> ------------------------------------------------------------------------------
> I agree with Thomas about the oddness of using >= in the _next_id
> wraparound check.  It seems like the place to initialize it is in
> set_active_mt_degree.  Oh, I see you are already considering that.
> I would also be ok with just a comment in next_id().

I implemented the initialization strategy and am testing it now.  If all 
goes
well, I'll publish a new webrev.

> ------------------------------------------------------------------------------
> src/share/vm/gc/shared/referenceProcessor.cpp
>   705   for (uint i = 0; i < active_length; ++i) {
>
> For paranoia, perhaps afterward loop from active_length below
> _max_num_q and report any unexpected non-empty lists.

I'll add this.

>
> ------------------------------------------------------------------------------
> test/gc/ergonomics/TestDynamicNumberOfGCThreads.java
>
> Copyright.

Fixed.

Thanks.

Jon
>
> ------------------------------------------------------------------------------
> src/share/vm/gc/shared/referenceProcessor.cpp
>   723     total_refs += ref_lists[i].length();
>   724     }
>   725   log_reflist_counts(ref_lists, _max_num_q, total_refs);
>
>   788     balanced_total_refs += ref_lists[i].length();
>   789     }
>   790   log_reflist_counts(ref_lists, _num_q, balanced_total_refs);
>
>