6348631 - request for review (updated)

[Volker Simonis]

On Wed, Dec 1, 2010 at 11:41 PM, David Holmes <David.Holmes at oracle.com> wrote:
> Ivan Krylov said the following on 12/02/10 08:33:
>>
>> David,
>> On 02.12.2010 1:15, David Holmes wrote:
>>>
>>> Hmmm
>>>
>>> coleen phillimore said the following on 12/01/10 04:28:
>>>>
>>>> This looks good to me.  I have some concerns with the inlined socket
>>>> functions in:
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~ikrylov/6348631.v3/src/os/linux/vm/os_linux.inline.hpp.cdiff.html
>>>>
>>>> as it causes the .hpp file to import linux system specific hpp files
>>>> which might cause namespace issues down the line.  I'd rather these sorts of
>>>> includes be in a .cpp file.  I don't know if they were inlined for
>>>> performance reasons though, but it seems unlikely to have any benefit.
>>>
>>> When I was taught C programming I was taught that each file should be
>>> self-contained and include all headers that it needed. That way if I include
>>> a header to get the definitions for the "interface" to some library, I don't
>>> have to know what all of its dependencies are - I just include one header
>>> and call the library functions. I don't have to discover all its
>>> dependencies by trial and error (nor does the provider of the library have
>>> to document them all).
>>>
>>> Arguably for our xxx.inline.hpp files it is a different situation because
>>> they should only be included by our own xxx.cpp file. But as a general rule
>>> I would not advocate removing all system includes from all hotspot headers.
>>
>> These files include os_linux.inline.hpp explicitly now.
>
> Hmmm that seems "wrong". I would expect the cpp files to include
> os_linux.hpp and then _that_ file would include os_linux.inline.hpp
>

As far as I remember, the rule of thumb (as detailed in includeDB_core) was:
- put declarations into .hpp
- put definitions of inline functions into .inline.hpp
- .hpp files only include other .hpp files
- every xxx.inline.hpp includes the corresponding xxx.hpp
- every yyy.cpp includes xxx.inline.hpp if it requires definitions
from it or just xxx.hpp if it only needs the declarations

Now after we have no includeDB anymore I would also advocate for the
inclusion of system headers where they are needed. So if
xxx.inline.hpp requires a declaration from zzz.h, it should include
it. Otherwise the users of xxx.inline.hpp (i.e. every file which
includes xxx.inline.hpp) would have to manually include zzz.h BEFORE
including xxx.inline.hpp which is not obvious.

As mentioned by Ivan, another nice benefit of this approach is that if
xxx.inline.hpp will be in the precompiled header file, so will be the
system headers like zzz.h. This  will speed up compilation compared to
the case where zzz.h would be directly included into  the .cpp  file.

> But in a way this reinforces my point. If os_linux.inline.cpp includes say 6
> system headers, we don't want to have to include those same 6 headers in
> every single file that includes os_linux.inline.hpp!
>
> I guess now that we've made the include dependencies explicit we're going to
> realize what an absolute mess they are in places. :(
>
> David
>
>> ./share/vm/classfile/classLoader.cpp
>> ./share/vm/runtime/synchronizer.cpp
>> ./share/vm/runtime/handles.cpp
>> ./share/vm/runtime/thread.cpp
>> ./share/vm/runtime/mutexLocker.hpp
>> ./share/vm/runtime/timer.cpp
>> ./share/vm/runtime/arguments.cpp
>> ./share/vm/runtime/virtualspace.cpp
>> ./share/vm/runtime/task.cpp
>> ./share/vm/runtime/threadLocalStorage.cpp
>> ./share/vm/runtime/atomic.cpp
>> ./share/vm/runtime/os.cpp
>> ./share/vm/runtime/objectMonitor.cpp
>> ./share/vm/memory/space.hpp
>> ./share/vm/memory/gcLocker.hpp
>> ./share/vm/memory/allocation.cpp
>> ./share/vm/prims/nativeLookup.cpp
>> ./share/vm/prims/jni.cpp
>> ./share/vm/compiler/disassembler.hpp
>> ./share/vm/code/stubs.hpp
>> ./share/vm/utilities/bitMap.cpp
>> ./share/vm/utilities/accessFlags.cpp
>> ./share/vm/utilities/histogram.hpp
>> ./share/vm/utilities/ostream.cpp
>> ./share/vm/utilities/debug.cpp
>> ./share/vm/gc_implementation/parallelScavenge/parMarkBitMap.cpp
>> ./share/vm/gc_implementation/parallelScavenge/psVirtualspace.cpp
>> ./share/vm/gc_implementation/concurrentMarkSweep/cmsAdaptiveSizePolicy.cpp
>> ./os/linux/vm/perfMemory_linux.cpp
>> ./os/linux/vm/mutex_linux.inline.hpp
>> ./cpu/sparc/vm/vm_version_sparc.cpp
>> ./cpu/zero/vm/vm_version_zero.cpp
>> ./cpu/x86/vm/vm_version_x86.cpp
>>
>> Even worse: when gcc generates a precompiled header it wants to see
>> definitions of those system structures and functions.That is before any cpp
>> file is being parsed.
>>
>> Thanks,
>> Ivan
>>
>