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

[David Holmes]

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.

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
>