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

[David Holmes]

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

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