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

[Lindenmaier, Goetz]

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.

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.
> >>>>>