RFR(L): 8146401: Clean up oop.hpp: add inline directives and fix header files
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Mon Jan 11 08:13:23 UTC 2016
Hi,
thanks Volker, I was just seacrching that reference in the C++ standard that
inline can go to the declaration.
Best regards,
Goetz.
> -----Original Message-----
> From: Volker Simonis [mailto:volker.simonis at gmail.com]
> Sent: Monday, January 11, 2016 8:57 AM
> To: David Holmes <david.holmes at oracle.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 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's
> 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...".
>
> 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