RFR (XS) 8143615 compactHashtable.hpp includes oop.inline.hpp file

[David Holmes]

On 4/12/2015 12:44 AM, Ioi Lam wrote:
> On 12/3/15 1:09 AM, Stefan Karlsson wrote:
>> Hi Ioi,
>>
>> Thanks a lot for fixing this!
>>
>> Would you mind swapping these lines around?:
>>
>>  #include "classfile/javaClasses.hpp"
>> +#include "classfile/compactHashtable.inline.hpp"
>
> Thanks for noticing. I will fix that.
>> Except for that nit, this looks good.
>>
>> As David says, we should get rid of the allocation.inline.hpp include
>> as well, but I think we could defer that to a separate patch (if you
>> want to).
>
> TL;DR  -- this fix probably won't do much in terms of speeding up a
> rebuild, but
> may still be a good clean up ...
>
>
> Read the following if you're bored ----v
>
> Actually, I am not quite familiar with the rules of when/if *.inline.hpp
> can
> be inlcluded by a "regular" hpp file.Is that documented somewhere?

I think this is evolving based on a number of cleanups in this area. 
Unfortunately include strategies can be many and varied and hotspot is 
certainly not an example of consistency.

My understanding of what we should aim for is that:
- no .hpp file should include a .inline.hpp file
- any .hpp file that needs to be included by other .hpp files should not 
have any implicit or explicit inline functions (they should be moved to 
a .inline.hpp file)
- .cpp files include .inline.hpp files if they need the inlined 
functions (else the .hpp file)
- .inline.hpp files include their associated .hpp files

Trying to enforce this in the current code base can lead to a transitive 
closure of changes that touches dozens of files. I know Mikael V. has 
also been looking into a problematic case. :)

David
-----

> My guess is this happened when Stefan reported the bug:
>
> + precompiled header is used
>
> + oop.inline.hpp was (indirectly) included into precompiled.hpp.gch.d
>     - this was actually not caused by compactHashtable.hpp(this file is not
>       included by precompiled.hpp)
>       You can verified this from auto-generated files such as
> hotspot/linux_amd64_compiler2/generated/dependencies/precompiled.hpp.gch.d
>     - the culprit is src/share/vm/compiler/compilerDirectives.hpp, which
> also
>       includes oop.inline.hpp,introduced in this changeset:
>           changeset:   9323:a176d4737606
>           user:        neliasso
>           8137167:     JEP165: Compiler Control: Implementation task
>           Summary:     Compiler Control JEP
>     - the depedndency is precompiled.hpp -> ciEnv.hpp ->
> compilerDirectives.hpp-> oop.inline.hpp
>
> + oop.inline.hpp itself includes many GC header files, so when Stefan
> touches one of them,
>    everything is recompiled.
>
> So my interpretation is:
>
> + We should try to minimize the number of other header files included in
> "regular" .hpp files
>    - Complex inline functions should be defined in .inline.hpp files.
> + Avoid including .inline.hpp files in "regular" .hpp files, since that
> may indirectly
>    include a large set of header files.
>
> Anyway, I don't think this can be easily adhered to, or easily
> understood, or cared
> about, by the developers.Moreover, there are actually a lot of various
> inline.hpp files
> being included in .hpp files.
>
> hotspot/src/share$ find . -name \*inline.hpp -prune -o -name \*.hpp
> -print | xargs grep inline.hpp | grep -v precompiled.hpp | wc
>       60     189    4387
>
> So I think in an ideal world, we should have a tool that can automatically
> flags the "bad" includes. But, I am not sure how much time we want to
> invest in that :-(
>
> In this particular case, after I removed oop.inline.hpp from
> compilerDirectives.hpp
> (it turned out to be an unnecessary include), only 7 include files were
> removed from
> precompiled.hpp.gch.d, so I am not sure it's a big win.
>
> But in terms of clean up, it's still a good idea to move big inline
> function
> definitions out of compactHashtable.hpp.
>
>
> Also, I can see that allocation.inline.hpp is included by many .hpp
> files, so
> I guess it's OK for compactHashtable.hpp to do the same. If a clean up
> is needed,
> that would need to be done in a separate RFE.
>
> hotspot/src/share$ find . -name \*inline.hpp -prune -o -name \*.hpp
> -print | xargs grep allocation.inline.hpp
> ./vm/precompiled/precompiled.hpp:# include "memory/allocation.inline.hpp"
> ./vm/prims/jvmtiEventController.hpp:#include "memory/allocation.inline.hpp"
> ./vm/prims/jvmtiEnvThreadState.hpp:#include "memory/allocation.inline.hpp"
> ./vm/prims/jvmtiThreadState.hpp:#include "memory/allocation.inline.hpp"
> ./vm/oops/generateOopMap.hpp:#include "memory/allocation.inline.hpp"
> ./vm/compiler/compileTask.hpp:#include "memory/allocation.inline.hpp"
> ./vm/compiler/methodMatcher.hpp:#include "memory/allocation.inline.hpp"
> ./vm/classfile/compactHashtable.hpp:#include "memory/allocation.inline.hpp"
> ./vm/classfile/symbolTable.hpp:#include "memory/allocation.inline.hpp"
> ./vm/classfile/stringTable.hpp:#include "memory/allocation.inline.hpp"
> ./vm/utilities/growableArray.hpp:#include "memory/allocation.inline.hpp"
> ./vm/utilities/stack.hpp:#include "memory/allocation.inline.hpp"
> ./vm/utilities/array.hpp:#include "memory/allocation.inline.hpp"
> ./vm/gc/g1/g1Predictions.hpp:#include "memory/allocation.inline.hpp"
> ./vm/runtime/perfData.hpp:#include "memory/allocation.inline.hpp"
> ./vm/logging/log.hpp:#include "memory/allocation.inline.hpp"
> ./vm/libadt/dict.hpp:#include "memory/allocation.inline.hpp"
>
>
> Thanks
> - Ioi
>
>
>
>> Thanks,
>> StefanK
>>
>> On 2015-12-03 03:32, Ioi Lam wrote:
>>> Please review a very small fix:
>>>
>>> http://cr.openjdk.java.net/~iklam/8143615-compactHashtable-inline-hpp/
>>>
>>> Bug: compactHashtable.hpp includes oop.inline.hpp file
>>>
>>>     https://bugs.openjdk.java.net/browse/JDK-8143615
>>>
>>> Summary of fix:
>>>
>>>     + deleted the include of oop.inline.hpp from compactHashtable.hpp
>>>     + moved all inlined functions that require oop.inline.hpp into a
>>>       new file, compactHashtable.inline.hpp
>>>
>>> Tests:
>>>
>>>     JPRT
>>>
>>> Thanks
>>> - Ioi
>>
>