PING: RFR: JDK-8151815: Could not parse core image with JSnap.

[Dmitry Samersoff]

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" <yasuenag at gmail.com
>>>>>>>> <mailto:yasuenag at 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.