RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Jul 14 12:37:18 UTC 2014
Hi Coleen,
Thanks for sponsoring this!
bytes, ad, nativeInst and vmreg.inline were used quite often
in shared files, so it definitely makes sense for these to have
a shared header.
vm_version and register had an umbrella header, but that
was not used everywhere, so I cleaned it up.
That left adGlobals, jniTypes and interp_masm which
are only used a few time. I did these so that all files
are treated similarly.
In the end, I didn't need a header for all, as they were
not really needed in the shared files, or I found
another good place, as for adGlobals.
I added you and David H. as reviewer to the webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8049325-cpuInc/webrev.01/
I hope this is ok with you, David.
Thanks,
Goetz.
-----Original Message-----
From: hotspot-dev [mailto:hotspot-dev-bounces at openjdk.java.net] On Behalf Of Coleen Phillimore
Sent: Montag, 14. Juli 2014 14:09
To: hotspot-dev at openjdk.java.net
Subject: Re: RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
I think this looks like a good cleanup. I can sponsor it and make the
closed changes also again. I initially proposed the #include cascades
because the alternative at the time was to blindly create a dispatching
header file for each target dependent file. I wanted to see the
#includes cleaned up instead and target dependent files included
directly. This adds 5 dispatching header files, which is fine. I
think the case of interp_masm.hpp is interesting though, because the
dispatching file is included in cpu dependent files, which could
directly include the cpu version. But there are 3 platform independent
files that include it. I'm not going to object though because I'm
grateful for this cleanup and I guess it's a matter of opinion which is
best to include in the cpu dependent directories.
Thanks,
Coleen
On 7/14/14, 3:56 AM, Lindenmaier, Goetz wrote:
> Hi,
>
> David, can I consider this a review?
>
> And I please need a sponsor for this change. Could somebody
> please help here? Probably some closed adaptions are needed.
> It applies to any repo as my other change traveled around
> by now.
>
> Thanks and best regards,
> Goetz.
>
>
> -----Original Message-----
> From: David Holmes [mailto:david.holmes at oracle.com]
> Sent: Freitag, 11. Juli 2014 07:19
> To: Lindenmaier, Goetz; Lois Foltan
> Cc: hotspot-dev at openjdk.java.net
> Subject: Re: RFR(L): 8049325: Introduce and clean up umbrella headers for the files in the cpu subdirectories
>
> On 10/07/2014 12:03 AM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> foo.hpp as few includes as possible, to avoid cycles.
>> foo.inline.hpp * must have foo.hpp, as it contains functions declared in foo.hpp
>> (either directly or via the platform files.)
>> * should include foo.platform.inline.hpp, so that shared files that
>> call functions from foo.platform.inline.hpp need not contain the
>> cascade of all the platform files.
>> If code in foo.platform.inline.hpp is only used in the platform files,
>> it is not necessary to have an umbrella header.
>> foo.platform.inline.hpp Should include what is needed in its code.
>>
>> For client code:
>> With this change I now removed all include cascades of platform files except for
>> those in the 'natural' headers.
>> Shared files, or files with less 'diversity' should include the general foo.[inline.]hpp.
>> (foo.platform.cpp should not get a cascade with bar.os_platform.[inline.]hpp
>> headers, but include bar.[inline.]hpp.)
>> If it's 1:1, I don't care, as discussed before.
>>
>> Does this make sense?
> I find the overall structure somewhat counter-intuitive from an
> implementation versus interface perspective. But ...
>
> Thanks for the explanation.
>
> David
>
>> Best regards,
>> Goetz.
>>
>>
>> which of the above should #include which others, and which should be
>> #include'd by "client" code?
>>
>> Thanks,
>> David
>>
>>> Thanks,
>>> Lois
>>>
>>>> David
>>>> -----
>>>>
>>>>> src/cpu/sparc/vm/c1_Runtime1_sparc.cpp
>>>>> - include nativeInst.hpp instead of nativeInst_sparc.hpp
>>>>> - include vmreg.inline.hpp instead of vmreg_sparc.inline.hpp
>>>>> (however this could pull in more code than needed since
>>>>> vmreg.inline.hpp also includes asm/register.hpp and code/vmreg.hpp)
>>>>>
>>>>> src/cpu/ppc/vm/stubGenerator_ppc.cpp
>>>>> - change not related to clean up of umbrella headers, please
>>>>> explain/justify.
>>>>>
>>>>> src/share/vm/code/vmreg.hpp
>>>>> - Can lines #143-#15 be replaced by an inclusion of
>>>>> vmreg.inline.hpp or will
>>>>> this introduce a cyclical inclusion situation, since
>>>>> vmreg.inline.hpp includes vmreg.hpp?
>>>>>
>>>>> src/share/vm/classfile/classFileStream.cpp
>>>>> - only has a copyright change in the file, no other changes
>>>>> present?
>>>>>
>>>>> src/share/vm/prims/jvmtiClassFileReconstituter.cpp
>>>>> - incorrect copyright, no current year?
>>>>>
>>>>> src/share/vm/opto/ad.hpp
>>>>> - incorrect copyright date for a new file
>>>>>
>>>>> src/share/vm/code/vmreg.inline.hpp
>>>>> - technically this new file does not need to include
>>>>> "asm/register.hpp" since
>>>>> vmreg.hpp already includes it
>>>>>
>>>>> My only lingering concern is the cyclical nature of
>>>>> vmreg.hpp/vmreg.inline.hpp. It might be better to not introduce the new
>>>>> file "vmreg.inline.hpp" in favor of having files include vmreg.hpp
>>>>> instead? Again since vmreg.inline.hpp includes vmreg.hpp there really
>>>>> is not much difference between the two?
>>>>>
>>>>> Thanks,
>>>>> Lois
>>>>>
>>>>>
>>>>> On 7/7/2014 4:52 AM, Lindenmaier, Goetz wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I decided to clean up the remaining include cascades, too.
>>>>>>
>>>>>> This change introduces umbrella headers for the files in the cpu
>>>>>> subdirectories:
>>>>>>
>>>>>> src/share/vm/utilities/bytes.hpp
>>>>>> src/share/vm/opto/ad.hpp
>>>>>> src/share/vm/code/nativeInst.hpp
>>>>>> src/share/vm/code/vmreg.inline.hpp
>>>>>> src/share/vm/interpreter/interp_masm.hpp
>>>>>>
>>>>>> It also cleans up the include cascades for adGlobals*.hpp,
>>>>>> jniTypes*.hpp, vm_version*.hpp and register*.hpp.
>>>>>>
>>>>>> Where possible, this change avoids includes in headers.
>>>>>> Eventually it adds a forward declaration.
>>>>>>
>>>>>> vmreg_<cpu>.inline.hpp contains functions declared in register_cpu.hpp
>>>>>> and vmreg.hpp, so there is no obvious mapping to the shared files.
>>>>>> Still, I did not split the files in the cpu directories, as they are
>>>>>> rather small.
>>>>>>
>>>>>> I didn't introduce a file for adGlobals_<cpu>.hpp, as adGlobals mainly
>>>>>> contains machine dependent, c2 specific register information. So I
>>>>>> think optoreg.hpp is a good header to place the adGlobals_<cpu>.hpp
>>>>>> includes in,
>>>>>> and then use optoreg.hpp where symbols from adGlobals are needed.
>>>>>>
>>>>>> I moved the constructor and destructor of CodeletMark to the .cpp
>>>>>> file, I don't think this is performance relevant. But having them in
>>>>>> the header requirs to pull interp_masm.hpp into interpreter.hpp, and
>>>>>> thus all the assembler include headers into a lot of files.
>>>>>>
>>>>>> Please review and test this change. I please need a sponsor.
>>>>>> http://cr.openjdk.java.net/~goetz/webrevs/8049325-cpuInc/webrev.01/
>>>>>>
>>>>>> I compiled and tested this without precompiled headers on linuxx86_64,
>>>>>> linuxppc64,
>>>>>> windowsx86_64, solaris_sparc64, solaris_sparc32, darwinx86_64,
>>>>>> aixppc64, ntamd64
>>>>>> in opt, dbg and fastdbg versions.
>>>>>>
>>>>>> Currently, the change applies to hs-rt, but once my other change
>>>>>> arrives in other
>>>>>> repos, it will work there, too. (I tested it together with the other
>>>>>> change
>>>>>> against jdk9/dev, too.)
>>>>>>
>>>>>> Best regards,
>>>>>> Goetz.
>>>>>>
>>>>>> PS: I also did all the Copyright adaptions ;)
More information about the hotspot-dev
mailing list