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

[Stefan Karlsson]

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