RFR: 8179268: Factor out AdaptiveSizePolicy from top-level interfaces CollectorPolicy and CollectedHeap

Per Liden per.liden at oracle.com
Wed Jul 12 06:44:43 UTC 2017


Hi Roman,

On 2017-07-11 16:17, Stefan Johansson wrote:
> Hi Roman,
>
> On 2017-07-11 08:34, Per Liden wrote:
>> Hi,
>>
>> On 2017-07-10 18:35, Roman Kennke wrote:
>>> Hi Per,
>>>
>>> thanks for the review!
>>>
>>>>
>>>>> AdaptiveSizePolicy is not used/called from outside the GCs, and not
>>>>> all
>>>>> GCs need them. It makes sense to remove it from the CollectedHeap and
>>>>> CollectorPolicy interfaces and move them down to the actual subclasses
>>>>> that used them.
>>>>>
>>>>> I moved AdaptiveSizePolicyOutput to parallelScavengeHeap.hpp, it's
>>>>> only
>>>>> used/implemented in the parallel GC. Also, I made this class AllStatic
>>>>> (was StackObj)
>>>>
>>>> AdaptiveSizePolicyOutput::print() is actually called from
>>>> runtime/java.cpp also, so it's used outside of ParallelGC. I'm fine
>>>> with moving it, but we should have the proper #includes in java.cpp.

I just realized that this doesn't build on linux-i586, which builds a 
minimal JVM where INCLUDE_ALL_GCS isn't defined (and will thus not 
include parallelScavangeHeap.hpp). Rather than having some ugly #ifdef 
INCLUDE_ALL_GCS at the AdaptiveSizePolicyOutput::print() call site I 
suggest we keep AdaptiveSizePolicyOutput in adaptiveSizePolicy.hpp for now.

(Use --with-target-bits=32 --with-jvm-variants=minimal when test 
building for linux-i586)

cheers,
Per

>>>>
>>>> (Your patch doesn't actually build in its current form. I suspect
>>>> you're using precompiled headers which have a tendency to hide a lot
>>>> of errors caused by missing includes)
>>>>
>>> I added the include.
>>>
>>>>>
>>>>> Tested by running hotspot_gc jtreg tests without regressions.
>>>>>
>>>>> http://cr.openjdk.java.net/~rkennke/8179268/webrev.00/
>>>>
>>>> collectorPolicy.hpp:
>>>> --------------------
>>>>  258   void cleared_all_soft_refs();
>>>>
>>>> Please declare this virtual too (that's the best we can do to signal
>>>> intent until we have C++11/override)
>>>>
>>> Ok.
>>>
>>>>
>>>> collectorPolicy.cpp:
>>>> --------------------
>>>>  224   this->CollectorPolicy::cleared_all_soft_refs();
>>>>
>>>> Please remove "this->" to match the super-call style used in other
>>>> places in this file.
>>>
>>> ok.
>>>
>>>
>>>>
>>>> Btw, I can sponsor the patch if you want.
>>>
>>> Find the updated webrev here:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8179268/webrev.03/
>>> <http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.03/>
>>
>>
>> Looks good!
>>
> This looks good to me too,
> Stefan
>> (Awaiting a second review before I can push)
>>
>> cheers,
>> Per
>>
>>>
>>> Cheers,
>>> Roman
>>>
>>>>
>>>> cheers,
>>>> Per
>>>>
>>>>> <http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.00/>
>>>>>
>>>>> Roman
>>>>>
>>>
>



More information about the hotspot-gc-dev mailing list