RFR(L): 8146401: Clean up oop.hpp: add inline directives and fix header files

Coleen Phillimore coleen.phillimore at oracle.com
Fri Jan 8 22:21:45 UTC 2016


Goetz,
Thanks for the reply.

On 1/7/16 10:41 AM, Lindenmaier, Goetz wrote:
> Hi Coleen,
>
> thanks for looking at this!
>
>> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-
>> rt/webrev.01/src/share/vm/code/dependencies.cpp.udiff.html
>>
>> Can you fix the preexisting weird indentation of
>> + assert(_call_site() ->is_a(SystemDictionary::CallSite_klass()), "must
>> be");
> Done.
>
>> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-
>> rt/webrev.01/src/share/vm/jvmci/jvmciJavaClasses.hpp.udiff.html
>>
>> Don't have an opinion about including the .inline.hpp files in these.
>> The compiler team should probably clean this up.
> Yes, I think so too.  It should be done in the compiler repo to avoid
> clashes.
>
>> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-
>> rt/webrev.01/src/share/vm/oops/objArrayOop.hpp.udiff.html
>>
>> Why isn't inline in the declaration for obj_at_put ?
>> Why is inline commented out for some?
> I have a follow-up change with a lot of cleanups in gc coding.
> In that change, I will fix the remaining inlines.  I wanted to
> make an extra change for gc to simplify reviewing.

Ah, thank you for explaining this mystery.  I couldn't think of why this 
was.
>   
>> + inline bool is_instance() const;
>> + /*inline*/ bool is_array() const;
>>
>>
>> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-
>> rt/webrev.01/src/share/vm/oops/oop.inline.hpp.udiff.html
>>
>> I'm not really happy with the re-sorting because it may make backporting
>> difficult, but it's not that many that were moved and the order of these
>> functions has always seeemed random to me.  So I guess it's ok.
>>
>> Can you line up the '\' in the OOP_ITERATE macros at the end?
>>
>> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-
>> rt/webrev.01/src/share/vm/oops/typeArrayOop.cpp.udiff.html
> I think that's an artefact of udiff.  The patch and my files look fine.
>
>> This non-inline function could affect performance.   It may be called
>> from GC.  We might need a typeArrayOop.inline.hpp for it.
> I'll make a new file to be on the safe side.
>
> Why is there the empty typeArrayOop.cpp file?
>   

Oh, heaven knows why there was an empty file!   Maybe an artifact of 
includeDB_junk.    Thanks for making this change.

>> I can sponsor this, pending further review.
> Thanks!

You have replied to Dan and David.  I agree with you that this is a 
worthwhile change.  I think there are some platforms (windows maybe?) 
that will just call 0 and crash if the definition of a function is 
missing, which you can do with a missing .inline.hpp include.   More 
compile time diagnostics makes the code less difficult to work on and 
more robust.

Thanks,
Coleen

>
> Best regards,
>     Goetz.
>
>
>
>> Coleen
>>
>>
>> On 1/4/16 10:44 AM, Lindenmaier, Goetz wrote:
>>> Hi,
>>>
>>> Several recent changes cleaned up includes of oop.inline.hpp in real .hpp
>> header file.
>>> Unfortunately, the 'inline' qualifier is added to the function
>> implementations
>>> in oop.inline.hpp instead of to the declarations in oop.hpp. Therefore, the
>>> compiler can not detect failing inlines properly.
>>>
>>> This change moves the inline directive from oop.inline.hpp to oop.hpp.
>> Also
>>> it sorts the methods in oop.inline.hpp as they are sorted in oop.hpp.
>>>
>>> Further, it moves a row of calls from hpp files to inline.hpp or .cpp files.
>>>
>>> I put oop.inline.hpp into serviceUtil.hpp.  This is not clean, but this is a
>>> very small .hpp file and no .cpp file exists.  So I think this is acceptable.
>>>
>>> Also, I put oop.inline.hpp into jvmciJavaClasses.hpp.  I don't want to do
>>> bigger changes to this file in the rt repo, because jvmci is subject to
>>> freqent changes recently.
>>>
>>> The following methods were moved to .cpp files:
>>>
>>> ProtectionDomainCacheTable::compute_hash()
>>> ProtectionDomainCacheTable::index_for()
>>> typeArrayOopDesc::object_size()
>>> This is called only once outside .cpp file:
>>> CallSiteDepChange::CallSiteDepChange()
>>> This is only called in .cpp file
>>> ConstantPool::string_at_put()
>>>
>>> If someone considers not inlining these harmful to performance,
>>> I will add a new .inline.hpp file for them.
>>>
>>> Please review this change.  I please need a sponsor.
>>> There are no functional edits, so it should be simple to review.
>>> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-rt/webrev.01/
>>>
>>> Best regards,
>>>     Goetz.



More information about the hotspot-runtime-dev mailing list