PING: RFR: JDK-8076212: AllocateHeap() and ReallocateHeap() should be inlined.
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Apr 27 17:35:43 UTC 2015
Hi,
This change looks fine. I thought it was reviewed and committed. I will
sponsor it if needed.
Coleen
On 4/5/15, 8:49 AM, Yasumasa Suenaga wrote:
> Hi all,
>
> I need more reviewer.
> Could you review it?
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8076212/webrev.00/
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2015/03/28 13:43, 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