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

David Holmes david.holmes at oracle.com
Mon Jan 11 08:38:11 UTC 2016 

On 11/01/2016 5:56 PM, Volker Simonis wrote:
> On Mon, Jan 11, 2016 at 3:00 AM, David Holmes <david.holmes at oracle.com> wrote:
>> On 8/01/2016 7:31 PM, 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.
>>
>>
>> Won't you get a link time failure in any case ?
>>
>
> With some compilers. But if the method has external linkage the
> compiler usually instantiates a version of the method even if it is
> not called.
>
>>
>> I'm just concerned we are relying on unspecified compiler behaviour which
>> might change in the future. The docs say you don't put "inline" on the
>> declaration. What's to stop the next version of the compiler from flagging
>> that as a warning?
>>
>
> Who did ever ever claime that putting the inline specifier on both,
> definition AND declaration is unspecified compiler behaviour?

That isn't what I meant. The unspecified part is how the compiler 
responds to seeing an inline declaration but not the definition.

> just plain wrong! Please simply read the C++ Standard [1]. It clearly
> states in paragraph 2 of section 9.3: "...An inline member function
> (whether static or non-static) may also be defined outside of its
> class definition provided either its declaration in the class
> definition or its definition outside of the class definition declares
> the function as inline...".

Yes it does - though 7.1.2 also claims "inline" is only valid on a 
declaration. And 9.3 seems at odds with the FAQ referenced as well.

Okay - the main point is that making the declaration inline is legal, so 
no compiler should be rejecting it. Even if it also doesn't provide any 
additional diagnostics.

Thanks for clarifying.

David
-----

> What has been cited here so far (e.g. [2]) is simply an advice (and
> not more!) which shouldn't be confused with the language
> specification! The reason behind the advice to 'place the inline
> specifier on the method definition rather than on its declaration' is
> because the writer deems it an implementation detail. BUT you have to
> take into account that in the HotSpot code base we already have the
> subdivision of header files into "true" .hpp files and .inline.hpp
> files anyway. We've done that because of performance reasons and thus
> already put the burden of deciding which one is required on the user
> anyway. So the argument raised in [2] that the "inlineness" of a
> method is "an implementation detail" doesn't quite hold true for the
> HotSpot code base.
>
> And once again - Goetz's change helps in ensuring that methods which
> are declared inline will be always visible at call sites and thus
> valid inline candidates. If this is not the case we will now (with
> Goetz's change) get a compiler warning where we just silently got a
> plain function call before.
>
> Regards,
> Volker
>
> [1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3242.pdf
> [2] https://isocpp.org/wiki/faq/inline-functions#where-to-put-inline-keyword
>
>> Thanks,
>>
>> David
>>
>>> 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.
>>>>>>>>>
>>

More information about the hotspot-runtime-dev mailing list