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

Jon Masamitsu jon.masamitsu at oracle.com
Wed Mar 16 15:48:28 UTC 2016

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 < 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. 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 Looks good. ------------------------------------------------------------------------------ I agree with Thomas about the oddness of using >= in the nextid wraparound check. It seems like the place to initialize it is in setactivemtdegree. Oh, I see you are already considering that. I would also be ok with just a comment in nextid().

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 < activelength; ++i) {

For paranoia, perhaps afterward loop from activelength below maxnumq 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 totalrefs += reflists[i].length(); 724 } 725 logreflistcounts(reflists, maxnumq, totalrefs); 788 balancedtotalrefs += reflists[i].length(); 789 } 790 logreflistcounts(reflists, numq, balancedtotalrefs);

More information about the hotspot-gc-dev mailing list