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

[Per Liden]

Hi,

On 2017-07-12 15:58, Roman Kennke wrote:
> Am 12.07.2017 um 14:20 schrieb Stefan Johansson:
>> Hi Roman,
>>
>> On 2017-07-12 12:47, Roman Kennke wrote:
>>> Am 12.07.2017 um 08:44 schrieb Per Liden:
>>>> 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.
>>> I tried that. Unfortunately, it also requires #ifdef INCLUDE_ALL_GCS to
>>> be able to call ParallelScavengeHeap::heap(), or else defeats the
>>> purpose of this patch by requiring CollectedHeap to still carry
>>> size_policy().. which we don't want. In addition to that, if I try to
>>> include parallelScavengeHeap.hpp in adaptiveSizePolicy.hpp, I am getting
>>> freaky circular dependency problems. #ifdef INCLUDE_ALL_GCS in java.cpp
>>> around AdaptiveSizePolicyOutput seems like the lesser evil... done so
>>> here:
>>>
>>> http://cr.openjdk.java.net/~rkennke/8179268/webrev.04/
>>> <http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.04/>
>>>
>>> Ok?
>> I'm no big fan of having #if INCLUDE_ALL_GCS if it can be avoided. A
>> few lines below the call to AdaptiveSizePolicyOutput::print(), we call
>> Universe::heap()->print_tracing_info(). I think we could move
>> AdaptiveSizePolicyOutput::print() into
>> ParallelScavengeHeap::print_tracing_info() without running into any
>> problems.
>>
>> What do you think about that solution?
>
> That's a very good idea!! It alters behaviour slightly (will print
> adaptive size policy stuff in hs_err now) but I think that's for the better.
>
> Incremental:
> http://cr.openjdk.java.net/~rkennke/8179268/webrev.05.diff/
> <http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.05.diff/>
> Full:
> http://cr.openjdk.java.net/~rkennke/8179268/webrev.05
> <http://cr.openjdk.java.net/%7Erkennke/8179268/webrev.05>
>
> Good now?

Looks good. Not sure I follow your comment on hs_err. The adaptive size 
policy stuff prints to the normal log (with gc+ergo=debug).

Before pushing I'll take the liberty of removing the extra space you 
added to the end of ParallelScavengeHeap::print_tracing_info().

  586   AdaptiveSizePolicyOutput::print();
  587
  588 }

Stefan, ok to push?

cheers,
Per

>
> Thanks,
> Roman
>