8034761: Remove the do_code_roots parameter from process_strong_roots

[Thomas Schatzl]

Hi Stefan,

On Wed, 2014-02-12 at 15:19 +0100, Stefan Karlsson wrote:
> Hi Thomas,
> 
> On 12/02/14 15:00, Thomas Schatzl wrote:
> > Hi Stefan,
> >
> > On Wed, 2014-02-12 at 10:43 +0100, 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.
> >>
> >> This cleanup is needed/wanted for G1 Class Unloading.
> >
> > I do not think that it is really "clean" to define an operator for
> > ScanningOption, because ScanningOption is an enum, and the result of the
> > | operator is a set of ScanningOption. This is just a minor comment, no
> > real problem for me.
> >
> > However since the change adds the operator for ScanningOption, is it
> > possible to fix up the uses where we first create an int with the
> > various options and then cast that int to ScanningOption? :)
> >
> > E.g.
> >
> >    int so = SharedHeap::SO_AllClasses | SharedHeap::SO_Strings |
> > SharedHeap::SO_ScavengeCodeCache;
> >
> > and later the cast of "so" to ScanningOption:
> >
> >    gch->gen_process_strong_roots(_gen->level(),
> >                                  true,  // Process younger gens, if any,
> >                                         // as strong roots.
> >                                  false, // [...]
> > ---->                           SharedHeap::ScanningOption(so),
> >                                  &par_scan_state.to_space_root_closure(),
> >                                  &par_scan_state.older_gen_closure(),
> >                                  &klass_scan_closure);
> 
> Yes, as you say, we already use the ScaningOption type to describe the 
> set of ScanningOptions.
>
> I don't want to change all the usages of ScanningOptions for this patch. 
> I would have to change code like:
> src/share/vm/gc_implementation/concurrentMarkSweep/concurrentMarkSweepGeneration.hpp: 
> void remove_root_scanning_option(int o) { _roots_scanning_options &= ~o;  }
> 
> which I think is out-of-scope for this patch. Could we try to figure out 
> a better way to handle the ScanningOptions in another RFE?

I saw that too :(

Fine with me to do this in another CR.

> > - the removal of g1_process_weak_roots seems changed the semantics of
> > the code slightly. Now the nmethods seem to be "marked", while they were
> > not before.
> > If I am correct, did you notice any significant performance impact?
> 
> You are correct. That's the part I tried to explain above with:
> 
> "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."

Sorry, I overlooked that paragraph :(

Then consider the change reviewed and good to me.

Thomas