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

Zhengyu Gu zhengyu.gu at oracle.com
Fri Jul 22 07:30:30 PDT 2011


> If you consider it a service with explicit init/uninit then it doesn't 
> really matter whether it is all static or a singleton instance. The 
> issue in question is where the calls to init/uninit get placed. If the 
> service is lazy-initialized then the initialization should be done 
> within the operations that need it. The decision of when to 
> "uninitialize" is more interesting if it is really just a "please free 
> up some resources" request.
>
The advantage of singleton instance is to initialize Decoder at 
Decoder::getInstance(), and not add initialize() call on every method.

>>> Further, if we aren't going to do any cleanup then what is the point 
>>> of having uninitialize ? Seems to be dead code.
>>>
>> There is no reason to uninitialize in this case, but there is a 
>> usecase (after integrates VM diagnostic functions): when VM thinks 
>> that it is running out of memory. Symbol cache can grow substantially 
>> on Unix platforms.
>
> Are you saying that frame decoding will be used in contexts other than 
> as part of fatal error handling? If so then you will need to 
> synchronize access to these resources that can be cleaned up. In which 
> case it it may be better to address this current bug within that 
> larger piece of work.
>
Yes, I don't intend to address those issues with this bug.

Thanks,

-Zhengyu


> David
> -----
>
>>
>> -Zhengyu
>>
>>> David
>>>
>>>
>>>> Regards,
>>>> Volker
>>>>
>>>> On Wed, Jul 20, 2011 at 4:46 PM, Zhengyu Gu <zhengyu.gu at oracle.com> 
>>>> wrote:
>>>>> Hi,
>>>>>
>>>>> Sorry for delay. I am having trouble to upload webrev to
>>>>> cr.openjdk.java.net, probably due to re-ip here.
>>>>>
>>>>> This revision, Decoder class is reverted back to AllStatic.
>>>>> Decoder::initialize() is added at Step 60, and Decoder won't be
>>>>> uninitialized.
>>>>>
>>>>> Here is the link to internal webrev:
>>>>> http://j2se.east.sun.com/~zg131198/7067266/webrev.01/
>>>>>
>>>>> Here is the diff:
>>>>>
>>>>> bash-3.00$ hg diff
>>>>> diff --git a/src/share/vm/utilities/decoder.hpp
>>>>> b/src/share/vm/utilities/decoder.hpp
>>>>> --- a/src/share/vm/utilities/decoder.hpp
>>>>> +++ b/src/share/vm/utilities/decoder.hpp
>>>>> @@ -45,7 +45,7 @@ class ElfFile;
>>>>>  #endif // _WINDOWS
>>>>>
>>>>>
>>>>> -class Decoder: public StackObj {
>>>>> +class Decoder: AllStatic {
>>>>>
>>>>>  public:
>>>>>   // status code for decoding native C frame
>>>>> @@ -61,9 +61,6 @@ class Decoder: public StackObj {
>>>>>   };
>>>>>
>>>>>  public:
>>>>> -  Decoder() { initialize(); };
>>>>> -  ~Decoder() { uninitialize(); };
>>>>> -
>>>>>   static bool can_decode_C_frame_in_vm();
>>>>>
>>>>>   static void initialize();
>>>>> diff --git a/src/share/vm/utilities/vmError.cpp
>>>>> b/src/share/vm/utilities/vmError.cpp
>>>>> --- a/src/share/vm/utilities/vmError.cpp
>>>>> +++ b/src/share/vm/utilities/vmError.cpp
>>>>> @@ -453,6 +453,8 @@ void VMError::report(outputStream* st) {
>>>>>                  );
>>>>>
>>>>>   STEP(60, "(printing problematic frame)")
>>>>> +      // initialize decoder to decode C frames
>>>>> +      Decoder::initialize();
>>>>>
>>>>>      // Print current frame if we have a context (i.e. it's a crash)
>>>>>      if (_context) {
>>>>> @@ -565,9 +567,6 @@ void VMError::report(outputStream* st) {
>>>>>        // see if it's a valid frame
>>>>>        if (fr.pc()) {
>>>>>           st->print_cr("Native frames: (J=compiled Java code, 
>>>>> j=interpreted,
>>>>> Vv=VM code, C=native code)");
>>>>> -
>>>>> -          // initialize decoder to decode C frames
>>>>> -          Decoder decoder;
>>>>>
>>>>>           int count = 0;
>>>>>           while (count++ < StackPrintLimit) {
>>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>> -Zhengyu
>>>>>
>>>>>
>>>>> On 7/18/2011 10:24 AM, Volker Simonis wrote:
>>>>>> 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