Fwd: Code review request: 7067266 Decoder class not properly initialized on Windows
Zhengyu Gu
zhengyu.gu at oracle.com
Thu Jul 21 06:12:24 PDT 2011
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.
> 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.
-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