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

[Ioi Lam]

Hi Stefan,

Thanks for the explanation. I have updated the webrev

http://cr.openjdk.java.net/~iklam/8143615-compactHashtable-inline-hpp.v2/

updates from the last version:

[1] Move the two add functions identified by Stefan to 
compactHashtable.inline.hpp
[2] Removed allocation.inline.hpp from compactHashtable.hpp
[3] Removed oop.inline.hpp from compilerDirectives.hpp

Thanks
- Ioi

On 12/4/15 1:23 AM, Stefan Karlsson wrote:
> 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
>>>
>>
>