RFR(S): 8199698: Change 8199275 breaks template instantiation for xlC (and potentially other compliers)

[coleen.phillimore at oracle.com]

Yes, this change looks really good.
Thanks!
Coleen

On 3/16/18 2:38 PM, Volker Simonis wrote:
> Hi Coleen, Stefan,
>
> I agree that Stefans proposal looks very attractive! I've further
> simplified it by removing the AllocateHeap versions which take a
> nothrow_t argument. We can simlpy call AllocateHeap with the
> corresponding AllocFailStrategy instead. Also we still need to fix the
> NOINLINE macro for xlC:
>
> http://cr.openjdk.java.net/~simonis/webrevs/2018/8199698.v2/
>
> I've build on Linux/x86, Solaris/SPARC and AIX. The NMT tests are
> still passing. Currently the submit-hs job is underway.
>
> What do you think?
>
> Regards,
> Volker
>
> On Fri, Mar 16, 2018 at 3:10 PM,  <coleen.phillimore at oracle.com> wrote:
>> Hi, I've looked at both fixes and I really like Stefan's version. Having new
>> and delete to be trivially inlined in allocation.hpp solves a lot of
>> problems that I've found when removing more .inline.hpp includes from .hpp
>> files (hashtable.inline.hpp from systemDictionary.hpp, I think).
>>
>> Also, rather than NOINLINE, it's a lot nicer to put this in the .cpp file.
>>
>> Thanks,
>> Coleen
>>
>>
>> On 3/16/18 4:18 AM, Stefan Karlsson wrote:
>>> Hi Volker,
>>>
>>> This seems fine to be.
>>>
>>> An alternative fix for the allocation.inline.hpp problem would be to move
>>> the AllocateHeap code into allocation.cpp, and get rid of the NOINLINE
>>> usage.
>>>
>>> I've created a prototype for that:
>>> http://cr.openjdk.java.net/~stefank/8199698/prototypeAllocateHeapInCpp/
>>>
>>> I've visually inspected the output from NMT and it seems to give correct
>>> stack traces. For example:
>>>
>>> [0x00007f4bffa10eff] ObjectSynchronizer::omAlloc(Thread*)+0x3cf
>>> [0x00007f4bffa1244c] ObjectSynchronizer::inflate(Thread*, oopDesc*,
>>> ObjectSynchronizer::InflateCause)+0x8c
>>> [0x00007f4bffa1425a] ObjectSynchronizer::FastHashCode(Thread*,
>>> oopDesc*)+0x7a
>>> [0x00007f4bff616142] JVM_IHashCode+0x52
>>>                               (malloc=4144KB type=Internal #129)
>>>
>>> Thanks,
>>> StefanK
>>>
>>> On 2018-03-15 18:20, Volker Simonis wrote:
>>>> Hi,
>>>>
>>>> can I please have a review for the following small fix:
>>>>
>>>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8199698/
>>>> https://bugs.openjdk.java.net/browse/JDK-8199698
>>>>
>>>> The fix is actually trivial: just defining the corresponding
>>>> "NOINLINE" macro for xlC. Unfortunately it's syntax requirements are a
>>>> little different for method declarations, compared to the other
>>>> platforms (it has to be placed AFTER the method declarator instead of
>>>> BEFORE it). Fortunately, there are no differences for method
>>>> definitions, so we can safely move the NOINLINE attributes from the
>>>> method declarations in allocation.hpp to the method definitions in
>>>> allocation.inline.hpp.
>>>>
>>>> Thank you and best regards,
>>>> Volker
>>>>
>>>> PS: or true C++ enthusiasts I've also included the whole story of why
>>>> this happens and why it happens just now, right after 8199275 :)
>>>>
>>>> Change "8199275: Fix inclusions of allocation.inline.hpp" replaced the
>>>> inclusion of "allocation.inline.hpp" in some .hpp files (e.g.
>>>> constantPool.hpp) by "allocation.hpp".
>>>>
>>>> "allocation.inline.hpp" contains not only the definition of some
>>>> inline methods (as the name implies) but also the definition of some
>>>> template methods (notably the various CHeapObj<>::operator new()
>>>> versions).
>>>>
>>>> Template functions are on orthogonal concept with regard to inline
>>>> functions, but they share on implementation communality: at their call
>>>> sites, the compiler absolutely needs the corresponding function
>>>> definition. Otherwise it can either not inline the corresponding
>>>> function in the case of inline functions or it won't even be able to
>>>> create the corresponding instantiation in the case of a template
>>>> function.
>>>>
>>>> Because of this reason, template functions and methods are defined in
>>>> their corresponding .inline.hpp files in HotSpot (even if they are not
>>>> subject to inlining). This is especially true for the before mentioned
>>>> CHeapObj<>:: new operators, which are explicitly marked as "NOINLINE"
>>>> in allocation.hpp but defined in allocation.inline.hpp.
>>>>
>>>> Now every call site of these CHeapOb<>::new() operators which only
>>>> includes "allocation.hpp" will emit a call to the corresponding
>>>> instantiation of the CHeapObj<>:: new operator, but wont be able to
>>>> actually create that instantiation (simply because it doesn't see the
>>>> corresponding definition in allocation.inline.hpp). On the other side,
>>>> call sites of a CHeapObj<>:: new operator which include
>>>> allocation.inline.hpp will instantiate the required version in the
>>>> current compilation unit (or even inline that method instance if it is
>>>> not flagged as "NOINLINE").
>>>>
>>>> If a compiler doesn't honor the "NOINLINE" attribute (or has an empty
>>>> definition for the NOINLIN macro like xlC), he can potentially inline
>>>> all the various template instances of CHeapObj<>:: new at all call
>>>> sites, if their implementation is available. This is exactly what has
>>>> happened on AIX/xlC before change 8199275 with the effect that the
>>>> resulting object files contained no single instance of the
>>>> corresponding new operators.
>>>>
>>>> After change 8199275, the template definition of the CHeapObj<>:: new
>>>> operators aren't available any more at all call sites (because the
>>>> inclusion of allocation.inline.hpp was removed from some other .hpp
>>>> files which where included transitively before). As a result, the xlC
>>>> compiler will emit calls to the corresponding instantiations instead
>>>> of inlining them. But at all other call sites of the corresponding
>>>> operators, the operator instantiations are still inlined (because xlC
>>>> does not support "NOINLINE"), so we end up with link errors in
>>>> libjvm.so because of missing CHeapObj<>::new instances.
>>>>
>>>>    As a general rule of thumb, we should always make template method
>>>> definitions available at all call sites, by placing them into
>>>> corresponding .inline.hpp files and including them appropriately.
>>>> Otherwise, we might end up without the required instantiations at link
>>>> time.
>>>>
>>>> Unfortunately, there's no compile time check to enforce this
>>>> requirement. But we can misuse the "inline" keyword here, by
>>>> attributing template functions/methods as "inline". This way, the
>>>> compiler will warn us, if a template definition isn't available at a
>>>> specific call site. Of course this trick doesn't work if we
>>>> specifically want to define template functions/methods which shouldn't
>>>> be inlined, like in the current case :)
>>>>