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