Fwd: Code review request: 7067266 Decoder class not properly initialized on Windows

Volker Simonis volker.simonis at gmail.com
Mon Jul 18 07:24:34 PDT 2011 

On Mon, Jul 18, 2011 at 3:47 PM, Zhengyu Gu <zhengyu.gu at oracle.com> wrote:
> I have to disagree, I would much prefer to revert back it AllStatic
> implementation. Followings are the reasons:
>
> 1. Uninitialize() was intended to shutdown decoder in low memory scenarios,
> as the decoder is a secondary function. It may not be very clear in this
> usecase, but it is a scenario in the project I am currently working on.
>

I don't know what project you are working on, but in the Hotspot as we
can see it, the Decoder::decode() functionality is only accessed from
os::dll_address_to_function_name(). This in turn is only called from
two places:
- from the VM error handler (in vmError.cpp) trough
frame::print_on_error() which calls it trough print_C_frame()
- from the FlatProfiler trough trough FlatProfiler::record_vm_tick()

> 2. Performance: Initial/uninitial decoder for decoding every symbol,
> definitely not optimal. In Unix implementation, it was programmed to
> maintain a symbol cache, which will not completely defeated, as it has to
> reopen/reparse elf file to decoder every frame. Again, decoder performance
> will be one of key factors for my other project.

I didn't propose to create a new Decoder object for the decoding of
each symbol. Instead a decoder object could be passed into
os::dll_address_to_function_name() and  frame::print_on_error(). The
user of the functionality could then decide how long he wants to keep
the Decoder object and its resources alive. In the case of the
FlatProfiler this would probably be the whole VM lifetime, for the
vmError use case it would be probably for the time of a complete stack
trace.

