8034761: Remove the do_code_roots parameter from process_strong_roots

[Stefan Karlsson]

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

Thanks for taking a look at the patch.

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

>
> - in SharedHeap::process_strong_roots(), the asserts reading
>
>     assert(&code_roots != NULL...
>
> will never fail, so they can be removed imo.

Yes, of course. I'll change it.
>
> - 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."


thanks,
StefanK
>
> Thanks,
>    Thomas
>
>