RFR (XS) 8143615 compactHashtable.hpp includes oop.inline.hpp file
Stefan Karlsson
stefan.karlsson at oracle.com
Mon Dec 7 08:17:18 UTC 2015
On 2015-12-04 23:59, Ioi Lam wrote:
> 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
Looks great! :)
Thanks,
StefanK
>
> 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
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list