Request for review: 8010862: The Method counter fields used for profiling can be allocated lazily

[Jiangli Zhou]

Hi Roland,

Here is updated webrev:

   http://cr.openjdk.java.net/~jiangli/8010862/webrev.01/

Thanks,

Jiangli

On 04/03/2013 05:59 PM, Jiangli Zhou wrote:
> Hi Roland,
>
> Thanks for the review! Please see comments below.
>
> On 04/03/2013 08:35 AM, Roland Westrelin wrote:
>> Hi Jiangli,
>>
>>> Please review the following change for 8010862.
>>>
>>>   http://cr.openjdk.java.net/~jiangli/8010862/webrev.00/
>> parseHelper.cpp:
>>
>> Why did you add a ctrl edge to the store (it didn't have one before)?
>
> It's to avoid the following assert:
>
> #  assert(C->get_alias_index(adr_type) != Compile::AliasIdxRaw || ctl 
> != NULL) failed: raw memory operations should have control edge
>
>
>> c1_LIRGenerator.cpp:
>>
>> 3047   LIR_Opr meth = new_register(T_METADATA);
>> 3048   __ metadata2reg(method->constant_encoding(), meth);
>>
>> is not used unless notify is true, right? I would move that code to the if (notify) {
>
> You are right. Fixed.
>
>> In advancedThresholdPolicy.cpp:
>>
>> If you get an exception in AdvancedThresholdPolicy::update_rate() you're leaving it pending and returning. Shouldn't you clear it?
>>
>> ciMethod.cpp:
>>
>> Same thing with ciMethod::ensure_method_counters(), the exception is left pending.
>>
>> ciReplay.cpp:
>>
>> The method counters allocation can fail here as well, right?
>>
>> interpreterRuntime.cpp:
>> An exception would be left pending in InterpreterRuntime::exception_handler_for_exception().
>
> Thanks for pointing those out. For the issue with 
> AdvancedThresholdPolicy::update_rate() and 
> InterpreterRuntime::exception_handler_for_exception(), how about 
> checking and clearing the pending exception in the private 
> Method::get_method_counters()?
>
>
>    MethodCounters* get_method_counters(TRAPS) {
>      if (_method_counters == NULL) {
>        build_method_counters(this,CHECK_AND_CLEAR_NULL);
>      }
>      return _method_counters;
>    }
>
> For ciMethod::ensure_method_counters() and ciReplay::initialize(), 
> change the calls build_method_counters() to also use the 
> CHECK_AND_CLEAR_* variations?
>
>     mcs = Method::build_method_counters(method,CHECK_AND_CLEAR);
>
>> In compilationPolicy.cpp:
>> The assert(mcs != NULL, "");
>> a useful message why mcs cannot be null here?
>
> Fixed.
>
> Thanks again!
>
> Jiangli
>
>> The rest looks good to me.
>>
>> Roland.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/attachments/20130404/1256500c/attachment-0001.html