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

Fri Jan 8 14:59:36 UTC 2016

On 1/8/16 2:31 AM, Lindenmaier, Goetz wrote:
> Hi David,
>
>> I still don't see how adding inline to the declaration changes anything
>> here. The compiler can't do the inlining unless the .inline.hpp file has
>> been included. ???
> Yes, and therefore it issues an error if the keyword is in the .hpp header:
>
> In file included from /sapmnt/home/d045726/oJ/8146401-oopInline-rt-hs-rt/src/share/vm/utilities/hashtable.cpp:27:0:
> /sapmnt/home/d045726/oJ/8146401-oopInline-rt-hs-rt/src/share/vm/classfile/javaClasses.hpp:113:30: error: inline function 'static typeArrayOopDesc* java_lang_String::value(oop)' used but never defined [-Werror]
>     static inline typeArrayOop value(oop java_string);
>
> So you can not miss including the inline.hpp header.

OK so now I'm confused...

You declare a method in the oop.hpp file, but it is not defined
there nor is it defined in the oop.cpp file. It is defined as
"inline" in the oop.inline.hpp file...

If I try to use the method in code that does not include oop.inline.hpp,
then I would expect the compiler to flag that because it can't find
the method. I could swear that I've run into that "problem" before...
and the solution was to add the "oop.inline.hpp" file...

What am I missing?

Dan

>
> Best regards,
>    Goetz.
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Freitag, 8. Januar 2016 08:54
>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>> dev at openjdk.java.net
>> Subject: Re: RFR(L): 8146401: Clean up oop.hpp: add inline directives and fix
>> header files
>>
>>
>>
>> On 8/01/2016 5:28 PM, Lindenmaier, Goetz wrote:
>>>
>>>> -----Original Message-----
>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>> Sent: Freitag, 8. Januar 2016 06:07
>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-runtime-
>>>> dev at openjdk.java.net
>>>> Subject: Re: RFR(L): 8146401: Clean up oop.hpp: add inline directives and
>> fix
>>>> header files
>>>>
>>>> On 8/01/2016 1:01 AM, Lindenmaier, Goetz wrote:
>>>>> Hi David,
>>>>>
>>>>> the documentation you point to makes the point that whether to inline
>>>>> is an implementation detail, and that that should not be annotated to
>>>>> the declaration.  Basically, this is a good point.
>>>>>
>>>>> But the documentation does not cover the fact that
>>>>> we have the implementation in a different file than the declaration.
>>>>> The .inline.hpp is not included always (as you pointed out).
>>>>>
>>>>> Therefore, if we compile with precompiled headers, we get more
>>>>> inlines than if we do so without.
>>>>> Further, many functions were moved to oop.linline.hpp during the
>>>>> recent cleanups, and I think all the #includes I had to add were not
>>>>> left out deliberately, but just happened because the compiler did
>>>>> not complain.
>>>>> I think if somebody decides not to place a function implementation
>>>>> in a .cpp file, it should have the chance to be inlined at all it's calls.
>>>>> Putting the 'inline' into the .hpp file assures this.
>>>> Sorry I don't follow that. If you include the .inline.hpp you don't need
>>>> "inline" in the .hpp. If you don't include it then the compiler doesn't
>>>> have access to the definition so that it can be inlined.
>>> Yes.
>>>
>>>> it seems to me that if
>>>> you want it inlined then you must include the .inline.hpp anywhere it is
>>>> needed. Anything else seems a bad-aid.
>>> But I think not the caller should decide whether it's to be inlined,
>>> but the implementor of the method (and next the compiler).
>>> As you describe, the caller has to decide which header to include.
>>> So the caller has to know implementation details of the callee,
>>> which is contrary to the encapsulation described in that documentation.
>> That's my understanding of the way things are expected to work. This is
>> not a public library exporting its interface in a .hpp file, this an
>> internal part of the bigger system. As I understand it we define the
>> .inline.hpp file precisely so that clients can include it. And yes the
>> clients have to know to do that.
>>
>>> If you add the 'inline' keyword in the header, the compiler enforces
>>> that the includes are in a way that it can do what the implementor
>>> of the callee / of the method to be inlined intended.
>> I still don't see how adding inline to the declaration changes anything
>> here. The compiler can't do the inlining unless the .inline.hpp file has
>> been included. ???
> Yes, but it issues a warning and  will abort if it's wrong.
>
>
>> Cheers,
>> David
>> -----
>>
>>
>>>> Maybe
>>>> precompiled headers messes with that somehow
>>> Actually, you are right.  Oop.inline.hpp is not in precompiled.hpp, nor
>>> is it dragged in by some other .inline.hpp header.
>>> I would assume this is a remnant of the problems with this header.
>>> But if it was listed there, it would make a difference.
>>>
>>> To put it the other way round: do you think all the places I had to
>>> clean up are well founded decisions not to include the methods
>>> from oop.inline.hpp?
>>> Before my change, the caller of obj_at_put() from objArrayOop.hpp
>>> had to include oop.inline.hpp to get it properly inlined.  Seems very
>>> unintuitive to me.
>>> http://cr.openjdk.java.net/~goetz/wr16/8146401-oopInline-
>> rt/webrev.01/src/share/vm/oops/objArrayOop.hpp.udiff.html
>>> (I'll add the inline keyword in a follow-up change for gc files, in case
>>> this change is accepted.)
>>>
>>> Best regards,
>>>     Goetz.
>>>
>>>> Cheers,
>>>> David
>>>>
>>>>> Best regards,
>>>>>      Goetz.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: David Holmes [mailto:david.holmes at oracle.com]
>>>>>> Sent: Mittwoch, 6. Januar 2016 05:59
>>>>>> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; hotspot-
>> runtime-
>>>>>> dev at openjdk.java.net
>>>>>> Subject: Re: RFR(L): 8146401: Clean up oop.hpp: add inline directives
>> and
>>>> fix
>>>>>> header files
>>>>>>
>>>>>> Hi Goetz,
>>>>>>
>>>>>> On 5/01/2016 1: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
>>>>>>
>>>>>> This seems contrary to the C++ FAQ:
>>>>>>
>>>>>> https://isocpp.org/wiki/faq/inline-functions
>>>>>>
>>>>>> The declaration in the .hpp should not have inline, only the definition,
>>>>>> which in our case is in the .inline.hpp file.
>>>>>>
>>>>>> Any code that includes the .inline.hpp will have seen the definition of
>>>>>> the inline function prior to its use - as long as the includes are correct.
>>>>>>
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>
>>>>>>> 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.
>>>>>>>