Fwd: Code review request: 7067266 Decoder class not properly initialized on Windows
David Holmes
David.Holmes at oracle.com
Sun Jul 24 19:14:15 PDT 2011
Zhengyu Gu said the following on 07/23/11 00:30:
>
>> 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.
True.
>>>> 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.
Then I suggest, again, simply leaving this bug and dealing with it as
part of this larger project.
Cheers,
David
> 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