RFR 8076212: AllocateHeap() and ReallocateHeap() should be inlined.
David Holmes
david.holmes at oracle.com
Mon Mar 30 02:17:34 UTC 2015
Changing subject to reflect proper RFR.
This change looks good to me.
Thanks,
David
On 28/03/2015 2:43 PM, Yasumasa Suenaga wrote:
> Sorry for the delay.
>
> I filed it to JBS and uploaded webrev:
>
> JDK-8076212: AllocateHeap() and ReallocateHeap() should be inlined.
> http://cr.openjdk.java.net/~ysuenaga/JDK-8076212/webrev.00/
>
> Could you review it?
>
>
>> 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.
> I'll send it to you after reviewing.
>
>
> Yasumasa
>
>
> On 2015年03月16日 09:44, David Holmes wrote:
>> 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
>>>>>>
>>>>>>
More information about the hotspot-runtime-dev
mailing list