RFR(L): 8146401: Clean up oop.hpp: add inline directives and fix header files
David Holmes
david.holmes at oracle.com
Wed Jan 13 12:25:59 UTC 2016
On 13/01/2016 9:13 PM, Lindenmaier, Goetz wrote:
> Hi David,
>
> do I understand this correctly that you are fine with this change?
Yes.
Thanks.
David
> Thanks,
> Goetz.
>
>> -----Original Message-----
>> From: David Holmes [mailto:david.holmes at oracle.com]
>> Sent: Montag, 11. Januar 2016 09:38
>> To: Volker Simonis <volker.simonis at gmail.com>
>> Cc: 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 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