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

[Stefan Karlsson]

On 2015-12-03 15:44, 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 ...

My main concern is not about the compile times, but that we're spreading 
the include mess.

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

https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#Files

>
> My guess is this happened when Stefan reported the bug:
>
> + precompiled header is used

I compile without precompiled headers.

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

Yes.

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

Yes.

> + Avoid including .inline.hpp files in "regular" .hpp files, since 
> that may indirectly
>   include a large set of header files.

Yes.

>
> Anyway, I don't think this can be easily adhered to, or easily 
> understood, or cared
> about, by the developers.

It's quite simple if you by default _don't_ put definitions in the 
header file. Put all implementation in the .cpp files, and if it's 
important to inline the functions, put the code in an .inline.hpp files. 
It's when code is put into .hpp that things start to become problematic.

Granted, _simple_ getters and setters in header files don't pose a problem.

> 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

$ find . -name \*inline.hpp -prune -o -name \*.hpp -print | xargs grep 
inline.hpp | grep -v precompiled.hpp | grep "#include" |  awk -F: '{ 
print $2}' | sort | uniq -c
       5 #include "asm/macroAssembler.inline.hpp"
       1 #include "assembler_zero.inline.hpp"
       1 #include "bytes_linux_ppc.inline.hpp"
      17 #include "memory/allocation.inline.hpp"
       2 #include "memory/universe.inline.hpp"
       2 #include "oops/oop.inline.hpp"
       1   #include "os_windows.inline.hpp"  // needed for sockaddr_in
       1 #include "prims/jvmtiThreadState.inline.hpp"
       6 #include "runtime/frame.inline.hpp"
       6 #include "runtime/handles.inline.hpp"
       1 #include "runtime/objectMonitor.inline.hpp"
       2 #include "runtime/orderAccess.inline.hpp"
       8 #include "runtime/thread.inline.hpp"
       2 #include "utilities/bitMap.inline.hpp"
       1 #include "utilities/hashtable.inline.hpp"


There used to be loads more includes of oop.inline.hpp, but I removed them.

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

Not that the size shouldn't be measured in number of lines, but what 
they depend on:

  120   void add(unsigned int hash, Symbol* symbol) {
  121     add(hash, new Entry(hash, symbol));
  122   }
  123
  124   void add(unsigned int hash, oop string) {
  125     add(hash, new Entry(hash, string));
  126   }


Take these lines, for example, they force the include of 
allocation.inline.hpp in compactHashtable.cpp. Simply moving these 
definitions to the .cpp removes the need to include allocation.inline.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"

It would be good to decrease that list, not add to it. Maybe 
allocation.inline.hpp needs to fixed, and parts moved to the .cpp file.

I see that g1Predictions.hpp added allocation.inline.hpp. I recently 
went over the entire GC code base and got rid of _all_ includes of 
.inline.hpp files from .hpp files.

Stefank

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