RFR(L): 8146401: Clean up oop.hpp: add inline directives and fix header files
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Wed Jan 13 11:13:25 UTC 2016
Hi David,
do I understand this correctly that you are fine with this change?
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