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

[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?

Thanks,
Stefan
>
> The incremental diff between 03 and 04:
>
> diff --git a/src/share/vm/runtime/java.cpp b/src/share/vm/runtime/java.cpp
> --- a/src/share/vm/runtime/java.cpp
> +++ b/src/share/vm/runtime/java.cpp
> @@ -487,7 +487,10 @@
>         ClassLoaderDataGraph::dump_on(log.trace_stream());
>       }
>     }
> +
> +#if INCLUDE_ALL_GCS
>     AdaptiveSizePolicyOutput::print();
> +#endif
>   
>     if (PrintBytecodeHistogram) {
>       BytecodeHistogram::print();
>
> Roman