RFR JDK-8028275: Metaspace ShrinkGrowTest causes fatal error if run with JFR

[Coleen Phillimore]

On 01/08/2014 08:55 AM, Zhengyu Gu wrote:
>
> On 1/8/2014 12:22 AM, David Holmes wrote:
>> On 8/01/2014 2:59 AM, Coleen Phillimore wrote:
>>>
>>> I vote that we clean it up rather than adding a special exception mark.
>>
>> Do you mean clean up the whole thing? That's a big(ger) job.
>>
>> I'm not a fan of a special exception mark though as I think this 
>> would be the single usage for it - and unless we add a regular 
>> ExceptionMark to cover the code after is_init_completed() then we may 
>> as well just have moved the original scope as I suggested.
> I believe that is_init_completed() check is only applied to this 
> scenario anyway. By refactoring ExceptionMark, we should be able to 
> remove is_init_completed() check. Instead, an additional parameter is 
> passed to ExceptionMark to indicate that it is initialization phase 
> (default to false), and pending exception will cause its dstor to exit 
> vm. Otherwise, pending exception in dstor cause fatal error.
>
> So the regular EXCEPTION_MARK stays the same, and INIT_EXCEPTION_MARK 
> passes initialization phase parameter = true.

I think Threads::create_vm() should be refactored so that the 
is_init_completed() check in the ExceptionMark destructor can be 
removed.  I don't think adding logic to exception mark or creating a new 
one would increase clarity of this code.

Coleen
>
>
> Thanks,
>
> -Zhengyu
>
>
>
>
>
>>
>> David
>>
>>> Coleen
>>>
>>> On 01/07/2014 09:09 AM, Zhengyu Gu wrote:
>>>> I wonder a special INIT_EXCPTION_MARK, which always takes
>>>> is_init_completed() == false path, a better solution? We eventually
>>>> have to clean up the error handling code here if we want to recover
>>>> init failure from this routine.
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>>
>>>> On Jan 6, 2014, at 9:49 PM, David Holmes wrote:
>>>>
>>>>> On 7/01/2014 6:48 AM, Zhengyu Gu wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> Thanks for the review.
>>>>>>
>>>>>> ExceptionMark's destructor only report pending exception as fatal
>>>>>> error after initialize is done (is_init_completed() == true),
>>>>>> otherwise, it will call vm_exit_during_initialization(), just as you
>>>>>> suggested.  Only after set_init_completed() is called (thread.cpp
>>>>>> #3568), pending exception has to be checked.
>>>>> Right - this subtlety is often overlooked. Though perhaps the simpler
>>>>> fix is to reduce the scope of the ExceptionMark so that it is
>>>>> "destructed" before is_init_completed() is called ? What you have is
>>>>> consistent with other error handling code so I'm okay with it as-is.
>>>>> Though a comment re CHECK and the ExceptionMark might be useful after
>>>>> set_init_completed().
>>>>>
>>>>> (All this initialization error handling is an awful mess regardless.
>>>>> It should be CHECK_JNIERR not CHECK_0.)
>>>>>
>>>>> David
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Jan 6, 2014, at 3:37 PM, Coleen Phillimore wrote:
>>>>>>
>>>>>>> Zhengyu,
>>>>>>>
>>>>>>> There are other calls to initialize_class in Threads::create_vm()
>>>>>>> that can create a pending exception and return with CHECK_0 after
>>>>>>> the EXCEPTION MARK.
>>>>>>>
>>>>>>> It seems to me that the EXCEPTION_MARK should be removed and the
>>>>>>> caller of this function checks for the pending exception.   Better
>>>>>>> yet would be to encapsulate the calls to initialize_class into a
>>>>>>> function(s) and check the return for pending exception and exit the
>>>>>>> jvm with jvm_exit_during_initialization().
>>>>>>>
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 01/06/2014 10:55 AM, Zhengyu Gu wrote:
>>>>>>>> This is another bug related on using EXCEPTION_MARK and CHECK
>>>>>>>> macros. JSR292 initialization is done after init_completed flag is
>>>>>>>> set to true, so any pending exceptions from initializing JSR292
>>>>>>>> classes can cause CHECK macro to return, and trigger the assertion
>>>>>>>> in ExceptionMark destructor.
>>>>>>>>
>>>>>>>>
>>>>>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8028275
>>>>>>>> Webrev: http://cr.openjdk.java.net/~zgu/8028275/webrev.00/
>>>>>>>>
>>>>>>>> Test:
>>>>>>>>    Tested the fix on reported platform.
>>>>>>>>
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> -Zhengyu
>>>
>