Request for review: JDK-8012052: java/lang/invoke/6987555/Test6987555.java crashes with assert(mcs != NULL) failed: MethodCounters cannot be NULL

[Jiangli Zhou]

Hi David,

Please see comments inlined.

On 04/11/2013 08:46 PM, David Holmes wrote:
> Hi Jiangli,
> On 12/04/2013 12:59 PM, Jiangli Zhou wrote:
>> Thanks for the review! Please see comments below regarding
>> build_method_counters().
>>
>> On 04/11/2013 07:43 PM, David Holmes wrote:
>>> Seems quite reasonable.
>>>
>>> Aside: when looking at other uses of method_counters() for missing
>>> NULL checks I noticed:
>>>
>>> MethodCounters* Method::build_method_counters(Method* m, TRAPS) {
>>>   methodHandle mh(m);
>>>   ClassLoaderData* loader_data =
>>> mh->method_holder()->class_loader_data();
>>>   MethodCounters* counters = MethodCounters::allocate(loader_data,
>>> CHECK_NULL);
>>>   if (mh->method_counters() == NULL) {
>>>     mh->set_method_counters(counters);
>>>   } else {
>>>     MetadataFactory::free_metadata(loader_data, counters);
>>>   }
>>>   return mh->method_counters();
>>> }
>>>
>>>
>>> Can't we simplify that to:
>>>
>>> MethodCounters* Method::build_method_counters(Method* m, TRAPS) {
>>>   methodHandle mh(m);
>>>   if (mh->method_counters() == NULL) {
>>>     ClassLoaderData* loader_data =
>>> mh->method_holder()->class_loader_data();
>>>     MethodCounters* counters = MethodCounters::allocate(loader_data,
>>> CHECK_NULL);
>>>     mh->set_method_counters(counters);
>>>   }
>>>   return mh->method_counters();
>>> }
>>
>> We need to recheck if mh->method_counters() is NULL before
>> set_method_counters() because some other thread might have created a
>> MethodCounters object and set to the method after the first NULL check.
>
> If you are worried about concurrency then both versions are broken - 
> all you are doing is narrowing the window with the current version.

Yes, agreed.

The race is rare. If it does happen, the overwritten one would just 
become "orphan".

Thanks,
Jiangli

>
> David
>
>> Thanks,
>> Jiangli
>>
>>>
>>> Thanks,
>>> David
>>>
>>> On 12/04/2013 10:39 AM, Jiangli Zhou wrote:
>>>> Please review the following fix for JDK-8012052
>>>> <https://jbs.oracle.com/bugs/browse/JDK-8012052>:
>>>>
>>>> http://cr.openjdk.java.net/~jiangli/8012052/webrev.00/
>>>>
>>>> The MethodCounters object is not guaranteed to be present for a method
>>>> that is being compiled when 
>>>> NonTieredCompPolicy::delay_compilation() is
>>>> called due to compiler shuts off. In
>>>> java/lang/invoke/6987555/Test6987555 test case, the current method 
>>>> being
>>>> compiled is java.lang.invoke.MethodHandle::linkToStatic(LL), which
>>>> hasn’t been executed before and has no associated MethodCounters at 
>>>> the
>>>> time. Since delay_compilation() decrements the _counter by dividing 2
>>>> only if _counter >0, there is no need to call delay_compilation() when
>>>> the method has no associated MethodCounters.
>>>>
>>>> Thanks,
>>>> Jiangli
>>