RFC: Organising .inline.hpp and .hpp includes

[David Holmes]

Hi Stefan,

On 12/08/2016 5:34 PM, Stefan Karlsson wrote:
> 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

I'm not sure what you are suggesting. There are platform specific 
implementations so they either have to be included (regardless of 
whether the file name includes .inline) or they cease to be inline 
functions (which is very unappealing).

> 2) get a new circular dependency

I can fix that. I hadn't realized the platform inline files includes the 
top-level atomic.hpp - that makes no sense. It will already have been 
included (both in old and new code)

> 3) get all os.hpp's dependencies when including atomic.hpp

There are a mix of cases here. A number of platforms don't actually use 
any os:: code - so easy fix. The Solaris code has some os::is_MP and 
stub code (os::atomic_xxxx_func) but it is only there for 32-bit, so it 
can go. Zero has a weird dependency where the atomic functions delegate 
to os::atomic* functions. Otherwise we are left with os::is_MP usage.


But we seem to be damned if we do and damned if we don't. There is no 
perfect arrangement of source and header files. At least this approach 
seems far less disruptive than the explosion of .inline.hpp files.

Cheers,
David

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