RFR 8209645: Split ClassLoaderData and ClassLoaderDataGraph into separate files

[coleen.phillimore at oracle.com]

On 9/28/18 1:59 AM, Thomas Stüfe wrote:
> Hi Coleen,
>
> looks nice to me. I wish there was an easy way to weed out unnecessary
> includes. I remember Eric Helin mentioning some google tool once but
> never got around testing it.

Yes, I can't remember the name of the tool.  I think there'll be some 
more cleanup of #include directives in the future as time permits.  Some 
of us are pretty dedicated to the cause.
Thanks,
Coleen

>
> Thanks, Thomas
>
>
> On Fri, Sep 28, 2018 at 3:57 AM,  <coleen.phillimore at oracle.com> wrote:
>>
>> On 9/27/18 5:23 PM, Thomas Stüfe wrote:
>>> On Thu, Sep 27, 2018 at 11:09 PM,  <coleen.phillimore at oracle.com> wrote:
>>>>
>>>> On 9/27/18 4:31 PM, Thomas Stüfe wrote:
>>>>> Hi Coleen,
>>>>>
>>>>> this looks good (not an easy patch to review, but I think all that
>>>>> code just moved without changing).
>>>>>
>>>>> Are all includes in the new classLoaderDataGraph.cpp really needed ? I
>>>>> found e.g. no reference to SymbolTable, so I am not sure
>>>>> symbolTable.hpp is needed. I may be off though.
>>>>
>>>> When I did this origially, I tried to minimize #includes, but I'll see if
>>>> there's any I can remove.
>>>> Yes, I can remove systemDictionary.hpp and symbolTable.hpp.
>>>>
>>>>> Does ClassLoaderDataGraphMetaspaceIterator have to be implemented
>>>>> fully in a header (classLoaderDataGraph.hpp)? But this preceedes your
>>>>> change.
>>>> from grep:
>>>> memory/metaspace.cpp:  ClassLoaderDataGraphMetaspaceIterator iter;
>>>>
>>> Sorry, I was not clear. What I oringally meant was whether the
>>> implementations of ClassLoaderDataGraphMetaspaceIterator methods have
>>> to be inline or can be moved to a cpp file, in order to get rid of
>>> "#include metaspace.hpp" in classLoaderDataGraph.hpp.
>>>
>>> However, I did not think this thru. In classLoaderDataGraph.hpp, if
>>> all we need from metaspace.hpp is ClassLoaderMetaspace* we could just
>>> forward-declare ClassLoaderMetaspace instead of including
>>> metaspace.hpp. No need to move method bodies to cpp files.
>>>
>>> Oh, and the patch looks fine as it is to me, I do not need to see
>>> another webrev. If you manage to comb unnecessary includes out, that
>>> would be just an added bonus.
>>
>> Oh, I see what you mean.  Yes, I put ClassLoaderDataGraphIterator::get_next
>> in the cpp file, and remove metaspace.hpp from the #include.  I also removed
>> a few more unneeded #includes from classLoaderDataGraph.hpp and
>> classLoaderData.hpp.  I think I have the minimal set now.  Tested without
>> precompiled headers and another run through mach5 to verify.
>>
>> http://cr.openjdk.java.net/~coleenp/8209645.02/webrev
>>
>> Thanks,
>> Coleen
>>
>>> Thanks, Thomas
>>>
>>>> I think it has to be in the header file.
>>>>
>>>> Thanks!
>>>> Coleen
>>>>
>>>>> Thanks for making classLoaderData.cpp smaller :)
>>>>>
>>>>> Cheers, Thomas
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Thu, Sep 27, 2018 at 7:51 PM,  <coleen.phillimore at oracle.com> wrote:
>>>>>> I think this might be as good of a time as any to split these files.
>>>>>> This
>>>>>> should help with too much coupling between the graph and the class
>>>>>> loader
>>>>>> data itself.  The only function changed and not moved is added an API
>>>>>> in
>>>>>> ClassLoaderDataGraph.
>>>>>>
>>>>>>     110   static void adjust_saved_class(ClassLoaderData* cld);
>>>>>>     111   static void adjust_saved_class(Klass* klass);
>>>>>>
>>>>>>
>>>>>> open webrev at http://cr.openjdk.java.net/~coleenp/8209645.01/webrev
>>>>>> bug link https://bugs.openjdk.java.net/browse/JDK-8209645
>>>>>>
>>>>>> Tested for mach5 hs-tier1-3
>>>>>>
>>>>>> Thanks,
>>>>>> Coleen
>>>>