8034761: Remove the do_code_roots parameter from process_strong_roots

[Mikael Gerdin]

On Thursday 13 February 2014 09.49.01 Stefan Karlsson wrote:
> Mikael,
> 
> On 13/02/14 09:30, Mikael Gerdin wrote:
> > Stefan,
> > 
> > On Wednesday 12 February 2014 10.43.33 Stefan Karlsson wrote:
> >> Hi all,
> >> 
> >> Please, review this patch to remove the do_code_roots parameter from
> >> SharedHeap::process_strong_roots.
> >> 
> >> The changes done are:
> >> - Change the code to rely on the ScannningOption so parameter instead of
> >> do_code_roots.
> >> - Change GenMarkSweep and G1MarkSweep to adjust the code roots with the
> >> help of process_strong_roots instead of doing it as a separate phase
> >> after process_strong_roots.
> >> - Removed the unused FalseClosure.
> >> 
> >> After this patch the adjust phase of the GenMarkSweep and G1MarkSweep
> >> will use the generic code in process_strong_roots, which mark/claim the
> >> nmethods before they are processed. Before the patch these two Serial
> >> Old GC adjust phases skipped the mark/claim part. No noticeable Serial
> >> Old GC time increases were found when this patch was performance tested.
> >> 
> >> This cleanup is needed/wanted for G1 Class Unloading.
> >> 
> >> Webrev:
> >> http://cr.openjdk.java.net/~stefank/8034761/webrev.00/
> > 
> > I think the change is good but I have a comment about the changes to G1's
> > verification code:
> > 
> > 3399     // We apply the relevant closures to all the oops in the
> > 3400     // system dictionary, the string table and the code cache.
> > 3401     const int so = SO_AllClasses | SO_Strings | SO_AllCodeCache;
> > 3402
> > 
> > Shouldn't you remove SO_AllCodeCache here since you plan to go through the
> > entire code cache just below?
> > Otherwise you're just going to do all the nmethod liveness verification
> > work twice since G1VerifyCodeRootOopClosure only adds verification
> > (compared to VerifyRootsClosure) if the non-default flag
> > +G1VerifyHeapRegionCodeRoots is set.
> 
> Yes, you're right.
> 
> http://cr.openjdk.java.net/~stefank/8034761/webrev.01.delta
> http://cr.openjdk.java.net/~stefank/8034761/webrev.01

Looks good, ship it!

/m

> 
> thanks for the review,
> StefanK
> 
> > /Mikael
> > 
> >> RFE:
> >> https://bugs.openjdk.java.net/browse/JDK-8034761
> >> 
> >> thanks,
> >> StefanK