RFC: Organising .inline.hpp and .hpp includes

[Stefan Karlsson]

Hi David,

On 2016-08-12 09:02, David Holmes wrote:
> Okay here's the inverted solution where we only have atomic.hpp:
>
> http://cr.openjdk.java.net/~dholmes/8157907/webrev/
>
> The only significant change is to atomic.hpp of course. ;) And we 
> still need to include the platform specific versions before we define 
> the shared versions.

I still think we need to fix the atomic_<os_cpu>.inline.hpp files, as I 
mentioned earlier:

<quote>
Regarding atomic.inline.hpp and the atomic_<os_cpu>.inline.hpp files,
these files typically don't have a lot of dependencies on other header
files and a pragmatic way forward could be to even further minimize
those dependencies and move the implementation into atomic_<os_cpu>.hpp
files and get rid of atomic.inline.hpp. I know that goes against the
guidelines mentioned above, but it's an alternative to use if the fan
out becomes too large.
</quote>

With the proposed patch we now have these dependencies:
atomic.hpp
-> atomic_linux_x86.inline.hpp
   -> atomic.hpp
   -> os.hpp
     -> handles.hpp etc

so we now:
1) include .inline.hpp from atomic.hpp
2) get a new circular dependency
3) get all os.hpp's dependencies when including atomic.hpp

Thanks,
StefanK

