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

Per Liden per.liden at oracle.com
Tue Jul 11 06:34:21 UTC 2017


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

(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