RFR: 8072911: Remove includes of oop.inline.hpp from .hpp files
stefan.karlsson at oracle.com
Thu Feb 12 08:24:26 UTC 2015
On 2015-02-12 04:44, Coleen Phillimore wrote:
> Hi Stefan,
> I think these cleanups are great.
> On 2/11/15, 7:38 AM, Stefan Karlsson wrote:
>> Hi all,
>> Please, review this patch to fix our includes of oop.inline.hpp.
>> 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
>> This guide line is documented on:
>> 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
>> Summary of the code changes:
>> - Added to needed .cpp and .inline.hpp files.
>> - This file isn't needed anymore and the contents have been moved to
>> - 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.
>> - 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() ?
- Can be used were we don't care if the function is inlined or not,
- Can be used in hpp files were we don't want to include
- Should be used were we want to give the compiler a chance to inline
- 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.
>> - Moved the VM_GC_Operation destructor to the cpp file.
>> - Moved print_on_error
>> - Moved the inline keyword from the hpp file to the inline.hpp file.
>> - Moved inline_write_ref_field to the inline.hpp file.
>> - Moved obj_at to the inline.hpp file.
>> - 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.
> 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.
More information about the hotspot-dev