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.
