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

[Thomas Schatzl]

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.

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.

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