RFR: 8072911: Remove includes of oop.inline.hpp from .hpp files

[Coleen Phillimore]

Sorry for top-post.  This is fine.  I must have misread has_klass_gap().

You didn't answer about fixing the copyright headers.  Please do it, so 
we don't have to deal with the discussion of who's going to do it.  In 
the runtime group at least, we decided to update the copyright headers.  
I have a 'sed' script if you need it.

Thanks,
Coleen

On 2/12/15, 3:24 AM, Stefan Karlsson wrote:
> On 2015-02-12 04:44, Coleen Phillimore wrote:
>>
>> Hi Stefan,
>> I think these cleanups are great.
>
> Thanks.
>
> Inlined:
>
>>
>> On 2/11/15, 7:38 AM, Stefan Karlsson wrote:
>>> Hi all,
>>>
>>> Please, review this patch to fix our includes of oop.inline.hpp.
>>>
>>> http://cr.openjdk.java.net/~stefank/8072911/webrev.01
>>> https://bugs.openjdk.java.net/browse/JDK-8072911
>>>
>>>
>>> From the enhancement description:
>>> ---
>>> Our inline.hpp files should preferably only be included from .cpp 
>>> files or other .inline.hpp files. When they are included in .hpp 
>>> files, the include dependency of the implementation spreads to 
>>> unrelated parts of the code base, and it's easy to get circular 
>>> dependencies.
>>>
>>> This guide line is documented on:
>>> https://wiki.openjdk.java.net/display/HotSpot/StyleGuide#Files
>>>
>>> oop.inline.hpp is one of the more problematic files, and I propose a 
>>> patch to get rid of all inclusions of oop.inline.hpp from other .hpp 
>>> files.
>>> ---
>>>
>>>
>>> Summary of the code changes:
>>>
>>> oop.inline.hpp
>>>  - Added to needed .cpp and .inline.hpp files.
>>>
>>> oop.inline2.hpp
>>>  - This file isn't needed anymore and the contents have been moved 
>>> to oop.inline.hpp.
>>
>> Hooray!!
>
> :)
>
>>
>>>
>>> oop.hpp
>>>  - Inlined klass_gap_offset_in_bytes in the hpp file, since it's 
>>> currently used from a few hpp files.
>>>
>>>  - Create non-inlined versions of is_instance checks, to be used in 
>>> asserts in handles.hpp.
>>>
>>>  - Added has_klass_gap to get rid of dependency against globals.hpp
>>>
>>
>> Why not put klass_gap_offset_in_bytes in the .cpp file?  It's not 
>> going to be inlined completely anyway because the new version is 
>> calling has_klass_gap() which is in the .cpp file.  I don't think 
>> it's usage is performance critical.
>
> has_klass_gap() is only used in the assert to in product builds it 
> will be inlined. It's also used by 
> instanceOopDesc::base_offset_in_bytes(), which is called a lot. I 
> don't want to risk introduce a regression by moving it to the .cpp file.
>
>>
>>> javaClasses.hpp
>>>  - There were a couple of usages of klass.hpp that were moved out to 
>>> javaClasses.cpp and a few that were moved to javaClasses.inline.hpp 
>>> when it wasn't obvious if we needed to inline the functions or not.
>>>
>>
>> I'm confused about this.  Why is there an 
>> java_lang_String::is_instance_inlined() and an is_instance() ?
>
> java_lang_String::is_instance()
>  - Can be used were we don't care if the function is inlined or not, 
> e.g. asserts.
>  - Can be used in hpp files were we don't want to include 
> javaClasses.inline.hpp
>
> java_lang_String::is_instance_inlined()
>  - Should be used were we want to give the compiler a chance to inline 
> the function.
>  - The current usages of this version is in methodHandles.cpp and 
> g1StringDedup.cpp and it isn't obvious to me that I wouldn't introduce 
> a performance regression by using the non-inlineable version.
>
> An alternative to all the changes to javaClasses would be to make 
> oopDesc::klass() completely inlined into oop.hpp, but that would 
> require some care to make sure that we don't include "too much" into 
> oop.hpp. It still might be worth doing as a separate change.
>
>>
>>> vmGCOperations.hpp
>>>  - Moved the VM_GC_Operation destructor to the cpp file.
>>>
>>> collectedHeap.hpp
>>>  - Moved print_on_error
>>>
>>> barrierSet.hpp
>>>  - Moved the inline keyword from the hpp file to the inline.hpp file.
>>>
>>> cardTableModRefBS.hpp
>>>  - Moved inline_write_ref_field to the inline.hpp file.
>>>
>>> objArrayOop.hpp
>>>  - Moved obj_at to the inline.hpp file.
>>>
>>> graphKit.hpp
>>>  - Moved byte_map_base_node to .cpp file, where it's used.
>>>
>>> <other added includes>
>>>  - Added missing includes that were removed when oop.inline.hpp was 
>>> removed from the hpp files.
>>>
>>>
>>> The patch has been tested with JPRT.
>>
>> JPRT compiles without precompiled headers on at least one of the 
>> solaris platforms, I think.  You could compile on linux without 
>> precompiled headers just for verification locally.
>
> I've done that already.
>
>>
>> This change looks awesome.
>
> Thanks,
> StefanK
>
>> Thank you for doing this cleanup!   Can you update the copyright 
>> headers when you check it in?  That should suppress this "please 
>> update the copyright headers" comment on all these files going forward.
>>
>> Thanks!
>> Coleen
>>>
>>> Thanks,
>>> StefanK
>>>
>>
>