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