RFR 8209645: Split ClassLoaderData and ClassLoaderDataGraph into separate files

[Thomas Stüfe]

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.

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