Request for review:JDK-8013036:vm/runtime/simpleThresholdPolicy.cpp: assert(mcs != NULL)

[Jiangli Zhou]

Hi Vladimir,

On 04/26/2013 11:07 AM, Vladimir Kozlov wrote:
> On 4/26/13 10:35 AM, Christian Thalinger wrote:
>>
>> On Apr 25, 2013, at 9:49 AM, Jiangli Zhou <jiangli.zhou at oracle.com> 
>> wrote:
>>
>>> Hi Vladimir and Christian,
>>>
>>> When TieredCompilation is enabled, in 
>>> InterpreterGenerator::generate_counter_incr() 
>>> (templateInterpreter_sparc.cpp, line 300~315), it increments the 
>>> MethodData::invocation_counter only and done. The invocation_counter 
>>> originally in Method, now in MethodCounters is never updated if 
>>> MethodData already exists, hence the MethodCounters object is never 
>>> created. The MethodCounters is only created when one of the counter 
>>> fields is updated the first time in order to save memory.
>>
>> The change is okay but the if is wrong (you probably already noticed 
>> that):
>>
>> +  if (mcs != NULL, "") {
>>
>> Did you see the RFE John filed?
>>
>> 8013169: consolidate MethodData and MethodCounters pointers in Method 
>> struct
>
> MethodData is created much later in non tiered case after 3000 
> invocations. And we need method_counters almost immediately during 
> method first execution.

Yes!

>
>>
>> It might help to avoid such cases.
>>
>> One thing I didn't see in the comments for 8010862, why is there a 
>> separate data structure for counters?  What's the benefit?
>
> It saves space for methods which never executed. But I am surprised 
> that Jiangli decided to do such very complex changes (and bug prone) 
> which allocates counters structure only on first counter increment in 
> template interpreter code on all platforms instead of in one place in 
> Interpreter where we invoked method first time.

The changes are indeed bug prone and complex. I explored the possibility 
of allocating the counters data structure in a centralized place but had 
issues with virtual invocations...

Thanks,
Jiangli

>
> Vladimir
>
>>
>> -- Chris
>>
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>
>>> On 04/24/2013 09:11 PM, Christian Thalinger wrote:
>>>> On Apr 24, 2013, at 8:44 PM, Vladimir Kozlov 
>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>
>>>>> So the question is why method_counters is NULL? It should not 
>>>>> happen if method is executed, they should be allocated at that point.
>>>> I agree.  We need to find out why it's null.
>>>>
>>>> -- Chris
>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 4/24/13 8:16 PM, Jiangli Zhou wrote:
>>>>>> Please review the fix for JDK-8013036
>>>>>> <https://jbs.oracle.com/bugs/browse/JDK-8013036>:
>>>>>>
>>>>>> http://cr.openjdk.java.net/~jiangli/8013036/webrev.00/
>>>>>>
>>>>>> The method_counters could be NULL when
>>>>>> SimpleThresholdPolicy::handle_counter_overflow is called under
>>>>>> TieredCompilation. The assert should be changed to be an 'if' 
>>>>>> check instead.
>>>>>>
>>>>>> Thanks,
>>>>>> Jiangli
>>>
>>