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

[Zhengyu Gu]

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.

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
>>