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

Jon Masamitsu jon.masamitsu at oracle.com
Tue Mar 15 22:19:34 UTC 2016

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 < maxnumq) failed: Id is out-of-bounds id 23 and max_ _id 23)_ _This was fixed by the change_ _- if (++nextid == numq) {_ _+ assert(!discoveryismt, "Round robin should only be used in_ _serial discovery");_ _+ if (++nextid >= numq) { See the CR for an example log which showed numq changing values between different phases of a collection and where the value of nextid was greater than numq. I assume that the cause for the nextid being greater than numq 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 nextid 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 logreflistcounts(DiscoveredList reflists[], sizet totalcount) PRODUCTRETURN; + void logreflistcounts(DiscoveredList reflists[], uint activelength, sizet totalcount) PRODUCTRETURN; 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: gctestsuite 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

More information about the hotspot-gc-dev mailing list