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

[Jiangli Zhou]

Hi Christian,

On 04/26/2013 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, "") {

Thanks very much for the review. Yes, I fixed in the code. Sorry the 
webrev was not updated properly. :( Just updated the webrev.

>
> Did you see the RFE John filed?

Yes.

>
> 8013169: consolidate MethodData and MethodCounters pointers in Method struct
>
> 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?

During the investigation of 8010862, I noticed the MethodData might not 
have the same life span as the Method interpreter counters. In some 
cases, for example when TieredCompilation is not enabled, the MethodData 
is not allocated until the interpreter counter reaches 
InterpreterProfileLimit. Allocating the MethodData early would waste 
memory for non-hot methods which only execute a few times. Also the 
MethodData is only allocated when ProfileInterpreter is enabled, which 
is the default case for c2 but not for c1. Those were the reasons why a 
separate data structure was chosen for the Method interpreter counters. 
Thank you for pointing out the missing elements in 8010862 description 
and comments, I will add those info.

The new RFE John filed brought up a very good question. Is it possible 
to consolidate the interpret counters and the counters in the 
MethodData? It worth further investigation to improve profiling and 
eliminate possible redundancy.

Thanks,
Jiangli
>
> -- 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