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

Jon Masamitsu jon.masamitsu at oracle.com
Wed Jan 22 21:44:16 UTC 2014

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/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 docoderoots. With docoderoots set we will call dooop on all on-stack nmethods regardless of the ScanOption value.

OK.

(docoderoots 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 oopsdo on them twice even if SOAllCodeCache|SOScavengeCodeCache is set. As I hinted at above we may already have seen some of the nmethods due to the CodeBlobClosure being passed to Threads::oopsdo.

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