RFR: 8073389: Remove the include of resourceArea.hpp from classFileParser.hpp
coleen.phillimore at oracle.com
Wed Feb 18 13:25:31 UTC 2015
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:
>>> Please review this patch to get rid of the inclusion of
>>> 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.
> 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.
>> 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
> --- The following branch was pruned ---
> 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.
Thanks for doing the copyrights.
More information about the hotspot-dev