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

[Kim Barrett]

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

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

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

Copyright.

------------------------------------------------------------------------------
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);