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

[Volker Simonis]

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 :)
>>>
>