RFR: 8179268: Factor out AdaptiveSizePolicy from top-level interfaces CollectorPolicy and CollectedHeap
Stefan Johansson
stefan.johansson at oracle.com
Tue Jul 11 14:17:19 UTC 2017
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.
>>>
>>> (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