8034761: Remove the do_code_roots parameter from process_strong_roots

Stefan Karlsson stefan.karlsson at oracle.com
Wed Feb 12 06:39:10 PST 2014


On 12/02/14 15:26, Thomas Schatzl wrote:
> 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.

https://bugs.openjdk.java.net/browse/JDK-8034787

If anyone wants to take this RFE, please do so.

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

thanks a lot,
StefanK

>
> Thomas
>
>



More information about the hotspot-gc-dev mailing list