RFC: Organising .inline.hpp and .hpp includes

[Volker Simonis]

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.

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

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