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

[Zhengyu Gu]

Withdraw this code review request. Coleen is now the owner of this bug.

Thanks,

-Zhengyu

On 1/8/2014 11:13 AM, Coleen Phillimore wrote:
>
> 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
>>>>
>>
>