8034761: Remove the do_code_roots parameter from process_strong_roots
Thomas Schatzl
thomas.schatzl at oracle.com
Wed Feb 12 14:26:20 UTC 2014
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
More information about the hotspot-gc-dev
mailing list