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

[Jon Masamitsu]

On 1/22/2014 8:45 AM, Mikael Gerdin wrote:
> 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.

OK.

>
> (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.

OK.  All good then.

Jon

>
>> 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