Fwd: Code review request: 7067266 Decoder class not properly initialized on Windows
David Holmes
David.Holmes at oracle.com
Thu Jul 21 17:32:27 PDT 2011
Zhengyu Gu said the following on 07/21/11 23:12:
>
> On 7/20/2011 5:29 PM, David Holmes wrote:
>> Volker Simonis said the following on 07/21/11 01:29:
>>> I must say that I in general don't like the idea to have a static
>>> method which is dependent on another static method being called
>>> before. This is just an implicit dependency which is not documented
>>> nor enforced anywhere.
>>>
>>> Your patch may fix the current problem, but what if somebody will use
>>> VMError::print_stack_trace() or frame::print_on_error() in the future.
>>> How can he know that he will have to initialize Decoder first?
>>
>> I have to agree. The initialization code should be called from the
>> methods that need it - as is actually done in one Windows case.
>>
> Then singleton object makes more sense. I still view Decoder as optional
> service, it can be shutdown silently, and output undecoded C frame does
> not imply a bug. We do see the scenarios in nightly tests.
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.
>> 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.
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