RFC: Organising .inline.hpp and .hpp includes

[Stefan Karlsson]

Hi David,

On 2016-08-15 03:50, David Holmes wrote:

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

I think this a good change and that it's worthwhile.

It seems like you agreed to do the renaming of the atomic_<os_cpu>.hpp:
   "Yes I was anticipating doing that rename."

With that in place I would be happy to review the patch.

Thanks,
StefanK

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