RFC: Organising .inline.hpp and .hpp includes

[Stefan Karlsson]

Hi David,

On 12/08/16 11:53, David Holmes wrote:
> 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).

OK. Let me try to explain how I see it:

When a .hpp file includes an .inline.hpp file it becomes "tainted" by 
that inclusion. All the restrictions on where .inline.hpp files can be 
included, now apply to that .hpp file. This is needed because 
.inline.hpp files are "allowed" to have a much larger include dependency 
set than the .hpp file. When someone changes an .inline.hpp file it's 
assumed to be OK to start including more .inline.hpp files. However, 
these new includes will start to leak into the "tainted" .hpp files, and 
this is what we try to avoid by having that rule that no .hpp files are 
allowed to include .inline.hpp files.

Note that the tainting of the .hpp files is a transitive property, so 
all .hpp files that include tainted .hpp files become tainted themselves.

So, back to your patch:

atomic.hpp now include atomic_<os_cpu>.inline.hpp, this taints both 
atomic.hpp and all includers of atomic.hpp. Therefore all these files 
will be affected if someone decides to include new .inline.hpp files 
into the atomic_<os_cpu>.inline.hpp files.

I'm suggesting that we convert the atomic_<os_cpu>.inline.hpp files into 
atomic_<os_cpu>.hpp files and make sure that they are not "tainted" by 
checking and minimizing the existing include dependencies in those files.

With that in place, it is less likely that new .inline.hpp dependencies 
will start to leak out from atomic.hpp. Someone would have to break the 
include rules we have in the style guide. Well, they could also have 
included a tainted .hpp file, but I hope we can gradually get rid of 
them by getting rid of inclusions of .inline.hpp from .hpp files.

Thanks,
StefanK
>
>> 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
>>>>>>>
>>>>>>>
>>>>>>
>>