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

Zhengyu Gu zhengyu.gu at oracle.com
Mon Jul 18 06:47:57 PDT 2011 

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.

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