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

[Jiangli Zhou]

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/20130403/c4ece03f/attachment.html