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

Mikael Gerdin mikael.gerdin at oracle.com
Wed Jan 22 16:45:32 UTC 2014

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/gcimpleme ntation/concurrentMarkSweep/concurrentMarkSweepGeneration.cpp.frames.html I don't mean to expand this task but the comment after the parameter "docoderoots" caught my eye. > 3736 gch->genprocessstrongroots(cmsGen->level(), > 3737 true, // younger gens are roots > 3738 true, // activate > StrongRootsScope 3739 > SharedHeap::ScanningOption(rootsscanningoptions()), 3740 > &notOlder, > 3741 true,// walk all of code cache if > (so & SOAllCodeCache) 3742 NULL, > 3743 &klassclosure); Did you consider whether the "docoderoots" 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 & SOScavengeCodeCache) { > 209 assert(coderoots != NULL, "must supply closure for code > cache"); > 210 > 211 // We only visit parts of the CodeCache when scavenging. > 212 CodeCache::scavengerootnmethodsdo(coderoots); > 213 } > 214 if (so & SOAllCodeCache) { > 215 assert(coderoots != 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::dounloading is not called. 219 > CodeCache::blobsdo(coderoots); > 220 } The new code allows for scavengerootnmethodsdo() and blobsdo() 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 blobsdo would also process the same roots again as scavengerootnmethodsdo().

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::processstrongroots is configured. One is the ScanOption > enum, the other is the isscavenging booelan. The semantic meaning of > isscavenging can easily be folded into the ScanOption enum by: > > * Introducing a SOAllCodeCache/SOScavengeCodeCache distinction (in a way > similar to SOAllClasses/SOSystemClasses) > * Noting that passing a CLD closure to Threads::oopsdo is only needed > when we want to determine the precise liveness of classes, this is > already signaled by SOSystemClasses. > > Bug link: https://bugs.openjdk.java.net/browse/JDK-8032379 > Webrev: http://cr.openjdk.java.net/~mgerdin/8032379/webrev.0 > > Thanks > /Mikael

More information about the hotspot-gc-dev mailing list