request for review (m) - CR 6798898

[John Coomes]

Jon Masamitsu (Jon.Masamitsu at Sun.COM) wrote:
> 6798898: CMS: bugs related to class unloading
> 
> Override the should_remember_klasses() and remember_klass() in
> more closures derived from OopClosure in cases where the closures are
> doing marking of classes when class unloading is on.
> 
> http://cr.openjdk.java.net/~jmasa/6798898/webrev.00/

Generally looks good, I like abstracting the revisit / remembering
info into new classes.  Found some nits and have some suggestions on
the debug-only code.

cmsOopClosures.hpp:  this assert is replicated a number of times in
different files

 102     assert(_should_remember_klasses || !must_remember_klasses(),
 103       "Should be remembering classes in this context.");

Would be nice to have a single copy in a debug-only function, e.g.,
check_remember_klasses() or verify_remember_klasses() or something.

Also, can you strengthen it to the following?  (Which makes it easier
to read, IMHO.)

       assert(_should_remember_klasses == must_remember_klasses(), "mismatch");

indentation nit on lines 116-117

 115   Par_OopWithRevisitClosure(CMSCollector* collector, 
 116                         ReferenceProcessor* rp, 
 117                         CMSMarkStack* revisit_stack):

Comment nit:  should this start with "See comment ..."?

 262   // Comment on should_remember_klasses() above.

concurrentMarkSweepGeneration.hpp:  line wrap nit - fits on one line

1799     _mark_and_push(collector, span, bit_map, 
1800       revisit_stack, work_queue) { }

iterator.hpp & iterator.cpp:

MarkingChecker & MarkingUnchecker - they check whether we should be
remembering classes.  Call them RememberKlassesChecker?

All the uses of _must_remember_klasses, must_remember_klasses(),
set_must_remember_klasses() are in asserts or in the MarkingChecker
classes, which are in an #ifdef ASSERT block.  But the decls in
iterator.hpp use PRODUCT_RETURN.  I'd drop PRODUCT_RETURN and instead
use #ifdef ASSERT consistently for the decls and the bodies.

-John