Review Request (M) 8032379: Remove the is_scavenging flag to process_strong_roots

[Mikael Gerdin]

Jon,

On Wednesday 22 January 2014 07.10.33 Jon Masamitsu wrote:
> Mikael,
> 
> http://cr.openjdk.java.net/~mgerdin/8032379/webrev.0/src/share/vm/gc_impleme
> ntation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.frames.html
> 
> I don't mean to expand this task but the comment after the parameter
> "do_code_roots" caught my eye.
> 
> > 3736       gch->gen_process_strong_roots(_cmsGen->level(),
> > 3737                                     true,   // younger gens are roots
> > 3738                                     true,   // activate
> > StrongRootsScope 3739                                    
> > SharedHeap::ScanningOption(roots_scanning_options()), 3740               
> >                      &notOlder,
> > 3741                                     true,// walk all of code cache if
> > (so & SO_AllCodeCache) 3742                                     NULL,
> > 3743                                     &klass_closure);
> 
> Did you consider whether the "do_code_roots" is needed any more?

The comment is not incorrect, but there is a second effect to setting 
do_code_roots. With do_code_roots set we will call do_oop on all on-stack 
nmethods regardless of the ScanOption value.

(do_code_roots is only ever false at one point where I can see, so we may want 
to get rid of it at some point anyway)

> 
> http://cr.openjdk.java.net/~mgerdin/8032379/webrev.0/src/share/vm/memory/sha
> redHeap.cpp.frames.html
> >   208     if (so & SO_ScavengeCodeCache) {
> >   209       assert(code_roots != NULL, "must supply closure for code
> >   cache");
> >   210
> >   211       // We only visit parts of the CodeCache when scavenging.
> >   212       CodeCache::scavenge_root_nmethods_do(code_roots);
> >   213     }
> >   214     if (so & SO_AllCodeCache) {
> >   215       assert(code_roots != NULL, "must supply closure for code
> >   cache");
> >   216
> >   217       // CMSCollector uses this to do intermediate-strength
> >   collections. 218       // We scan the entire code cache, since
> >   CodeCache::do_unloading is not called. 219      
> >   CodeCache::blobs_do(code_roots);
> >   220     }
> 
> The new code allows for scavenge_root_nmethods_do() and blobs_do() both
> to be called
> (depending on the contents of "so").  That could not happen with the old
> code.  Is that
> OK?  I didn't read enough code to know if blobs_do would also process
> the same roots
> again as scavenge_root_nmethods_do().

It will not cause any problems since nmethods are "claimed" when scanned so we 
won't call oops_do on them twice even if SO_AllCodeCache|SO_ScavengeCodeCache 
is set. As I hinted at above we may already have seen some of the nmethods due 
to the CodeBlobClosure being passed to Threads::oops_do.

> 
> That's all.  Rest looks good.  Nice change.

Thanks Jon.
/Mikael

> 
> Jon
> 
> On 1/21/2014 5:13 AM, Mikael Gerdin wrote:
> > Hi all,
> > 
> > as a part of implementing JEP-156 (G1 class unloading) we are doing some
> > cleanups in order to refactor the strong root processing code.
> > 
> > Currently there are two "dimensions" in which
> > SharedHeap::process_strong_roots is configured. One is the ScanOption
> > enum, the other is the is_scavenging booelan. The semantic meaning of
> > is_scavenging can easily be folded into the ScanOption enum by:
> > 
> > * Introducing a SO_AllCodeCache/SO_ScavengeCodeCache distinction (in a way
> > similar to SO_AllClasses/SO_SystemClasses)
> > * Noting that passing a CLD closure to Threads::oops_do is only needed
> > when we want to determine the precise liveness of classes, this is
> > already signaled by SO_SystemClasses.
> > 
> > Bug link: https://bugs.openjdk.java.net/browse/JDK-8032379
> > Webrev: http://cr.openjdk.java.net/~mgerdin/8032379/webrev.0
> > 
> > Thanks
> > /Mikael