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

[Vladimir Kozlov]

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.

>
> 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.

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
>>
>