>
> I will post new webrev to reflect AllStatic implementation.
>
> Thanks,
>
> -Zhengyu
>
> On 7/18/2011 5:25 AM, Volker Simonis wrote:
>>
>> What about making the Decoder::decode() and all the other methods in
>> the Decoder class instance methods?
>> Then it would be clear that users of the decode() method would have to
>> create (and initialize) a Decode object and it would be their
>> responsibility when they create it and and when they uninitialize and
>> clean it up. Depending on the platform, the Decoder class may still
>> decide to keep its resources globally (as static data members) or just
>> for the lifetime of a Decoder object.
>>
>> With the current solution the problem is (as pointed out by David)
>> that it is not clear for callers if they have to do initialization or
>> not, and this even differs between Unix and Windows.
>>
>> @Zhengyu: will you prepare a new webrev or should I submit one?
>>
>> Regards,
>> Vovlker
>>
>> On Sat, Jul 16, 2011 at 12:59 AM, David Holmes<David.Holmes at oracle.com>
>>  wrote:
>>>
>>> John Coomes said the following on 07/16/11 07:01:
>>>>
>>>> Zhengyu Gu (zhengyu.gu at oracle.com) wrote:
>>>>>
>>>>> Forward to openjdk for review:
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~zgu/7067266/webrev.00/
>>>>
>>>> The change looks correct.
>>>>
>>>> It's not new in this change, but I find it strange that static
>>>> initialization of Decoder is triggered by creating an unused instance.
>>>> It'd be clearer to call Decoder::initialize() instead, and then
>>>> there'd be no need for the comment:
>>>>
>>>>        // initialize decoder to decode C frames
>>>>
>>>> Decoder should also be changed to inherit from AllStatic instead of
>>>> StackObj, but that's getting beyond this bug fix.
>>>
>>> This is extremely confusing code. We have static data being manipulated
>>> by
>>> stack-scoped local instances, where the constructor does an initialize
>>> function and the destructor does an uninitalize that cleans up various
>>> resources. In addition we have a static function on Windows:
>>>
>>> bool Decoder::can_decode_C_frame_in_vm() {
>>>  initialize();
>>>  return  _can_decode_in_vm;
>>> }
>>>
>>> which initializes but never cleans up! And the use of these two mechanism
>>> can be interspersed.
>>>
>>> To top it off we create the Decoder instances in places where there is
>>> absolutely no indication that somewhere down in the call chain the
>>> Decoder
>>> functionality will be used! This is truly awful code.
>>>
>>> Volker's patch to move the Decoder into print_C_frame would seem much
>>> cleaner, but even it doesn't go far enough as we have redundant
>>> initialize()
>>> in can_decode_C_frame_in_vm, and I don't see why we should be working
>>> with
>>> static functions instead of the Decoder object that was created.
>>>
>>> Zhengyu is concerned that the repeated init/un-init might be too much if
>>> moved into print_C_frame, but his fix goes to the other extreme by giving
>>> the Decoder "global" scope in the error reporting process so we won't do
>>> any
>>> cleanup until right at the end (where we will presumably terminate the
>>> process) - so this seems to defeat the purpose of having the cleanup code
>>> in
>>> the first place.
>>>
>>> Given that we are in the process of dying I don't see that we should be
>>> overly concerned about overhead so I'd be inclined to go with Volker's
>>> fix
>>> and at least do the init and cleanup in the location where the Decoder is
>>> actually used.
>>>
>>> David
>>> -----
>>>
>>>
>>>> -John
>>>>
>>>>> -------- Original Message --------
>>>>> Subject:        Re: Code review request: 7067266 Decoder class not
>>>>> properly initialized on Windows
>>>>> Date:   Fri, 15 Jul 2011 11:52:44 -0600
>>>>> From:   Daniel D. Daugherty<daniel.daugherty at oracle.com>
>>>>> Reply-To:       daniel.daugherty at oracle.com
>>>>> To:     Zhengyu Gu<zhengyu.gu at oracle.com>
>>>>> CC:     hotspot-runtime-dev-confidential
>>>>> <hotspot-runtime-dev-confidential at sun.com>, volker-simonis at gmail.com
>>>>>
>>>>>
>>>>>
>>>>> Thumbs up on the code.
>>>>>
>>>>> Zhengyu, this came in via hotspot-runtime-dev at openjdk.java.net
>>>>> so this review should go out on that alias also (and the webrev
>>>>> copied to cr.openjdk.java.net)
>>>>>
>>>>> Dan
>>>>>
>>>>>
>>>>> On 7/15/11 11:26 AM, Zhengyu Gu wrote:
>>>>>>
>>>>>> This is a simple fix that moves Decoder initialization to step 60 in
>>>>>> vmError.cpp, so the decoder can decode "Problematic frame:" in hs file
>>>>>> header.
>>>>>>
>>>>>> The bug was reported by volker-simonis at gmail.com
>>>>>> <mailto:volker-simonis at gmail.com>  from SAP with proposed patch. The
>>>>>> solution
>>>>>> was to move decoder initialization code to
>>>>>> print_C_frame() function, but it is less optimal, since decoder will
>>>>>> have to be initialized and uninitialized every time in/out of the
>>>>>> print_C_frame() function.
>>>>>>
>>>>>> CR: http://monaco.sfbay.sun.com/detail.jsf?cr=7067266
>>>>>> Webrev: http://j2se.east.sun.com/~zg131198/7067266/webrev/
>>>>>>
>>>>>> Testcase:
>>>>>>  A simple Java program with JNI call that crashes VM:
>>>>>>
>>>>>> JNIEXPORT void JNICALL Java_jni_1excpt_Main_crashVM (JNIEnv *, jclass)
>>>>>> {
>>>>>>  int* p = NULL;
>>>>>>  *p = 1;
>>>>>> }
>>>>>>
>>>>>> Without fix, hs file header:
>>>>>>
>>>>>> #
>>>>>> # A fatal error has been detected by the Java Runtime Environment:
>>>>>> #
>>>>>> #  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x5b431308, pid=7860,
>>>>>> tid=4140
>>>>>> #
>>>>>> # JRE version: 6.0_23-b05
>>>>>> # Java VM: Java HotSpot(TM) Client VM (19.0-b09 mixed mode, sharing
>>>>>> windows-x86 )
>>>>>> # Problematic frame:
>>>>>> # C  [mydll.dll+0x11308]
>>>>>> #
>>>>>> # If you would like to submit a bug report, please visit:
>>>>>> # http://java.sun.com/webapps/bugreport/crash.jsp
>>>>>> # The crash happened outside the Java Virtual Machine in native code.
>>>>>> # See problematic frame for where to report the bug.
>>>>>> #
>>>>>>
>>>>>>
>>>>>> With fix:
>>>>>>
>>>>>> #
>>>>>> # A fatal error has been detected by the Java Runtime Environment:
>>>>>> #
>>>>>> #  EXCEPTION_ACCESS_VIOLATION (0xc0000005) at pc=0x5c981308, pid=1652,
>>>>>> tid=5964
>>>>>> #
>>>>>> # JRE version: 6.0_23-b05
>>>>>> # Java VM: Java HotSpot(TM) Client VM (21.0-b16-fastdebug mixed mode
>>>>>> windows-x86 )
>>>>>> # Problematic frame:
>>>>>> # C  [mydll.dll+0x11308]  Java_jni_1excpt_Main_crashVM+0x28
>>>>>> #
>>>>>> # Failed to write core dump. Minidumps are not enabled by default on
>>>>>> client versions of Windows
>>>>>> #
>>>>>> # If you would like to submit a bug report, please visit:
>>>>>> # http://bugreport.sun.com/bugreport/crash.jsp
>>>>>> # The crash happened outside the Java Virtual Machine in native code.
>>>>>> # See problematic frame for where to report the bug.
>>>>>> #
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>
>

More information about the hotspot-runtime-dev mailing list