RFC: Organising .inline.hpp and .hpp includes

[David Holmes]

Hi Volker,

On 12/08/2016 11:34 PM, Volker Simonis wrote:
> 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).

Okay - that all sounds reasonable, albeit somewhat subjective.

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

Yes I was anticipating doing that rename.

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

Yep it was a mechanical replacement.

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

Sounds reasonable.

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

Note it was this refactoring in my initial approach that leading to a 
cascade of related refactorings.

> 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

Thanks Volker, that is much appreciated.

If Stefan is okay with this going ahead in this form I will send out the 
formal RFR for 8157907. Otherwise if it is felt this is just 
"rearranging the deck-chairs" I will likely close 8157907 as a dup of 
Stefan's bug and let it all get taken care of at some future date.

Thanks again,
David

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