RFC: Organising .inline.hpp and .hpp includes

[David Holmes]

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
>>>
>>>
>>