RFC: Organising .inline.hpp and .hpp includes

[Volker Simonis]

Hi,

in general I agree with Stefan. The only reason for having .inline.hpp
files is to break dependencies because .inline.hpp contain function
DEFINITIONS which obviously have more dependencies than the function
DECLARATIONS in the corresponding .hpp files. That said, theres no
strong enforcement in hotspot to put all inline function definitions
into .inline.hpp files and there a plenty of examples where an
.inline.hpp file doesn't even exist and the corresponding .hpp files
contains all the inline function definitions (just because it didn't
cause any problems until now).

Now to this specific change:

There exists no atomic_<os_cpu>.hpp. We could just as well rename
atomic_<os_cpu>.inline.hpp files into atomic_<os_cpu>.hpp - that's
just cosmetics. But it would help to enforce the general
recommendation the we shouldn't include .inline.hpp files into .hpp
files (which I personally think is a good rule).

Before this change we had only 3 .cpp files which included atomic.hpp:

src/share/vm/code/dependencyContext.cpp
src/share/vm/services/mallocSiteTable.cpp
src/share/vm/services/mallocTracker.cpp

>From these mallocTracker.cpp already included atomic.inline.hpp as
well, so no problem after this change.
@David: it now actually includes atomic.hpp two times because you've
probably mechanically replaced all includes of atomic.inline.hpp by
atomic.hpp. So please remove one include from your patch.

The other two files, mallocSiteTable.cpp and mallocTracker.cpp really
use an inline function from Atomic and should thus actually include
atomic.inline.hpp. If you look at the preprocessed output of the
compilation, you can see that they actually indirectly already include
atomic.inline.hpp (that's why we didn't got a warning until now that
an inline function can not be inlined). So again, David's change
doesn't change anything for these files.

Before this change we had only 7 .hpp files which included atomic.hpp:

src/share/vm/gc/g1/g1StringDedup.hpp
src/share/vm/logging/logOutputList.hpp
src/share/vm/services/memTracker.hpp
src/share/vm/oops/symbol.hpp
src/share/vm/services/mallocSiteTable.hpp
src/share/vm/services/mallocTracker.hpp
src/share/vm/precompiled/precompiled.hpp

>From these, the first three don't actually need atomic.hpp so I
suggest to remove it (see attached webrev).

symbol.hpp only uses the ATOMIC_SHORT_PAIR macro from atomic.hpp. I
propose to move that macro to macros.hpp and include macros.hpp
instead of atomic.hpp into symbol.hpp. (see attached webrev).

precompiled.hpp included atomic.hpp until now and will now (with
David's change) also include what has been atomic.inline.hpp. But
because precompiled.hpp already includes os.hpp anyway, that makes no
difference.

So there remains only the two files mallocSiteTable.hpp and
mallocTracker.hpp. They are already a bad example, because they
contain a lot of inline function DEFINITIONS which should actually
live in a separate .inline.hpp anyway. And both files use inline
functions from Atomic and this apparently only works because
atomic.inline.hpp has been already indirectly included into every
compilation unit which includes mallocSiteTable.hpp and
mallocTracker.hpp. So once again, David's change shouldn't change
anything for these files (although we may consider refactoring these
two files into .hpp and .inline.hpp files in the future).

To sum it up, I think David's change is a nice cleanup. We may
consider to rename the atomic_<os_cpu>.inline.hpp files into
atomic_<os_cpu>.hpp to conform to the rule of not including inline.hpp
files into .hpp files.

For your convenience I've collected all my suggestions and corrections
on top of Davids webrev in a new webrev:

http://cr.openjdk.java.net/~simonis/webrevs/2016/atomic.inline.v1

Regards,
Volker

On Fri, Aug 12, 2016 at 1:16 PM, Stefan Karlsson
<stefan.karlsson at oracle.com> wrote:
> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>
>