inlining AllocateHeap()

[David Holmes]

On 14/03/2015 9:29 AM, Coleen Phillimore wrote:
>
> There are other inline and noinline directives in allocation.hpp. We
> always assume that AllocateHeap and others are inlined.  NMT is touchy
> with respect to how it walks the stack and it took a bit of work and
> testing to get just the most useful frames saved.  I don't really want
> to risk this breaking!
>
> I think the gcc directive is acceptable in this case.

Okay I'll follow Coleen's guidance on this. The original patch is fine.

Yasumasa you will need to file a CR and you will need a sponsor to push 
your changeset through JPRT once you have created it. I can do the 
latter, just email me the final changeset directly.

Thanks,
David

> Coleen
>
>
> On 3/13/15, 9:16 AM, Yasumasa Suenaga wrote:
>> Hi,
>>
>>> That would require more significant changes to NMT I think
>>
>> I think two changes:
>>
>>  1. Remove AllocateHeap(size_t, MEMFLAGS, AllocFailType) .
>>  2. Add "const NativeCallStack&" to argument of ReallocateHeap() .
>>
>> I think that caller of AllocateHeap() and ReallocateHeap() should give
>> PC to them.
>> However, it is significant changes.
>> Thus I proposed to add always_inline .
>>
>>
>>> I don't see how it will help if you have to know a-priori whether
>>> inlining has occurred or not. ??
>>
>> I think we can use SA.
>> In case of Linux,
>> sun.jvm.hotspot.debugger.linux.LinuxDebuggerLocal#lookup()
>> can lookup symbol from target process - we can check whether the
>> function has been
>> inlined (cannot lookup) or not (can lookup).
>> So I think that we can write jtreg testcase.
>>
>> BTW, should I file it to JBS?
>>
>>
>> Thanks,
>>
>> Yasumasa
>>
>>
>> On 2015/03/13 17:35, David Holmes wrote:
>>> On 13/03/2015 6:13 PM, Thomas Stüfe wrote:
>>>> Hi Yasumasa, David,
>>>>
>>>> Maybe it would make sense to make the
>>>> number-of-frames-to-skip-parameter
>>>> configurable?
>>>
>>> That would require more significant changes to NMT I think - plus I
>>> don't see how it will help if you have to know a-priori whether
>>> inlining has occurred or not. ??
>>>
>>> Thanks,
>>> David
>>>
>>>> Because the direct caller of AllocateHeap or os::malloc may also not be
>>>> interesting but still a generic wrapper. So, the user doing the
>>>> allocation trace could finetune this parameter to fit his needs.
>>>>
>>>> Thomas
>>>>
>>>>
>>>>
>>>> On Fri, Mar 13, 2015 at 6:40 AM, David Holmes <david.holmes at oracle.com
>>>> <mailto:david.holmes at oracle.com>> wrote:
>>>>
>>>>     Hi Yasumasa,
>>>>
>>>>     On 12/03/2015 9:58 PM, Yasumasa Suenaga wrote:
>>>>
>>>>         Hi all,
>>>>
>>>>         I tried to use NMT with details option on OpenJDK7 on RHEL6.6,
>>>>         but I got
>>>>         address at AllocateHeap() as malloc() caller.
>>>>
>>>>         I checked symbol in libjvm.so <http://libjvm.so/> in
>>>>         OracleJDK8u40 Linux
>>>>         x64, it has AllocateHeap()
>>>>         symbol.
>>>>
>>>>         AllocateHeap() is defined as inline function, and it gives
>>>>         CURRENT_PC to
>>>>         os::malloc(). I guess that implementation expects
>>>> AllocateHeap()
>>>>         will be
>>>>         inlined.
>>>>
>>>>
>>>>     It seems so.
>>>>
>>>>         It may occur with GCC (g++) optimization only, however I
>>>> want to
>>>>         fix it to
>>>>         analyze native memory with NMT on Linux.
>>>>
>>>>
>>>>     According to the docs [1]:
>>>>
>>>>     "GCC does not inline any functions when not optimizing unless you
>>>>     specify the ‘always_inline’ attribute for the function"
>>>>
>>>>         I applied patch as below. This patch makes AllocateHeap() as
>>>> inline
>>>>         function.
>>>>         --------------
>>>>         diff -r af3b0db91659
>>>> src/share/vm/memory/__allocation.inline.hpp
>>>>         --- a/src/share/vm/memory/__allocation.inline.hpp Mon Mar 09
>>>>         09:30:16 2015
>>>>         -0700
>>>>         +++ b/src/share/vm/memory/__allocation.inline.hpp Thu Mar 12
>>>>         20:45:57 2015
>>>>         +0900
>>>>         @@ -62,11 +62,18 @@
>>>>              }
>>>>              return p;
>>>>         }
>>>>         +
>>>>         +#ifdef __GNUC__
>>>>         +__attribute__((always_inline)__)
>>>>         +#endif
>>>>
>>>>
>>>>     I dislike seeing the gcc specific directives in common code. I'm
>>>>     wondering whether we should perhaps only use CURRENT_PC in product
>>>>     (and optimized?) builds and use CALLER_PC otherwise. That would be
>>>>     imperfect of course It also makes me wonder whether the inlining is
>>>>     occurring as expected on other platforms.
>>>>
>>>>     I'd like to get other people's views on this.
>>>>
>>>>     Thanks,
>>>>     David
>>>>
>>>>     [1] https://gcc.gnu.org/__onlinedocs/gcc/Inline.html
>>>>     <https://gcc.gnu.org/onlinedocs/gcc/Inline.html>
>>>>
>>>>
>>>>         inline char* AllocateHeap(size_t size, MEMFLAGS flags,
>>>>                AllocFailType alloc_failmode =
>>>> AllocFailStrategy::EXIT_OOM) {
>>>>              return AllocateHeap(size, flags, CURRENT_PC,
>>>> alloc_failmode);
>>>>         }
>>>>
>>>>         +#ifdef __GNUC__
>>>>         +__attribute__((always_inline)__)
>>>>         +#endif
>>>>         inline char* ReallocateHeap(char *old, size_t size, MEMFLAGS
>>>> flag,
>>>>                AllocFailType alloc_failmode =
>>>> AllocFailStrategy::EXIT_OOM) {
>>>>              char* p = (char*) os::realloc(old, size, flag,
>>>> CURRENT_PC);
>>>>         --------------
>>>>
>>>>         If this patch is accepted, I will file it to JBS and will
>>>> upload
>>>>         webrev.
>>>>
>>>>         Thanks,
>>>>
>>>>         Yasumasa
>>>>
>>>>
>