RFC: Organising .inline.hpp and .hpp includes

[David Holmes]

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.

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