Request for review(S): 7003554: (tiered) assert(is_null_object() || handle() != NULL) failed: cannot embed null pointer

[Vladimir Kozlov]

Looks good.

vladimir

Igor Veresov wrote:
> Done.
> 
> Webrev: http://cr.openjdk.java.net/~iveresov/7003554/webrev.02/
> 
> igor
> 
> On 12/1/10 9:44 PM, Tom Rodriguez wrote:
>>
>> On Dec 1, 2010, at 9:07 PM, Igor Veresov wrote:
>>
>>> On 12/1/10 8:22 PM, Tom Rodriguez wrote:
>>>>
>>>> On Dec 1, 2010, at 5:42 PM, Igor Veresov wrote:
>>>>
>>>>> C1 with profiling doesn't check whether the MDO has been really 
>>>>> allocated, which can silently fail if the perm gen is full. The 
>>>>> solution is to check if the allocation failed and bailout out of 
>>>>> inlining or compilation.
>>>>
>>>> The logic around build_method_data doesn't look right to me.  Won't 
>>>> it return false if there is already a valid MDO which would cause us 
>>>> to incorrectly bailout?
>>>
>>> Oops :( Right, fixed.
>>>
>>>> Maybe build_method_data should become ensure_method_data and return 
>>>> true if the method has a real MDO, creating it if necessary.
>>>
>>> But that's kind of what it does, except for accessors, etc...? Would 
>>> it be just the name change?
>>
>> Yes it would be just a name change after your fix.  Having a boolean 
>> return from something called build_method_data seems to imply that it 
>> should work the way it did in your original webrev, return true if it 
>> created something.  The name change seems like it would match the 
>> return value better, though I don't feel that strongly about it.
>>
>>>
>>>>
>>>> Shouldn't this assert:
>>>>
>>>> +     assert(md != NULL, "Sanity");
>>>>
>>>> be stronger?  It seems like it should be checking that it's not an 
>>>> empty method data too since those won't have a real object behind 
>>>> them.  Maybe C1 should be using a new method like 
>>>> method_data_or_null() so that it never sees empty method datas.
>>>>
>>>
>>> Good idea, done.
>>
>> Could you put a short comment on method_data_or_null.  Otherwise it 
>> looks good.
>>
>> tom
>>
>>>
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~iveresov/7003554/webrev.01/
>>>
>>> igor
>>>
>>>> tom
>>>>
>>>>>
>>>>>
>>>>> Webrev: http://cr.openjdk.java.net/~iveresov/7003554/webrev.00/
>>>>>
>>>>>
>>>>> Tested with the failing nightly test, specjvm98, and JPRT.
>>>>>
>>>>> Thanks,
>>>>> igor
>>>>
>>>
>>
>