RFR 8209645: Split ClassLoaderData and ClassLoaderDataGraph into separate files

[coleen.phillimore at oracle.com]

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