8034761: Remove the do_code_roots parameter from process_strong_roots

Thomas Schatzl thomas.schatzl at oracle.com
Wed Feb 12 06:00:29 PST 2014


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

I had a first pass over the changes, some comments:

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);

- in SharedHeap::process_strong_roots(), the asserts reading

   assert(&code_roots != NULL...

will never fail, so they can be removed imo.

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

Thanks,
  Thomas




More information about the hotspot-gc-dev mailing list