RFR(XS) 8191688: Assert failed in > 200 tests: failed dependencies, but counter didn't change

[Nils Eliasson]

On 2017-11-22 20:14, dean.long at oracle.com wrote:
> On 11/22/17 4:13 AM, Vladimir Ivanov wrote:
>
>>> https://bugs.openjdk.java.net/browse/JDK-8191688
>>> http://cr.openjdk.java.net/~dlong/8191688/webrev/
>>
>> The fix looks good.
>>
>
> Thanks Vladimir.  I already pushed it, so I wasn't able to add you as 
> a reviewer.
>
>> An observation not related to the bug:
>>
>> +  if (env->jvmti_can_hotswap_or_post_breakpoint()) {
>>      // 6328518 check hotswap conditions under the right lock.
>>      MutexLocker locker(Compile_lock);
>>      if (Dependencies::check_evol_method(h_m()) != NULL) {
>>        _is_c1_compilable = false;
>>        _is_c2_compilable = false;
>> +      _can_be_parsed = false;
>>      }
>>    } else {
>>
>> Klass* Dependencies::check_evol_method(Method* m) {
>> ...
>>   if (m->is_old()
>>       || m->number_of_breakpoints() > 0) {
>>     return m->method_holder();
>>
>> It seems once there's a breakpoint set in a method, it will never be 
>> compiled again. (I looked in the code couldn't find where 
>> _is_c1_compilable/_is_c2_compilable are reset once all breakpoints 
>> are cleared). If that's the case, it has severe performance 
>> consequences, so better to fix it.
>>
>
> The ciMethod only lasts for the lifetime of the compiler task, right?

Vladmir is right, it will never be compiled again. _is_c*_compilable is 
never reset.

A core problem is that _is_c*_compilable was overloaded with at least 
three different meanings:
1) Have a breakpoint
2) Have had too many failures compiling
3) Have been excluded for compilation by compile command. (And before my 
fix it was also used for not inlining anything that was specified by 3, 
but only after the first compilation attempt. Not at all surprising.)

At these three have different effects - (1) and (2) should exclude 
compilation and inlining, (3) only compilation. (1) and (3) should be 
resettable. (1) applies to all compilation levels, (2) and (3) is per level.

After Deans change 1 is split out into can_be_parsed. That is good, but 
I think the field name should be changed to reflect the reason. The 
effect are clear from the accessor methods (can_be_parsed(), 
can_be_compiled(), should_inline()) that contains easy to read list of 
reasons.

Regards,
Nils


>
>> Also, some nitpicking follows:
>>
>> +  bool _can_be_parsed;
>>
>> +  bool can_be_parsed() const { return _can_be_parsed; }
>>
>> The name confuses me. I'd presume it signals there are some problems 
>> with parsing the method, so don't even try. It seems the choice was 
>> affected by InlineTree::check_can_parse(), but the latter looks 
>> justified:
>>
>> const char* InlineTree::check_can_parse(ciMethod* callee) {
>>   // Certain methods cannot be parsed at all:
>>   if ( callee->is_native())                     return "native method";
>>   if ( callee->is_abstract())                   return "abstract 
>> method";
>>   if (!callee->has_balanced_monitors())         return "not 
>> compilable (unbalanced monitors)";
>>   if ( callee->get_flow_analysis()->failing())  return "not 
>> compilable (flow analysis failed)";
>>
>> Why doesn't it stress it's all about inlining? E.g., 
>> ciMethod::can_be_inlined looks more descriptive in that context.
>>
>
> The way things are now, "can be parsed" seems to mean "can be compiled 
> or inlined".  If there was a way to say a method can be compiled but 
> not inlined, then I agree can_be_inlined would make sense.
>
> dl
>
>> Best regards,
>> Vladimir Ivanov
>