Yasumasa, I need some more time to check what is happening with performance counters and what side effect this fix can have.
Sorry about it. -Dmitry On 2016-04-20 15:14, Yasumasa Suenaga wrote: > PING: Could you review it? > >> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/ > > This changes is based on jdk9/hs-rt. > But I confirmed this patch works fine on jdk9/hs . > > > Thanks, > > Yasumasa > > > On 2016/04/13 22:22, Yasumasa Suenaga wrote: >> Hi Dmitry, >> >> Thank you for your comment. >> I uploaded new webrev. Could you review again? >> >> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.02/ >> >> >> Thanks, >> >> Yasumasa >> >> >> On 2016/04/13 21:41, Dmitry Samersoff wrote: >>> Yasumasa, >>> >>> Yes. It's better. >>> >>> Please place all _saved_* declarations together and add a comment >>> explaining what is the purpose of this variables. >>> >>> -Dmitry >>> >>> >>> On 2016-04-13 15:20, Yasumasa Suenaga wrote: >>>> Hi all, >>>> >>>> Could you review and sponsor it? >>>> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/ >>>> >>>> If it is not accepted, I think that we can add new field to PerfMemory >>>> for using in JSnap: >>>> ----------------------- >>>> diff -r 4823056a5bbd src/share/vm/runtime/perfMemory.cpp >>>> --- a/src/share/vm/runtime/perfMemory.cpp Tue Apr 12 09:08:48 >>>> 2016 +0000 >>>> +++ b/src/share/vm/runtime/perfMemory.cpp Wed Apr 13 14:18:15 >>>> 2016 +0900 >>>> @@ -45,11 +45,16 @@ >>>> UINT_CHARS + 1; >>>> >>>> char* PerfMemory::_start = NULL; >>>> +char* PerfMemory::_saved_start = NULL; >>>> char* PerfMemory::_end = NULL; >>>> +char* PerfMemory::_saved_end = NULL; >>>> char* PerfMemory::_top = NULL; >>>> +char* PerfMemory::_saved_top = NULL; >>>> size_t PerfMemory::_capacity = 0; >>>> +size_t PerfMemory::_saved_capacity = 0; >>>> jint PerfMemory::_initialized = false; >>>> PerfDataPrologue* PerfMemory::_prologue = NULL; >>>> +PerfDataPrologue* PerfMemory::_saved_prologue = NULL; >>>> >>>> void perfMemory_init() { >>>> >>>> @@ -103,6 +108,8 @@ >>>> >>>> // allocate PerfData memory region >>>> create_memory_region(capacity); >>>> + _saved_start = _start; >>>> + _saved_capacity = _capacity; >>>> >>>> if (_start == NULL) { >>>> >>>> @@ -132,8 +139,11 @@ >>>> } >>>> >>>> _prologue = (PerfDataPrologue *)_start; >>>> + _saved_prologue = _prologue; >>>> _end = _start + _capacity; >>>> + _saved_end = _end; >>>> _top = _start + sizeof(PerfDataPrologue); >>>> + _saved_top = _top; >>>> } >>>> >>>> assert(_prologue != NULL, "prologue pointer must be initialized"); >>>> ----------------------- >>>> >>>> If it is better, I will upload a webrev. >>>> >>>> >>>> Thanks, >>>> >>>> Yasumasa >>>> >>>> >>>> On 2016/04/06 12:42, Yasumasa Suenaga wrote: >>>>> Hi Dmitry, >>>>> >>>>> Thanks for your comment. >>>>> >>>>> I uploaded a new webrev. Could you review again? >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.01/ >>>>> >>>>> On 2016/04/06 0:31, Dmitry Samersoff wrote: >>>>>> Yasumasa, >>>>>> >>>>>> 1. It's better to change JSnap code to produce meaningful error >>>>>> message >>>>>> instead of NPE. >>>>> >>>>> I added null check and set message to PerfMemory.java . >>>>> >>>>> >>>>>> 2. We should check that no other consumer of perf counters rely on >>>>>> the >>>>>> fact it's NULL after call to destroy(). I'm not sure that this >>>>>> part of >>>>>> the fix is not dangerous. >>>>> >>>>> I added new flag (_destroyed) in PerfMemory class. >>>>> This flag set true at PerfMemory::destroy(). >>>>> >>>>> _start, _end, and more field which I removed from destroy() are >>>>> private member of >>>>> PerfMemory. I implemented that the getter functions of them check >>>>> _destroyed flag, >>>>> and returns same value before this change. >>>>> >>>>> So I think this change does not affect other place. >>>>> >>>>> >>>>> Thanks, >>>>> >>>>> Yasumasa >>>>> >>>>> >>>>>> -Dmitry >>>>>> >>>>>> On 2016-03-29 15:02, Yasumasa Suenaga wrote: >>>>>>> PING: Could you review it? >>>>>>> >>>>>>> >>>>>>> Yasumasa >>>>>>> >>>>>>> >>>>>>> On 2016/03/22 21:24, Yasumasa Suenaga wrote: >>>>>>>> PING: Could you review it? >>>>>>>> >>>>>>>> Yasumasa >>>>>>>> >>>>>>>> 2016/03/14 23:39 "Yasumasa Suenaga" <yasue...@gmail.com >>>>>>>> <mailto:yasue...@gmail.com>>: >>>>>>>> >>>>>>>> Hi all, >>>>>>>> >>>>>>>> When I use `jhsdb jsnap` to get PerfCounter from core >>>>>>>> images, I >>>>>>>> encountered NPE: >>>>>>>> ------------- >>>>>>>> Exception in thread "main" java.lang.NullPointerException >>>>>>>> at sun.jvm.hotspot.tools.JSnap.run(JSnap.java:45) >>>>>>>> at >>>>>>>> sun.jvm.hotspot.tools.Tool.startInternal(Tool.java:260) >>>>>>>> at sun.jvm.hotspot.tools.Tool.start(Tool.java:223) >>>>>>>> at sun.jvm.hotspot.tools.Tool.execute(Tool.java:118) >>>>>>>> at sun.jvm.hotspot.tools.JSnap.main(JSnap.java:67) >>>>>>>> at >>>>>>>> sun.jvm.hotspot.SALauncher.runJSNAP(SALauncher.java:352) >>>>>>>> at >>>>>>>> sun.jvm.hotspot.SALauncher.main(SALauncher.java:404) >>>>>>>> ------------- >>>>>>>> >>>>>>>> PerfMemory::destroy() clears all members which are used in >>>>>>>> JSnap. >>>>>>>> Thus NPE is occurred. >>>>>>>> >>>>>>>> I uploaded webrev for this issue. >>>>>>>> Could you review it? >>>>>>>> >>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8151815/webrev.00/ >>>>>>>> >>>>>>>> I cannot access JPRT. >>>>>>>> So I need a Sponsor. >>>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Yasumasa >>>>>>>> >>>>>> >>>>>> >>> >>> -- Dmitry Samersoff Oracle Java development team, Saint Petersburg, Russia * I would love to change the world, but they won't give me the sources.
