RFR: 8073389: Remove the include of resourceArea.hpp from classFileParser.hpp

[David Holmes]

On 18/02/2015 11:25 PM, Coleen Phillimore wrote:
>
> On 2/18/15, 7:17 AM, Stefan Karlsson wrote:
>> Hi David,
>>
>> On 2015-02-18 12:36, David Holmes wrote:
>>> On 18/02/2015 7:35 PM, Stefan Karlsson wrote:
>>>> Hi,
>>>>
>>>> Please review this patch to get rid of the inclusion of
>>>> resourceArea.hpp
>>>> from classFileParser.hpp.
>>>>
>>>>  From the RFE:
>>>>
>>>> The inclusion of resourceArea.hpp in classFileParser.hpp is causing
>>>> cyclic dependencies when I'm changing unrelated code. The main reason
>>>> for this is that a lot of implementation is put inside the
>>>> resourceArea.hpp file instead of a .cpp file.
>>>
>>> I must be missing something here - the implementation in the .hpp
>>> file is because all of the functions are implicitly inline. No
>>> guarantee they will be inlined of course but at least in product mode
>>> many of them should be. So assuming this is a good thing and we want
>>> to keep that for performance then the fix would be to introduce a
>>> .inline.hpp file, not to move stuff to a .cpp file.
>
> I don't see how error reporting functions should be performance
> critical.  I think they're better off in the .cpp file.

We're talking about the implementations in resourceArea.hpp.

David


>>
>> Yes, you're right, if we assume that these functions are performance
>> critical. Personally, I'm not so sure that they were put in the header
>> file for performance reasons. But since I'm not changing
>> resourceArea.hpp I don't have to figure it out at this point.
>>
>>>
>>>
>>>> I've opted to go the easy route now and get rid of the the
>>>> resourceArea.hpp dependency from classFileParser.hpp, but eventually it
>>>> would be good to fix that file.
>>>>
>>>> This patch has to add explicit includes of resourceArea.hpp to other
>>>> .hpp files, that used to get their include from classFileParser.hpp. I
>>>> could have gotten rid of those dependencies as well, but I chose to not
>>>> do that for this patch.
>>>>
>>>> http://cr.openjdk.java.net/~stefank/8073389/webrev.01/
>>>> https://bugs.openjdk.java.net/browse/JDK-8073389
>>>
>>> What was classFileParser.hpp using from resourceArea.hpp ?
>>>
>>> How does the change to src/share/vm/services/runtimeService.cpp fit
>>> in ??
>>
>> There is an include path that was removed when I cleaned up
>> classFileParser.hpp:
>>
>> runtimeServices.cpp
>>  classLoader.hpp
>>   classFileParser.hpp
>>    --- The following branch was pruned ---
>>    resourceArea.hpp
>>     thread.inline.hpp
>>      thread.hpp
>>       threadLocalAllocBuffer.hpp
>>        vm_version.hpp
>>
>> So, I added vm_version.hpp to runtimeService.cpp since it uses
>> Abstract_VM_Version in vm_version.hpp.
>>
>>>
>>> Seems okay - the proof is in the building as always. Need to check
>>> with and without precompiled headers.
>>
>> It passes JPRT, which runs without PCH on Solaris. I've compiled with
>> and without PCH on Linux x64.
>>
>>> And copyright dates need updating.
>>
>> Sure.
>
> Thanks for doing the copyrights.
> Coleen
>
>>
>> Thanks,
>> StefanK
>>
>>>
>>> Cheers,
>>> David
>>>
>>>> Thanks,
>>>> StefanK
>>
>