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

[Ioi Lam]

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?

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
>