>
> David
> -----
>
> On 12/08/2016 10:56 AM, David Holmes wrote:
>> Hi Volker,
>>
>> On 11/08/2016 11:55 PM, Volker Simonis wrote:
>>> On Thu, Aug 11, 2016 at 2:38 PM, David Holmes
>>> <david.holmes at oracle.com> wrote:
>>>> Hi Stefan,
>>>>
>>>> On 11/08/2016 6:21 PM, Stefan Karlsson wrote:
>>>>>
>>>>> Hi David,
>>>>>
>>>>>
>>>>> On 2016-08-11 09:31, David Holmes wrote:
>>>>>>
>>>>>> I'm trying to tackle the incorrect inclusion of .hpp files 
>>>>>> instead of
>>>>>> .inline.hpp starting with the use of atomic.inline.hpp ref:
>>>>>>
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8157907
>>>>>>
>>>>>> The basic rules would seem to be:
>>>>>>
>>>>>> If class Foo has inline methods that need to call the inline Atomic
>>>>>> methods then:
>>>>>>
>>>>>> - Those methods should be defined in foo.inline.hpp
>>>>>>
>>>>>>    inline void Foo::a() { /* do stuff with atomics */ }
>>>>>>
>>>>>> - foo.hpp declares those methods:
>>>>>>
>>>>>>    inline void a();
>>>>>>
>>>>>> - foo.inline.hpp includes foo.hpp
>>>>>> - foo.inline.hpp includes atomic.inline.hpp
>>>>>> - foo.cpp includes foo.inline.hpp
>>>>>>
>>>>>> Do have agreement so far?
>>>>>
>>>>>
>>>>> Yes.
>>>>
>>>>
>>>> Good start :)
>>>>
>>>>>>
>>>>>> Now suppose we have class Bar which uses Foo.
>>>>>>
>>>>>> - bar.cpp should include foo.inline.hpp
>>>
>>> Only if it calls inline functions from Foo, otherwise including
>>> foo.hpp would be sufficient.
>>
>> okay ... I thought I had seen compiler errors when only including the
>> .hpp file but it may have been a case where the inline functions were 
>> used.
>>
>>>>>> - If bar.hpp only references Foo but doesn't need to see a full
>>>>>> definition then bar.hpp can forward declare Foo
>>>>>> - If bar.hpp defines inline methods that require full access to Foo
>>>>>> then either:
>>>>>>
>>>>>> a) bar.hpp includes foo.inline.hpp; or
>>>>>> b) The inline functions of Bar are themselves moved to 
>>>>>> bar.inline.hpp
>>>>>> which includes foo.inline.hpp
>>>>>>
>>>>>> I think there was general opinion that (a) is undesirable, and that
>>>>>> (b) is preferable - is that right?
>>>>>
>>>>>
>>>>> Yes.
>>>>>
>>>
>>> I agree as well.
>>>
>>> (b) is preferable because .hpp files should generally only include
>>> other .hpp files and not inline.hpp files. This makes it possible that
>>> users of Bar who don't need Bar's inline functions can safely include
>>> bar.hpp. This is the case if users require the full class layout of
>>> Bar and a simple forward declaration of Bar is not enough. So if we
>>> have something like:
>>>
>>> class FooBar {
>>>   void fooBar(Bar b);
>>> }
>>>
>>> foobar.hpp could include bar.hpp without pulling Bar's inline
>>> definitions. foobar.cpp could safely include bar.inline.hpp if inline
>>> functions from Bar are required for the implmentation of foobar(Bar
>>> b).
>>>
>>>>> Alternatively, if inlining isn't needed, place these functions in
>>>>> bar.cpp.
>>>>
>>>>
>>>> Right - though it is effectively impossible to tell whether 
>>>> inlining is
>>>> "needed" - there is a tendency to define "small" methods as inline
>>>> regardless of need.
>>>>
>>>>>>
>>>>>> What I am finding is that the fan out of applying (b) is rather 
>>>>>> large
>>>>>> - to the point where we may end up having x.hpp, x.inline.hpp and
>>>>>> x.cpp variants for very many x!
>>>>>
>>>>>
>>>>> Yes, that's what I've also seen while investigating:
>>>>>
>>>>> https://bugs.openjdk.java.net/browse/JDK-8154079
>>>>> http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-April/022626.html 
>>>>>
>>>>>
>>>>
>>>>
>>>> So I see that you basically avoided the explosive fan out by getting
>>>> rid of
>>>> the inline functions.
>>>>
>>>> I think I missed when this was discussed. :( It completely subsumes
>>>> what I'm
>>>> trying to fix with atomic.hpp.
>>>>
>>>>> I have an updated version that works with and without precompiled
>>>>> headers:
>>>>> http://cr.openjdk.java.net/~stefank/8154079/webrev.02/
>>>>>
>>>>> Regarding atomic.inline.hpp and the atomic_<os_cpu>.inline.hpp files,
>>>>> these files typically don't have a lot of dependencies on other 
>>>>> header
>>>>> files and a pragmatic way forward could be to even further minimize
>>>>> those dependencies and move the implementation into 
>>>>> atomic_<os_cpu>.hpp
>>>>> files and get rid of atomic.inline.hpp. I know that goes against the
>>>>> guidelines mentioned above, but it's an alternative to use if the fan
>>>>> out becomes too large.
>>>>
>>>
>>> If the inline functions of a class Foo have no dependencies on other
>>> HotSpot classes, there's no need to separate foo.hpp and
>>> foo.inline.hpp. In such cases (and atomic.. is probably such a case)
>>> it is perfectly fine to put all the inline definitions right into the
>>> normal foo.hpp file. The separation of foo.hpp and foo.inline.hpp is
>>> only required to break cycles in the hotspot include hierarchy.
>>
>> So you seem to be saying that for my atomic.inline.hpp case the
>> real/best solution is to get rid of atomic.inline.hpp rather than change
>> the files that include only atomic.hpp?
>>
>> To be honest I thought there was more to creating the .inline.hpp files
>> than just avoiding include cycles. But this certainly simplifies the
>> problem I think (he says without actually having tried it ;-) ).
>>
>> Thanks,
>> David
>>
>>>>
>>>> Would be simpler I think to get rid of atomic.inline.hpp and place 
>>>> those
>>>> definitions in atomic.hpp. But that is going against the guidelines
>>>> completely - and there is nothing special about atomic.inline.hpp.
>>>> The main
>>>> issue to address is avoiding compilation errors because the .hpp was
>>>> included and not the .inline.hpp.
>>>>
>>>> I'm not sure we have a tractable problem at this stage. The simplest
>>>> fix is
>>>> to not define the various inline functions, at least at the
>>>> second-level (so
>>>> assume something like atomic.inline.hpp is first-level and important,
>>>> but
>>>> any .hpp's that need it are "fixed" by un-inlining their functions and
>>>> moving to .cpp file). That removes the fan out, at the loss of
>>>> inlining at
>>>> the second level.
>>>>
>>>>>> Just to add another dimension to this, what should precompiled.hpp
>>>>>> include? I would assume the .inline.hpp files right?
>>>>>
>>>>>
>>>>> I would prefer if we got rid of the .inline.hpp files from
>>>>> precompiled.hpp.
>>>>
>>>>
>>>> Okay ... not sure what that really means from a precompilation
>>>> perspective
>>>> ... but if it works, okay.
>>>>
>>>
>>> We currently have the following .inline.hpp files in precompiled.hpp
>>>
>>> # include "asm/assembler.inline.hpp"
>>> # include "interpreter/bytecodeInterpreter.inline.hpp"
>>> # include "memory/allocation.inline.hpp"
>>> # include "memory/universe.inline.hpp"
>>> # include "oops/markOop.inline.hpp"
>>> # include "runtime/frame.inline.hpp"
>>> # include "runtime/handles.inline.hpp"
>>> # include "runtime/orderAccess.inline.hpp"
>>> # include "runtime/prefetch.inline.hpp"
>>> # include "utilities/bitMap.inline.hpp"
>>>
>>> It should be no problem to remove them if every .cpp file has complete
>>> and correct includes (and this is obviously the case as long as the
>>> build without PCH still works). The only drawback may be a performance
>>> degradation but I don't expect that it will be observable.
>>>
>>> What we definitely don't need is to include both versions of a header,
>>> .inline.hpp and .hpp into precompiled.hpp as we have it now, because
>>> every .inline.hpp already includes its corresponding .hpp file anyway.
>>>
>>>> So how to proceed? :)
>>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> Thanks,
>>>>> StefanK
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>
>>>>>
>>>>